Merge RemoteSourceDescriptorService into DataVaultService #8980
Conversation
📝 WalkthroughWalkthroughReplaces RemoteSourceDescriptor/RemoteSourceDescriptorService with CredentializedUPath/DataVaultService across datastore and tracingstore. Updates vault creation factories, DI wiring, controllers/services, bucket providers, and explorers. Removes RemoteSourceDescriptorService entirely. Overhauls DataVaultService API to provide vaultPathFor overloads and credential handling. Adjusts tests accordingly. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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.
| val credential = remoteSourceDescriptor.credential.map(f => f.asInstanceOf[GoogleServiceAccountCredential]) | ||
| new GoogleCloudDataVault(remoteSourceDescriptor.toUriUnsafe, credential) | ||
| def create(credentializedUpath: CredentializedUPath): GoogleCloudDataVault = { | ||
| val credential = credentializedUpath.credential.map(f => f.asInstanceOf[GoogleServiceAccountCredential]) |
There was a problem hiding this comment.
Consider safer credential type extraction.
The asInstanceOf[GoogleServiceAccountCredential] cast is unsafe and will throw a ClassCastException at runtime if a credential of a different type is provided (e.g., S3AccessKeyCredential).
Consider a safer approach using pattern matching or type-safe extraction:
- val credential = credentializedUpath.credential.map(f => f.asInstanceOf[GoogleServiceAccountCredential])
+ val credential = credentializedUpath.credential.flatMap {
+ case c: GoogleServiceAccountCredential => Some(c)
+ case _ => None
+ }This pattern is already used consistently in S3DataVault.create (lines 184-191 in relevant snippets) and should be applied here as well for consistency.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| val credential = credentializedUpath.credential.map(f => f.asInstanceOf[GoogleServiceAccountCredential]) | |
| val credential = credentializedUpath.credential.flatMap { | |
| case c: GoogleServiceAccountCredential => Some(c) | |
| case _ => None | |
| } |
🤖 Prompt for AI Agents
In
webknossos-datastore/app/com/scalableminds/webknossos/datastore/datavault/GoogleCloudDataVault.scala
around line 118, the direct cast using
asInstanceOf[GoogleServiceAccountCredential] is unsafe; replace it with a
pattern match (or collect/flatMap) on credentializedUpath.credential to safely
extract a GoogleServiceAccountCredential, handling the None or wrong-type cases
by logging/returning a clear error or throwing a descriptive exception rather
than letting a ClassCastException occur, matching the safer approach used in
S3DataVault.create.
| val resolvedMagPath = dataVaultService.resolveMagPath( | ||
| mag, | ||
| absoluteDatasetPath, | ||
| absoluteRealLayerPath, | ||
| absoluteRealLayerPath.getFileName.toString, | ||
| ) |
There was a problem hiding this comment.
Potentially incorrect layerName passed to resolveMagPath (symlink case)
resolveMagPath builds a fallback path using datasetDir.resolve(layerName). Passing absoluteRealLayerPath.getFileName may misresolve relative mag paths when the layer dir is a symlink (basename differs from dataLayer.name). Use the logical layer name.
Apply this diff:
- val resolvedMagPath = dataVaultService.resolveMagPath(
- mag,
- absoluteDatasetPath,
- absoluteRealLayerPath,
- absoluteRealLayerPath.getFileName.toString,
- )
+ val resolvedMagPath = dataVaultService.resolveMagPath(
+ mag,
+ absoluteDatasetPath,
+ absoluteRealLayerPath,
+ dataLayer.name,
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| val resolvedMagPath = dataVaultService.resolveMagPath( | |
| mag, | |
| absoluteDatasetPath, | |
| absoluteRealLayerPath, | |
| absoluteRealLayerPath.getFileName.toString, | |
| ) | |
| val resolvedMagPath = dataVaultService.resolveMagPath( | |
| mag, | |
| absoluteDatasetPath, | |
| absoluteRealLayerPath, | |
| dataLayer.name, | |
| ) |
🤖 Prompt for AI Agents
In
webknossos-datastore/app/com/scalableminds/webknossos/datastore/services/DataSourceService.scala
around lines 133 to 138, the call to dataVaultService.resolveMagPath passes
absoluteRealLayerPath.getFileName which can be incorrect when the layer
directory is a symlink; replace that argument with the logical layer name (e.g.,
the dataLayer.name or the variable that holds the logical layer name) so
resolveMagPath builds its fallback using the logical layer name instead of the
filesystem basename.
| import scala.concurrent.ExecutionContext | ||
|
|
||
| class DataVaultService @Inject()(ws: WSClient, config: DataStoreConfig) extends LazyLogging { | ||
| case class CredentializedUPath(upath: UPath, credential: Option[DataVaultCredential]) |
There was a problem hiding this comment.
Missing import: DataVaultCredential (compile error).
CredentializedUPath references DataVaultCredential without an import. Add the import (and CredentialConfigReader, see below).
Apply this diff near the other imports:
+import com.scalableminds.webknossos.datastore.datavault.{DataVaultCredential, CredentialConfigReader}🤖 Prompt for AI Agents
In
webknossos-datastore/app/com/scalableminds/webknossos/datastore/storage/DataVaultService.scala
around line 26, the case class CredentializedUPath references
DataVaultCredential (and also needs CredentialConfigReader) but these types are
not imported, causing a compile error; add the missing imports for
DataVaultCredential and CredentialConfigReader near the other imports at the top
of the file (follow the existing import grouping and style) so the file
compiles.
| private lazy val globalCredentials = { | ||
| val res = config.Datastore.DataVaults.credentials.flatMap { credentialConfig => | ||
| new CredentialConfigReader(credentialConfig).getCredential | ||
| } | ||
| logger.info(s"Parsed ${res.length} global data vault credentials from datastore config.") | ||
| res | ||
| } |
There was a problem hiding this comment.
Missing import: CredentialConfigReader (compile error).
new CredentialConfigReader(...) is used but not imported.
If not added via the grouped import above, add:
+import com.scalableminds.webknossos.datastore.datavault.CredentialConfigReader🤖 Prompt for AI Agents
In
webknossos-datastore/app/com/scalableminds/webknossos/datastore/storage/DataVaultService.scala
around lines 112 to 118, the code instantiates CredentialConfigReader but the
symbol is not imported, causing a compile error; fix by adding the appropriate
import for CredentialConfigReader at the top of the file (or qualify the class
with its full package name where used) so the compiler can resolve the type.
| private def findGlobalCredentialFor(pathOpt: Option[UPath]): Option[DataVaultCredential] = | ||
| pathOpt.flatMap(path => globalCredentials.find(c => path.toString.startsWith(c.name))) | ||
|
|
There was a problem hiding this comment.
Credential selection matches by naive string prefix; tighten boundary.
startsWith(c.name) can overmatch (e.g., s3://bucket-other). Use an exact-or-boundary check.
- private def findGlobalCredentialFor(pathOpt: Option[UPath]): Option[DataVaultCredential] =
- pathOpt.flatMap(path => globalCredentials.find(c => path.toString.startsWith(c.name)))
+ private def findGlobalCredentialFor(pathOpt: Option[UPath]): Option[DataVaultCredential] =
+ pathOpt.flatMap { path =>
+ val s = path.toString
+ globalCredentials.find { c =>
+ val prefix = c.name.stripSuffix(UPath.separator)
+ s == prefix || s.startsWith(prefix + UPath.separator)
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private def findGlobalCredentialFor(pathOpt: Option[UPath]): Option[DataVaultCredential] = | |
| pathOpt.flatMap(path => globalCredentials.find(c => path.toString.startsWith(c.name))) | |
| private def findGlobalCredentialFor(pathOpt: Option[UPath]): Option[DataVaultCredential] = | |
| pathOpt.flatMap { path => | |
| val s = path.toString | |
| globalCredentials.find { c => | |
| val prefix = c.name.stripSuffix(UPath.separator) | |
| s == prefix || s.startsWith(prefix + UPath.separator) | |
| } | |
| } |
🤖 Prompt for AI Agents
In
webknossos-datastore/app/com/scalableminds/webknossos/datastore/storage/DataVaultService.scala
around lines 120 to 122, the current credential lookup uses startsWith(c.name)
which can overmatch similar prefixes (e.g. s3://bucket matches
s3://bucket-other); change the match to enforce an exact-or-boundary check by
normalizing both path and credential name (trim any trailing slash) and then
return the credential if path == credName or if path startsWith(credName + "/")
(or other valid path separator), ensuring you check equality first and only
accept the prefix when followed by a separator character.
| private def pathIsInLocalDirectoryWhitelist(path: UPath): Boolean = | ||
| path.isLocal && | ||
| config.Datastore.localDirectoryWhitelist.exists(whitelistEntry => path.toString.startsWith(whitelistEntry)) | ||
|
|
There was a problem hiding this comment.
Whitelist check uses string prefix; use normalized Path startsWith to avoid bypass.
String prefix allows /allowed-other to pass. Normalize and compare Path prefixes.
- private def pathIsInLocalDirectoryWhitelist(path: UPath): Boolean =
- path.isLocal &&
- config.Datastore.localDirectoryWhitelist.exists(whitelistEntry => path.toString.startsWith(whitelistEntry))
+ private def pathIsInLocalDirectoryWhitelist(path: UPath): Boolean =
+ path.isLocal && {
+ val candidate = path.toLocalPathUnsafe.toAbsolutePath.normalize
+ config.Datastore.localDirectoryWhitelist.exists { entry =>
+ val base = java.nio.file.Path.of(entry).toAbsolutePath.normalize
+ candidate.startsWith(base)
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private def pathIsInLocalDirectoryWhitelist(path: UPath): Boolean = | |
| path.isLocal && | |
| config.Datastore.localDirectoryWhitelist.exists(whitelistEntry => path.toString.startsWith(whitelistEntry)) | |
| private def pathIsInLocalDirectoryWhitelist(path: UPath): Boolean = | |
| path.isLocal && { | |
| val candidate = path.toLocalPathUnsafe.toAbsolutePath.normalize | |
| config.Datastore.localDirectoryWhitelist.exists { entry => | |
| val base = java.nio.file.Path.of(entry).toAbsolutePath.normalize | |
| candidate.startsWith(base) | |
| } | |
| } |
🤖 Prompt for AI Agents
In
webknossos-datastore/app/com/scalableminds/webknossos/datastore/storage/DataVaultService.scala
around lines 158 to 161, the whitelist check currently compares the path string
with whitelist entries using startsWith which can be bypassed (e.g.
/allowed-other); instead parse and normalize whitelist entries into UPath
instances and use a proper path-prefix check (UPath.startsWith or comparable
normalized Path.startsWith) so both the candidate path and whitelist entries are
normalized (resolve ., .., symlinks as appropriate) before comparison; update
the predicate to convert whitelistEntry -> UPath.of(whitelistEntry).normalized
and return path.normalized.startsWith(whitelistUPath) (or use toRealPath/resolve
if symlink resolution is required) to ensure correct prefix matching.
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: