Merge RemoteSourceDescriptorService into DataVaultService #8980
Conversation
📝 WalkthroughWalkthroughReplaced RemoteSourceDescriptor and RemoteSourceDescriptorService with CredentializedUPath and a reworked DataVaultService. Updated datavault factory signatures, DI, controllers, providers, services, explorers, tracing layers, and tests to use DataVaultService/CredentializedUPath and new vaultPathFor/createVault APIs. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Areas to focus:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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: BindDataVaultServicedirectly without shadowing the option.Pattern matching on
dataVaultServiceOptascase 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) orcase 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
📒 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
bucketProvidersignature update to acceptOption[DataVaultService]is consistent with the project-wide refactoring. The implementation correctly delegates tovolumeBucketProvider.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
createmethod signature and implementation are correctly updated to acceptCredentializedUPath. Credential extraction logic for S3AccessKeyCredential and LegacyDataVaultCredential is preserved, and URI derivation usescredentializedUpath.upath.toRemoteUriUnsafeappropriately.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.runOptionalto handle the optional UsableDataSource, andforallensures all explicit paths are checked viadataVaultService.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
CredentializedUPathinstead ofRemoteSourceDescriptor, and the URI derivation correctly usescredentializedUpath.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 aRemoteSourceDescriptorfirst. 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
DataVaultServiceinstead ofRemoteSourceDescriptorService, aligning with the repository-wide refactoring to consolidate vault path resolution.
35-35: LGTM! Vault path resolution consistently updated.All three usages of
vaultPathForhave been correctly updated to calldataVaultService.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
DataVaultServiceinstead ofRemoteSourceDescriptorService, consistent with the refactoring across the datastore module.
650-650: LGTM! Path validation correctly delegated.The call to
pathIsAllowedToAddDirectlycorrectly usesdataVaultServiceinstead of the previousremoteSourceDescriptorService, 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
DataVaultServiceinstead ofRemoteSourceDescriptorService, maintaining consistency with the repository-wide refactoring.
68-68: LGTM! Vault path resolution consistently updated.Both usages of
vaultPathForhave been correctly updated to calldataVaultService.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
vaultPathForhave been correctly updated to calldataVaultService.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
DataVaultServiceinstead ofRemoteSourceDescriptorService, maintaining consistency with the repository-wide refactoring.
24-30: LGTM! Service correctly passed to BinaryDataService.The
BinaryDataServiceinstantiation correctly passesSome(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
DataVaultServiceinstead ofRemoteSourceDescriptorService, completing the consistent refactoring pattern across all Zarr-based services.
57-57: LGTM! Vault path resolution consistently updated.Both usages of
vaultPathForhave been correctly updated to calldataVaultService.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 correctAll 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 consistentConstructor 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: LGTMSignature 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: OKSignature 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.
...os-datastore/app/com/scalableminds/webknossos/datastore/datavault/GoogleCloudDataVault.scala
Show resolved
Hide resolved
...nossos-datastore/app/com/scalableminds/webknossos/datastore/services/DataSourceService.scala
Show resolved
Hide resolved
webknossos-datastore/app/com/scalableminds/webknossos/datastore/storage/DataVaultService.scala
Show resolved
Hide resolved
webknossos-datastore/app/com/scalableminds/webknossos/datastore/storage/DataVaultService.scala
Show resolved
Hide resolved
webknossos-datastore/app/com/scalableminds/webknossos/datastore/storage/DataVaultService.scala
Show resolved
Hide resolved
webknossos-datastore/app/com/scalableminds/webknossos/datastore/storage/DataVaultService.scala
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/connectome/ZarrConnectomeFileService.scala (1)
182-190: Second call site replacement — LGTM.Zarr3Array.open now receives a VaultPath derived via DataVaultService, consistent with the refactor.
Minor: factor a small helper to resolve groupVaultPath once, reducing duplication between attributes and array open paths.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
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/services/BinaryDataServiceHolder.scala(2 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/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)
🚧 Files skipped from review as they are similar to previous changes (5)
- webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mapping/ZarrAgglomerateService.scala
- webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/DataSourceController.scala
- webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/uploading/UploadService.scala
- webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/segmentindex/ZarrSegmentIndexFileService.scala
- webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/BinaryDataServiceHolder.scala
🧰 Additional context used
🧬 Code graph analysis (2)
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/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)
⏰ 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: build-smoketest-push
- GitHub Check: backend-tests
🔇 Additional comments (8)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/mesh/ZarrMeshFileService.scala (3)
16-16: LGTM!The import statement has been correctly updated to use
DataVaultServiceinstead ofRemoteSourceDescriptorService, consistent with the refactoring objective.
62-62: LGTM!The constructor dependency has been correctly updated to inject
DataVaultServiceinstead ofRemoteSourceDescriptorService, aligning with the service consolidation effort.
73-73: LGTM!The method calls have been correctly updated to use
dataVaultService.vaultPathFor(meshFileKey.attachment). The API signature is preserved, and based on the relevant code snippets,DataVaultServiceproperly supports thevaultPathFormethod forLayerAttachmentparameters, returning the expectedFox[VaultPath]type.Also applies to: 152-152
webknossos-datastore/app/com/scalableminds/webknossos/datastore/DataStoreModule.scala (2)
31-31: Import swap to DataVaultService looks correct.Matches the PR intent to retire RemoteSourceDescriptorService.
43-43: Verified: DI wiring is correct and cleanup is complete.DataVaultService is properly defined and actively used across the codebase (injected in VolumeTracingLayer). The eager singleton binding at line 43 follows the established pattern for all module-level services and ensures availability for downstream dependencies. No lingering RemoteSourceDescriptor references were found—the removal was fully cleaned up.
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/connectome/ZarrConnectomeFileService.scala (3)
13-13: Import updated to DataVaultService — good.Keeps the service dependency surface consistent post-merge.
44-49: Constructor now injects DataVaultService — approve; verify cache isolation across credentials.Injection change is correct. One caveat: openArraysCache/attributesCache are keyed without a credential/tenant dimension. If DataVaultService resolves different credentials per TokenContext for the same ConnectomeFileKey, a cached DatasetArray/attributes created under one user could be reused under another.
Please confirm whether:
- vault resolution for connectome attachments is invariant across users, or
- credentials are global for these sources.
If not invariant, consider either:
- keying caches with a stable vault/credential fingerprint, or
- not caching DatasetArray at this layer and relying on the shared chunk cache.
Happy to propose a patch once you confirm the expected behavior.
57-63: Call site switched to dataVaultService.vaultPathFor(attachment) — LGTM.Implicit ExecutionContext and TokenContext are in scope here; aligns with DataVaultService API.
MichaelBuessemeyer
left a comment
MichaelBuessemeyer
left a comment
There was a problem hiding this comment.
Awesome, looks good. Did not find anything and testing worked 🎉
This service depends on datastore config + datastore rpc client. It cannot be instantiated in the webknossos module. It is also not needed there. Follow-up fix for #8980
- When scanning datasources from directories, always add resolved mag paths immediately - Introduces realpaths for attachments - Report all realpaths that could be determined, scale down logging for those that couldn’t. ### URL of deployed dev instance (used for testing): - https://___.webknossos.xyz ### Steps to test: - start wk with some datasets in the binaryData dir, watch backend logging, should show realpath scan failures (provoke some by deleting mags or attachments that are referenced in some datasource-properties.jsons) - All realpaths that could be determined should be added to the database ### TODOs: - [x] immediate paths for datasources - [x] simplify layerpath logic - [x] attachment realpath column - [x] don’t fail for datasource on single realpath failure - [x] log failures - [x] scan also attachments - [x] evolution - [x] adapt s3-delete code to also consider realpaths of attachments ### Issues: - blocked by #8924 (paths routes changed) - blocked by #8980 (datavault path refactoring) - fixes #9018 ------ - [x] Added changelog entry (create a `$PR_NUMBER.md` file in `unreleased_changes` or use `./tools/create-changelog-entry.py`) - [x] Removed dev-only changes like prints and application.conf edits - [x] Considered [common edge cases](../blob/master/.github/common_edge_cases.md) - [x] Needs datastore update after deployment --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: MichaelBuessemeyer <[email protected]>
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: