Skip to content

Merge RemoteSourceDescriptorService into DataVaultService #8980

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Merge RemoteSourceDescriptorService into DataVaultService #8980

wants to merge 4 commits into from

Conversation

fm3
Copy link
Member

@fm3 fm3 commented Oct 7, 2025 •
edited
Loading

While it was nice that the DataVaultService was small and clean, it was weird that callers needed to use RemoteSourceDescriptorService to get their vaultpaths. Lately the separation felt artificial.

I also renamed the dataclass RemoteSourceDescriptor to CredentializedUPath, which is hopefully more descriptive.

URL of deployed dev instance (used for testing):

Steps to test:

  • Load some datasets, both local and remote, should show data.
  • Explore remote datast with credentials, should work.
  • Removed dev-only changes like prints and application.conf edits
  • Considered common edge cases
  • Needs datastore update after deployment

@fm3 fm3 self-assigned this Oct 7, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 7, 2025 •
edited
Loading

📝 Walkthrough

Walkthrough

Removed RemoteSourceDescriptor and RemoteSourceDescriptorService; introduced CredentializedUPath and expanded DataVaultService APIs. Updated factories, DI, controllers, services, providers, explorers, tracing layers, and tests to use DataVaultService/CredentializedUPath and vaultPathFor/createVault signatures.

Changes

Cohort / File(s) Summary
Tests update
test/backend/DataVaultTestSuite.scala		 Replace RemoteSourceDescriptor with CredentializedUPath in vault creation calls for HTTPS, GCS, and S3; no logic changes.
DataVault factories
webknossos-datastore/.../datavault/HttpsDataVault.scala, .../datavault/GoogleCloudDataVault.scala, .../datavault/S3DataVault.scala		 Factory create methods now accept CredentializedUPath instead of RemoteSourceDescriptor; credential and URI extraction updated.
DataVaultService overhaul
webknossos-datastore/.../storage/DataVaultService.scala		 Introduces CredentializedUPath; adds multiple vaultPathFor overloads (UPath, Path, MagLocator, Attachment), credential resolution helpers, cache invalidation variants, and createVault/removeVaultFromCache operating on CredentializedUPath.
Remove RemoteSourceDescriptorService
webknossos-datastore/.../storage/RemoteSourceDescriptorService.scala		 Deletes RemoteSourceDescriptor DTO and entire RemoteSourceDescriptorService implementation and helpers.
DI / module / controllers
webknossos-datastore/.../DataStoreModule.scala, .../controllers/DataSourceController.scala		 Remove binding/injection of RemoteSourceDescriptorService; inject/use DataVaultService instead and update call sites (e.g., path validation).
Upload & DataSource services
webknossos-datastore/.../services/uploading/UploadService.scala, .../services/DataSourceService.scala		 Replace RemoteSourceDescriptorService with DataVaultService in constructors and usages (pathIsAllowedToAddDirectly, vault cache ops, resolveMagPath).
Providers & models
webknossos-datastore/.../dataformats/DatasetArrayBucketProvider.scala, .../models/datasource/DataLayer.scala		 bucketProvider signatures switch to Option[DataVaultService]; providers call dataVaultServiceOpt.vaultPathFor(...); LazyLogging mixed into provider.
Binary data / holders
webknossos-datastore/.../services/BinaryDataService.scala, .../services/BinaryDataServiceHolder.scala, .../services/DSUsedStorageService.scala		 Constructor parameters and usages updated to DataVaultService; forward dataVaultServiceOpt to bucket providers and use vaultPathFor.
Exploration utilities
webknossos-datastore/.../explore/ExploreLocalLayerService.scala, .../explore/ExploreRemoteLayerService.scala, .../explore/NeuroglancerUriExplorer.scala		 Replace RemoteSourceDescriptor construction with direct vaultPathFor/CredentializedUPath lookups; remove descriptor intermediate.
Mesh / connectome / segment services
webknossos-datastore/.../services/connectome/ZarrConnectomeFileService.scala, .../services/mapping/ZarrAgglomerateService.scala, .../services/mesh/NeuroglancerPrecomputedMeshFileService.scala, .../services/mesh/ZarrMeshFileService.scala, .../services/segmentindex/ZarrSegmentIndexFileService.scala		 Replace injected RemoteSourceDescriptorService with DataVaultService; update vaultPathFor call sites accordingly.
Tracingstore layers
webknossos-tracingstore/.../tracings/editablemapping/EditableMappingLayer.scala, .../tracings/volume/VolumeTracingLayer.scala		 Public bucketProvider signatures now accept Option[DataVaultService]; imports and parameter names updated; delegation unchanged.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Suggested reviewers

  • frcroth
  • normanrz

Poem

I munched a token, hopped a trail, swapped descriptors for a key,
Vaults now hold my upaths snug — credentialized and free.
Tests hopped green, DI rearranged, caches thumped in time,
I twitch my whiskers, waltz the patch — a carrot-coded rhyme. 🥕🐇

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title Check ✅ Passed The title succinctly states the main refactor of merging RemoteSourceDescriptorService into DataVaultService, accurately reflecting the core change in the pull request while remaining concise and clear.
Description Check ✅ Passed The description explains the rationale for merging the services, notes the renaming of RemoteSourceDescriptor to CredentializedUPath, and includes concrete testing steps and a deployment URL, directly relating to the changeset without being off-topic.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch refactor-datavault-service
📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7bf68a3 and f0c484e.

📒 Files selected for processing (1)
  • webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/DataSourceController.scala (3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/DataSourceController.scala (1)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/storage/DataVaultService.scala (2)
  • DataVaultService (28-191)
  • pathIsAllowedToAddDirectly (142-148)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: build-smoketest-push
  • GitHub Check: frontend-tests
  • GitHub Check: backend-tests
🔇 Additional comments (3)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/DataSourceController.scala (3)

41-41: LGTM! Import updated correctly.

The import statement has been correctly updated to reflect the new dependency on DataVaultService.

80-80: LGTM! Dependency injection updated correctly.

The constructor parameter has been correctly updated from remoteSourceDescriptorService: RemoteSourceDescriptorService to dataVaultService: DataVaultService, maintaining consistency with the refactoring objective.

685-685: LGTM! Method call updated correctly.

The call to pathIsAllowedToAddDirectly has been correctly updated to use the new dataVaultService instance. The method signature and usage are compatible, and no other references to the old service remain in this file.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@fm3 fm3 marked this pull request as ready for review October 7, 2025 11:48
coderabbitai[bot]
coderabbitai bot reviewed Oct 7, 2025
View reviewed changes
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/dataformats/DatasetArrayBucketProvider.scala (1)

72-117: Bind DataVaultService directly without shadowing the option.

Pattern matching on dataVaultServiceOpt as case Some(dataVaultServiceOpt: DataVaultService) both shadows the outer option and performs a redundant type annotation. This is confusing and violates the “Don’t shadow values” guidance. Use a fresh name (svc) or case Some(dataVaultService) instead.

Apply this diff:

-        dataVaultServiceOpt match {
-          case Some(dataVaultServiceOpt: DataVaultService) =>
+        dataVaultServiceOpt match {
+          case Some(dataVaultService) =>
             for {
-              magPath: VaultPath <- dataVaultServiceOpt.vaultPathFor(magLocator,
+              magPath: VaultPath <- dataVaultService.vaultPathFor(magLocator,
🧹 Nitpick comments (2)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/storage/DataVaultService.scala (2)

87-88: Use provided layerDir instead of recomputing it.

Slightly cleaner and avoids divergence if computation changes elsewhere.

-          val pathRelativeToLayer =
-            localDatasetDir.resolve(layerName).resolve(magLocatorPath.toLocalPathUnsafe).normalize
+          val pathRelativeToLayer =
+            layerDir.resolve(magLocatorPath.toLocalPathUnsafe).normalize

34-35: Cache key includes full credential object; consider using an identity key.

Using the full credential in the cache key can reduce hit rate (rotating tokens) and stores secrets in keys. Prefer a stable identifier (e.g., credentialId or credential.name).

Consider a key like (UPath, Option[String]) where the string is credentialId for request-scoped creds or c.name for global creds.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1eea8e7 and 188d316.

📒 Files selected for processing (25)
  • test/backend/DataVaultTestSuite.scala (12 hunks)
  • webknossos-datastore/app/com/scalableminds/webknossos/datastore/DataStoreModule.scala (1 hunks)
  • webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/DataSourceController.scala (3 hunks)
  • webknossos-datastore/app/com/scalableminds/webknossos/datastore/dataformats/DatasetArrayBucketProvider.scala (3 hunks)
  • webknossos-datastore/app/com/scalableminds/webknossos/datastore/datavault/GoogleCloudDataVault.scala (2 hunks)
  • webknossos-datastore/app/com/scalableminds/webknossos/datastore/datavault/HttpsDataVault.scala (2 hunks)
  • webknossos-datastore/app/com/scalableminds/webknossos/datastore/datavault/S3DataVault.scala (2 hunks)
  • webknossos-datastore/app/com/scalableminds/webknossos/datastore/explore/ExploreLocalLayerService.scala (3 hunks)
  • webknossos-datastore/app/com/scalableminds/webknossos/datastore/explore/ExploreRemoteLayerService.scala (2 hunks)
  • webknossos-datastore/app/com/scalableminds/webknossos/datastore/explore/NeuroglancerUriExplorer.scala (2 hunks)
  • webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/datasource/DataLayer.scala (3 hunks)
  • webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/BinaryDataService.scala (3 hunks)
  • webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/BinaryDataServiceHolder.scala (2 hunks)
  • webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/DSUsedStorageService.scala (3 hunks)
  • webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/DataSourceService.scala (4 hunks)
  • webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/connectome/ZarrConnectomeFileService.scala (4 hunks)
  • webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mapping/ZarrAgglomerateService.scala (3 hunks)
  • webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/NeuroglancerPrecomputedMeshFileService.scala (5 hunks)
  • webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/ZarrMeshFileService.scala (4 hunks)
  • webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/segmentindex/ZarrSegmentIndexFileService.scala (4 hunks)
  • webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/uploading/UploadService.scala (3 hunks)
  • webknossos-datastore/app/com/scalableminds/webknossos/datastore/storage/DataVaultService.scala (2 hunks)
  • webknossos-datastore/app/com/scalableminds/webknossos/datastore/storage/RemoteSourceDescriptorService.scala (0 hunks)
  • webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingLayer.scala (2 hunks)
  • webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/VolumeTracingLayer.scala (2 hunks)
💤 Files with no reviewable changes (1)
  • webknossos-datastore/app/com/scalableminds/webknossos/datastore/storage/RemoteSourceDescriptorService.scala
🧰 Additional context used
🧬 Code graph analysis (23)
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/VolumeTracingLayer.scala (3)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/storage/DataVaultService.scala (1)
  • DataVaultService (28-191)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/datasource/DataLayer.scala (1)
  • bucketProvider (73-78)
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingLayer.scala (1)
  • bucketProvider (87-93)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/DataSourceController.scala (1)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/storage/DataVaultService.scala (2)
  • DataVaultService (28-191)
  • pathIsAllowedToAddDirectly (142-148)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/uploading/UploadService.scala (4)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/storage/DataVaultService.scala (2)
  • DataVaultService (28-191)
  • pathIsAllowedToAddDirectly (142-148)
util/src/main/scala/com/scalableminds/util/tools/Fox.scala (1)
  • runOptional (159-169)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/datasource/DataLayer.scala (1)
  • allExplicitPaths (44-51)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/datasource/DataSource.scala (1)
  • allExplicitPaths (97-97)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/DSUsedStorageService.scala (1)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/storage/DataVaultService.scala (6)
  • DataVaultService (28-191)
  • vaultPathFor (37-40)
  • vaultPathFor (42-45)
  • vaultPathFor (45-53)
  • vaultPathFor (53-59)
  • vaultPathFor (162-167)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/explore/ExploreRemoteLayerService.scala (1)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/storage/DataVaultService.scala (7)
  • DataVaultService (28-191)
  • CredentializedUPath (26-26)
  • vaultPathFor (37-40)
  • vaultPathFor (42-45)
  • vaultPathFor (45-53)
  • vaultPathFor (53-59)
  • vaultPathFor (162-167)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mapping/ZarrAgglomerateService.scala (2)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/storage/AgglomerateFileCache.scala (1)
  • AgglomerateFileKey (13-13)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/storage/DataVaultService.scala (6)
  • DataVaultService (28-191)
  • vaultPathFor (37-40)
  • vaultPathFor (42-45)
  • vaultPathFor (45-53)
  • vaultPathFor (53-59)
  • vaultPathFor (162-167)
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingLayer.scala (3)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/storage/DataVaultService.scala (1)
  • DataVaultService (28-191)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/datasource/DataLayer.scala (1)
  • bucketProvider (73-78)
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/VolumeTracingLayer.scala (2)
  • bucketProvider (117-122)
  • bucketProvider (122-122)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/BinaryDataServiceHolder.scala (2)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/storage/DataVaultService.scala (1)
  • DataVaultService (28-191)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/DataStoreConfig.scala (1)
  • Datastore (18-65)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/explore/NeuroglancerUriExplorer.scala (1)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/storage/DataVaultService.scala (6)
  • DataVaultService (28-191)
  • vaultPathFor (37-40)
  • vaultPathFor (42-45)
  • vaultPathFor (45-53)
  • vaultPathFor (53-59)
  • vaultPathFor (162-167)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/connectome/ZarrConnectomeFileService.scala (1)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/storage/DataVaultService.scala (6)
  • DataVaultService (28-191)
  • vaultPathFor (37-40)
  • vaultPathFor (42-45)
  • vaultPathFor (45-53)
  • vaultPathFor (53-59)
  • vaultPathFor (162-167)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/BinaryDataService.scala (3)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/datasource/DataLayer.scala (1)
  • bucketProvider (73-78)
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingLayer.scala (1)
  • bucketProvider (87-93)
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/VolumeTracingLayer.scala (2)
  • bucketProvider (117-122)
  • bucketProvider (122-122)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/dataformats/DatasetArrayBucketProvider.scala (1)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/storage/DataVaultService.scala (6)
  • DataVaultService (28-191)
  • vaultPathFor (37-40)
  • vaultPathFor (42-45)
  • vaultPathFor (45-53)
  • vaultPathFor (53-59)
  • vaultPathFor (162-167)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/DataSourceService.scala (1)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/storage/DataVaultService.scala (6)
  • DataVaultService (28-191)
  • resolveMagPath (79-105)
  • resolveMagPath (105-110)
  • removeVaultFromCache (59-66)
  • removeVaultFromCache (66-72)
  • removeVaultFromCache (167-170)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/ZarrMeshFileService.scala (1)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/storage/DataVaultService.scala (6)
  • DataVaultService (28-191)
  • vaultPathFor (37-40)
  • vaultPathFor (42-45)
  • vaultPathFor (45-53)
  • vaultPathFor (53-59)
  • vaultPathFor (162-167)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/models/datasource/DataLayer.scala (3)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/storage/DataVaultService.scala (1)
  • DataVaultService (28-191)
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingLayer.scala (1)
  • bucketProvider (87-93)
webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/VolumeTracingLayer.scala (2)
  • bucketProvider (117-122)
  • bucketProvider (122-122)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/datavault/GoogleCloudDataVault.scala (4)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/storage/DataVaultService.scala (1)
  • CredentializedUPath (26-26)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/datavault/HttpsDataVault.scala (1)
  • create (166-168)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/datavault/S3DataVault.scala (1)
  • create (185-192)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/datavault/VaultPath.scala (1)
  • toRemoteUriUnsafe (75-78)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/storage/DataVaultService.scala (8)
util/src/main/scala/com/scalableminds/util/tools/Fox.scala (10)
  • Fox (30-230)
  • Fox (232-305)
  • shiftBox (312-312)
  • flatMap (284-294)
  • flatMap (379-379)
  • s (236-240)
  • s (240-250)
  • s (250-259)
  • find (149-159)
  • successful (53-56)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/dataformats/MagLocator.scala (2)
  • MagLocator (10-15)
  • MagLocator (17-19)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/helpers/PathSchemes.scala (1)
  • PathSchemes (3-9)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/helpers/UPath.scala (2)
  • UPath (54-96)
  • fromLocalPath (80-80)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/DataStoreConfig.scala (3)
  • Datastore (18-65)
  • DataVaults (61-63)
  • Http (14-16)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/datavault/GoogleCloudDataVault.scala (4)
  • getCredential (103-103)
  • GoogleCloudDataVault (18-114)
  • GoogleCloudDataVault (116-121)
  • create (117-120)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/datavault/HttpsDataVault.scala (4)
  • getCredential (144-144)
  • create (166-168)
  • HttpsDataVault (22-155)
  • HttpsDataVault (157-168)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/datavault/S3DataVault.scala (4)
  • getCredential (170-170)
  • create (185-192)
  • S3DataVault (46-182)
  • S3DataVault (184-276)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/explore/ExploreLocalLayerService.scala (1)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/storage/DataVaultService.scala (6)
  • DataVaultService (28-191)
  • vaultPathFor (37-40)
  • vaultPathFor (42-45)
  • vaultPathFor (45-53)
  • vaultPathFor (53-59)
  • vaultPathFor (162-167)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/segmentindex/ZarrSegmentIndexFileService.scala (1)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/storage/DataVaultService.scala (6)
  • DataVaultService (28-191)
  • vaultPathFor (37-40)
  • vaultPathFor (42-45)
  • vaultPathFor (45-53)
  • vaultPathFor (53-59)
  • vaultPathFor (162-167)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/datavault/S3DataVault.scala (5)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/storage/DataVaultService.scala (1)
  • CredentializedUPath (26-26)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/datavault/GoogleCloudDataVault.scala (1)
  • create (117-120)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/datavault/HttpsDataVault.scala (1)
  • create (166-168)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/storage/DataVaultCredentials.scala (1)
  • toS3AccessKey (68-75)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/datavault/VaultPath.scala (1)
  • toRemoteUriUnsafe (75-78)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/datavault/HttpsDataVault.scala (3)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/storage/DataVaultService.scala (1)
  • CredentializedUPath (26-26)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/datavault/GoogleCloudDataVault.scala (1)
  • create (117-120)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/datavault/S3DataVault.scala (1)
  • create (185-192)
test/backend/DataVaultTestSuite.scala (5)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/storage/DataVaultCredentials.scala (2)
  • GoogleServiceAccountCredential (48-54)
  • GoogleServiceAccountCredential (56-58)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/storage/DataVaultService.scala (1)
  • CredentializedUPath (26-26)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/datavault/HttpsDataVault.scala (3)
  • HttpsDataVault (22-155)
  • HttpsDataVault (157-168)
  • create (166-168)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/datavault/GoogleCloudDataVault.scala (3)
  • create (117-120)
  • GoogleCloudDataVault (18-114)
  • GoogleCloudDataVault (116-121)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/datavault/S3DataVault.scala (3)
  • create (185-192)
  • S3DataVault (46-182)
  • S3DataVault (184-276)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/NeuroglancerPrecomputedMeshFileService.scala (1)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/storage/DataVaultService.scala (6)
  • DataVaultService (28-191)
  • vaultPathFor (37-40)
  • vaultPathFor (42-45)
  • vaultPathFor (45-53)
  • vaultPathFor (53-59)
  • vaultPathFor (162-167)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: frontend-tests
  • GitHub Check: backend-tests
  • GitHub Check: build-smoketest-push
🔇 Additional comments (32)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/explore/NeuroglancerUriExplorer.scala (1)

10-10: LGTM! Clean migration to DataVaultService.

The replacement of RemoteSourceDescriptor with direct vault path resolution via dataVaultService.vaultPathFor(upath) is correct and simplifies the code. Error handling is preserved with the same message.

Also applies to: 47-47

webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mapping/ZarrAgglomerateService.scala (1)

17-17: LGTM! Dependency migration is correct.

The service dependency swap from RemoteSourceDescriptorService to DataVaultService is consistent with the PR's refactoring goals. The call site correctly uses dataVaultService.vaultPathFor(agglomerateFileKey.attachment).

Also applies to: 27-27, 54-54

webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/volume/VolumeTracingLayer.scala (1)

15-15: LGTM! Method signature aligned with broader migration.

The bucketProvider signature update to accept Option[DataVaultService] is consistent with the project-wide refactoring. The implementation correctly delegates to volumeBucketProvider.

Also applies to: 117-120

webknossos-datastore/app/com/scalableminds/webknossos/datastore/DataStoreModule.scala (1)

31-31: LGTM! Dependency injection configuration updated correctly.

The removal of RemoteSourceDescriptorService from the module bindings and retention of DataVaultService aligns with the consolidation of vault services.

webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/DSUsedStorageService.scala (1)

8-8: LGTM! Service dependency correctly updated.

The migration from RemoteSourceDescriptorService to DataVaultService is correct, with the call site properly updated to use dataVaultService.vaultPathFor(pathPair.upath).

Also applies to: 34-34, 67-67

webknossos-datastore/app/com/scalableminds/webknossos/datastore/explore/ExploreRemoteLayerService.scala (1)

12-12: LGTM! Clean adoption of CredentializedUPath API.

The migration from RemoteSourceDescriptor construction to direct vault path resolution using CredentializedUPath(upath, credentialOpt) simplifies the code while maintaining the same functionality. Error handling is preserved.

Also applies to: 88-88

webknossos-datastore/app/com/scalableminds/webknossos/datastore/datavault/S3DataVault.scala (1)

7-7: LGTM! Factory method migrated to CredentializedUPath correctly.

The create method signature and implementation are correctly updated to accept CredentializedUPath. Credential extraction logic for S3AccessKeyCredential and LegacyDataVaultCredential is preserved, and URI derivation uses credentializedUpath.upath.toRemoteUriUnsafe appropriately.

Also applies to: 185-191

webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/uploading/UploadService.scala (1)

20-20: LGTM! Service migration and path validation correctly implemented.

The dependency swap to DataVaultService is correct. The path validation logic at lines 385-387 properly uses Fox.runOptional to handle the optional UsableDataSource, and forall ensures all explicit paths are checked via dataVaultService.pathIsAllowedToAddDirectly.

Also applies to: 95-95, 385-387

webknossos-datastore/app/com/scalableminds/webknossos/datastore/datavault/GoogleCloudDataVault.scala (1)

117-119: LGTM! Signature correctly updated to use CredentializedUPath.

The factory method signature has been properly updated to accept CredentializedUPath instead of RemoteSourceDescriptor, and the URI derivation correctly uses credentializedUpath.upath.toRemoteUriUnsafe.

webknossos-datastore/app/com/scalableminds/webknossos/datastore/explore/ExploreLocalLayerService.scala (2)

50-50: LGTM! Simplified vault path resolution.

The direct call to dataVaultService.vaultPathFor(dir) is cleaner than the previous approach of constructing a RemoteSourceDescriptor first. The error message is preserved.

136-136: LGTM! Consistent vault path resolution.

The direct call to dataVaultService.vaultPathFor(path.resolve(layer)) follows the same pattern as line 50, providing consistent path resolution across the exploration logic.

webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/NeuroglancerPrecomputedMeshFileService.scala (2)

26-29: LGTM! Constructor correctly updated.

The constructor now properly injects DataVaultService instead of RemoteSourceDescriptorService, aligning with the repository-wide refactoring to consolidate vault path resolution.

35-35: LGTM! Vault path resolution consistently updated.

All three usages of vaultPathFor have been correctly updated to call dataVaultService.vaultPathFor(meshFileKey.attachment) instead of the previous service, maintaining functional correctness.

Also applies to: 78-78, 107-107

webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/DataSourceController.scala (2)

70-70: LGTM! Dependency correctly updated.

The constructor now properly injects DataVaultService instead of RemoteSourceDescriptorService, consistent with the refactoring across the datastore module.

650-650: LGTM! Path validation correctly delegated.

The call to pathIsAllowedToAddDirectly correctly uses dataVaultService instead of the previous remoteSourceDescriptorService, maintaining the same validation logic.

webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/segmentindex/ZarrSegmentIndexFileService.scala (2)

52-54: LGTM! Constructor correctly updated.

The constructor now properly injects DataVaultService instead of RemoteSourceDescriptorService, maintaining consistency with the repository-wide refactoring.

68-68: LGTM! Vault path resolution consistently updated.

Both usages of vaultPathFor have been correctly updated to call dataVaultService.vaultPathFor(segmentIndexFileKey.attachment), preserving functional correctness while using the new service API.

Also applies to: 129-129

webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/ZarrMeshFileService.scala (2)

62-65: LGTM! Constructor correctly updated.

The constructor now properly injects DataVaultService, maintaining consistency with other Zarr-based services in this refactoring.

73-73: LGTM! Vault path resolution consistently updated.

Both usages of vaultPathFor have been correctly updated to call dataVaultService.vaultPathFor(meshFileKey.attachment), preserving the mesh file reading logic while using the new service API.

Also applies to: 152-152

webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/BinaryDataServiceHolder.scala (2)

19-22: LGTM! Constructor correctly updated.

The constructor now properly injects DataVaultService instead of RemoteSourceDescriptorService, maintaining consistency with the repository-wide refactoring.

24-30: LGTM! Service correctly passed to BinaryDataService.

The BinaryDataService instantiation correctly passes Some(dataVaultService) instead of the previous service, maintaining the singleton pattern for the shared bucket provider cache.

webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/connectome/ZarrConnectomeFileService.scala (2)

44-46: LGTM! Constructor correctly updated.

The constructor now properly injects DataVaultService instead of RemoteSourceDescriptorService, completing the consistent refactoring pattern across all Zarr-based services.

57-57: LGTM! Vault path resolution consistently updated.

Both usages of vaultPathFor have been correctly updated to call dataVaultService.vaultPathFor(connectomeFileKey.attachment), preserving the connectome file reading logic while using the new service API.

Also applies to: 182-182

test/backend/DataVaultTestSuite.scala (1)

44-44: CredentializedUPath migration in tests looks correct

All create(...) call sites correctly wrap UPath and credentials with CredentializedUPath, matching the new APIs (HTTPS/GCS/S3). No issues spotted.

Also applies to: 58-58, 90-93, 107-107, 128-128, 142-142, 156-156, 170-170, 183-183, 197-197, 211-211

webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/DataSourceService.scala (1)

27-27: DI migration to DataVaultService is consistent

Constructor and cache invalidation now route via DataVaultService. Looks good.

Also applies to: 275-278

webknossos-datastore/app/com/scalableminds/webknossos/datastore/datavault/HttpsDataVault.scala (1)

161-168: Factory now accepts CredentializedUPath: LGTM

Signature and implementation updated consistently; integrates with DataVaultService.create path routing.

webknossos-tracingstore/app/com/scalableminds/webknossos/tracingstore/tracings/editablemapping/EditableMappingLayer.scala (1)

87-90: bucketProvider signature updated to DataVaultService: OK

Signature matches the new API. Implementation remains valid.

webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/BinaryDataService.scala (1)

24-25: bucketProvider overrides updated and no stale references found
Verified no occurrences of RemoteSourceDescriptorService; all bucketProvider overrides accept Option[DataVaultService].

webknossos-datastore/app/com/scalableminds/webknossos/datastore/storage/DataVaultService.scala (4)

176-178: Verify HttpsDataVault dataStoreHost argument.

HttpsDataVault.create expects a host string (per scaladoc). You’re passing config.Http.uri (likely a full URI). Confirm it behaves as intended.

If a host is required, pass only the host component (or adjust HttpsDataVault to accept a full URI consistently).

37-58: Overloads for vaultPathFor look good.

API surface is coherent and covers UPath, local Path, MagLocator, and LayerAttachment. Flow for credential resolution is consistent.

170-186: Creation paths per scheme look correct; message hygiene ok.

Scheme dispatch and error reporting are sound. Logging avoids printing credentials.

167-169: No change needed: removeVaultFromCache already returns Fox[Unit]
AlfuCache.remove is defined to return Unit, so Fox.successful(vaultCache.remove(...)) correctly yields Fox[Unit]. The proposed refactor is unnecessary.

Likely an incorrect or invalid review comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments

@MichaelBuessemeyer MichaelBuessemeyer Awaiting requested review from MichaelBuessemeyer

At least 1 approving review is required to merge this pull request.

Assignees

@fm3 fm3

Labels

backend refactoring

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@fm3