Refactor backend: Use “folder” only for dashboard folders, otherwise “directory” #8292
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive renaming effort across multiple files to distinguish between "folder" and "directory" terminology. The changes primarily focus on configuration options, method signatures, and variable names in the webknossos-datastore and webknossos-tracingstore projects. Key modifications include renaming configuration properties like Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
webknossos-tracingstore/conf/com.scalableminds.webknossos.tracingstore.routes (2)
51-52: Consider consolidating trailing slash variants
Both routes with and without a trailing slash are defined, which is typically fine for backward compatibility. To streamline, consider automatically handling the trailing slash in your routing framework if no backward compatibility requirement exists.
63-64: Trailing slash usage
Similar to lines 51-52, you have a trailing slash variant. Evaluate whether to unify these routes if you do not need to differentiate them for specific client requests or to maintain backward compatibility.webknossos-datastore/app/com/scalableminds/webknossos/datastore/storage/RemoteSourceDescriptorService.scala (1)
63-68: Consider using a more specific exception type and adding contextual details.While this logic correctly enforces the new
localDirectoryWhitelist, using a genericExceptionmight hamper error handling and troubleshooting. You could throw a custom exception (e.g.,IllegalArgumentException) or integrate more contextual details (like organization/dataset identifiers) to provide better visibility into potential mismatches.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
MIGRATIONS.unreleased.md(1 hunks)app/utils/WkConf.scala(0 hunks)test/e2e/End2EndSpec.scala(1 hunks)util/src/main/scala/com/scalableminds/util/io/ZipIO.scala(1 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/DataStoreConfig.scala(1 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/ExportsController.scala(1 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/ZarrStreamingController.scala(6 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/explore/ExploreRemoteLayerService.scala(1 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/AgglomerateService.scala(1 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/BinaryDataServiceHolder.scala(1 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/ConnectomeFileService.scala(1 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/DSRemoteTracingstoreClient.scala(1 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/DSUsedStorageService.scala(1 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/DataSourceService.scala(1 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/MappingService.scala(1 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/MeshFileService.scala(1 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/SegmentIndexFileService.scala(1 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/uploading/UploadService.scala(1 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/storage/RemoteSourceDescriptorService.scala(1 hunks)webknossos-datastore/conf/com.scalableminds.webknossos.datastore.routes(1 hunks)webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/controllers/VolumeTracingZarrStreamingController.scala(4 hunks)webknossos-tracingstore/conf/com.scalableminds.webknossos.tracingstore.routes(1 hunks)
💤 Files with no reviewable changes (1)
- app/utils/WkConf.scala
✅ Files skipped from review due to trivial changes (8)
- webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/AgglomerateService.scala
- webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/uploading/UploadService.scala
- webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/DSUsedStorageService.scala
- webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/ConnectomeFileService.scala
- webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/SegmentIndexFileService.scala
- webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/DSRemoteTracingstoreClient.scala
- webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/ZarrStreamingController.scala
- webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/controllers/VolumeTracingZarrStreamingController.scala
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (24)
webknossos-tracingstore/conf/com.scalableminds.webknossos.tracingstore.routes (2)
44-47: Consistent naming convention for directory-based endpoints
These routes correctly replace "folder" nomenclature with "directory," aligning well with the PR objective. The method names in theVolumeTracingZarrStreamingControllerappear to match these new routes.
57-60: Good extension to Zarr v3 routes
The transition to the "directory" naming for Zarr v3 routes is consistent and clear.test/e2e/End2EndSpec.scala (1)
65-65: Rename aligns with PR objective.The call to
ZipIO.unzipToDirectoryis consistent with the rename from “folder” to “directory” mandated by this PR. Good to see that the method usage here now matches the updated utility methods.util/src/main/scala/com/scalableminds/util/io/ZipIO.scala (2)
278-283: Maintains consistent naming and behavior.Renaming
unzipToFoldertounzipToDirectoryclarifies the intended usage for filesystem paths, fully aligning with the PR’s goal of distinguishing “directories” from “folders” in the codebase. The parameters remain the same, so existing logic is preserved.
287-292: Implementation carried over correctly.The new
unzipToDirectorymethod (accepting aZipFileinstead of aFile) mirrors the logic of the earlier approach while improving naming clarity. Good job streamlining the code.webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/MappingService.scala (1)
23-23: Consistent Renaming of Base DirectorySwitching from
config.Datastore.baseFoldertoconfig.Datastore.baseDirectoryaligns perfectly with the PR objective of unifying terminology. The logic remains unchanged and should continue functioning without issue.webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/ExportsController.scala (1)
34-34: Terminology Change: Base DirectoryRenaming this variable to
baseDirectoryfrombaseFolderis consistent with the unified naming approach introduced in this PR. This change is straightforward and fully aligns with the updated configuration property.webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/BinaryDataServiceHolder.scala (1)
45-45: Unified Terminology for Directory HandlingUsing
config.Datastore.baseDirectoryclarifies that the path refers to a filesystem directory rather than a dashboard folder. This renaming completes the move toward consistent vocabulary across the codebase.webknossos-datastore/app/com/scalableminds/webknossos/datastore/DataStoreConfig.scala (1)
23-24: Property Renaming in ConfigurationRenaming
baseFoldertobaseDirectoryandlocalFolderWhitelisttolocalDirectoryWhitelistreflects the new convention for directory paths and ensures consistency throughout the configuration options. These changes meet the PR goal of distinguishing between dashboard folders and filesystem directories.webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/DataSourceService.scala (1)
44-44: This renaming aligns well with the new terminology.The update from
baseFoldertobaseDirectoryis consistent with the broader PR objective and aids in clarity. No concerns here.webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/MeshFileService.scala (1)
180-180: Terminology update looks good.Changing
baseFoldertobaseDirectoryis consistent with the codebase-wide renaming strategy.webknossos-datastore/conf/com.scalableminds.webknossos.datastore.routes (12)
19-20: Consistently renamed routes for Zarr data source.The new naming convention aligns with the PR objectives of distinguishing “directory” from “folder.” This appears consistent and helps avoid confusion in the future.
23-24: Good consistency for data layer route.Renaming to
requestDataLayerDirectoryContentsclarifies that it deals with filesystem directories, in line with the broader refactor.
27-28: Maintains naming clarity for magnification sub-routes.Renaming to
requestDataLayerMagDirectoryContentsis consistent with the rest of the changes. No concerns noted.
32-33: Private link route rename is clear.Renaming to
dataSourceDirectoryContentsPrivateLinkmirrors the underlying concept of directories vs. folders. Well done.
36-37: Consistent rename for data layer directory in private link.The route name properly reflects the usage of “directory.” This also ensures uniform usage of new terminology across private link endpoints.
40-41: Magnification route naming is aligned with “directory” refactor.Using “Directory” in
dataLayerMagDirectoryContentsPrivateLinkclarifies the endpoint’s nature.
46-47: Zarr 3 experimental routes maintain consistent directory naming.Adoption of directory-based terminology remains consistent with the rest of the file. Looks good.
49-50: Renamed data layer routes for zarr3_experimental.This continues the “directory” nomenclature. The approach is uniform across all zarr versions.
52-53: Magnification sub-routes remain consistent.No additional caveats; changes match the standardized naming practice introduced by the PR.
57-58: Annotations in zarr3_experimental also follow directory pattern.The usage of “directory” is consistent, so references to “folder” do not appear.
60-61: Data layer directory naming in annotations.No issues spotted. The rename is coherent with the approach used in the other zarr endpoints.
63-64: Magnification route for zarr3_experimental is updated to directory naming.The naming scheme remains consistent throughout. Nice job.
MIGRATIONS.unreleased.md (1)
12-12: Clear rename of datastore configuration options.Renaming
datastore.baseFolder→datastore.baseDirectoryandlocalFolderWhitelist→localDirectoryWhitelistaligns perfectly with the new convention. Make sure to update any references or environment variable docs to avoid confusion.
| bool2Fox(dataStoreConfig.Datastore.localDirectoryWhitelist.exists(whitelistEntry => | ||
| uri.getPath.startsWith(whitelistEntry))) ?~> s"Absolute path ${uri.getPath} in local file system is not in path whitelist. Consider adding it to datastore.localDirectoryWhitelist" |
There was a problem hiding this comment.
Validate using canonical path checks instead of simple startsWith.
Relying on startsWith may introduce a security loophole (e.g., /allowed vs. /allowed_evil). To avoid prefix-based false positives or negatives, consider normalizing both the target path and the whitelist entries with toRealPath() or a similar method to ensure robust checks.
Apply this diff to implement canonical path checks:
- bool2Fox(dataStoreConfig.Datastore.localDirectoryWhitelist.exists(whitelistEntry =>
- uri.getPath.startsWith(whitelistEntry))) ?~> s"Absolute path ${uri.getPath} ...
+ val localCanonicalPath = Paths.get(uri.getPath).toRealPath().toString
+ bool2Fox(dataStoreConfig.Datastore.localDirectoryWhitelist.exists(whitelistEntry =>
+ localCanonicalPath.startsWith(Paths.get(whitelistEntry).toRealPath().toString))) ?~>
+ s"Absolute path $localCanonicalPath ...Committable suggestion skipped: line range outside the PR's diff.
Unified terminology: we decided a while ago that “folder” should refer only to the dashboard dataset folders that are implemented in postgres, while “directory” should refer to the actual filesystem. This PR adapts the backend config and code to follow this terminology.
Steps to test
Issues: