feat(dispatch): multi s3 tenant - #153
Conversation
WalkthroughThis update refactors the dispatch-service to support multi-tenant operations. The changes remove the old Changes
Sequence Diagram(s)sequenceDiagram
participant DC as DispatcherUseCase
participant SA as Streaming Adapters
participant FS as FileSystemOutPort/S3Adapter
participant SP as S3Properties
participant MC as MinioClient
DC->>SA: dispatchFile(event with tenant)
SA->>FS: Invoke file operations (tag, move, read) with tenant info
FS->>SP: getClient(tenant)
SP->>MC: Return tenant-specific client
MC-->>SP: Client operations complete
SP-->>FS: Client provided
FS-->>DC: File processed/finished or error handled
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes detected. All code changes align with the stated objectives related to multi-tenant support. Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (11)
dispatch-service/src/test/resources/application-test.yml (4)
2-5: Introduce Tenant-Specific S3 Configuration.
The news3.tenantsmapping correctly registers atest-tenantwith its associated URL (https://s3.muenchen.de). This setup is essential for routing S3 operations under a multi-tenant strategy. Please verify that downstream components utilize this configuration correctly when instantiating tenant-specific S3 clients.
10-12: Link Use-Case 'test-meta' to Tenant.
The addition oftenant: test-tenantto thetest-metause-case ensures that file operations are directed to the correct tenant environment. Confirm that all relevant adapters and use case implementations correctly reference this tenant configuration.
21-23: Attach Tenant Information for Use-Case 'test-meta-dest'.
Adding thetenant: test-tenantparameter for thetest-meta-destuse-case is consistent with the multi-tenancy objective. It’s important to ensure that tenant-specific routing in the business logic is updated accordingly.
31-33: Assign Tenant for Use-Case 'test2'.
The configuration now includestenant: test-tenantfor thetest2use-case, which promotes consistency across your use case configurations. Make sure that test scenarios and service components are updated to simulate and validate tenant-aware behavior.dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/application/port/out/ReadProtocolOutPort.java (1)
17-17: JavaDoc needs to be updated to include the new tenant parameter.The method signature now includes a
tenantparameter, but the JavaDoc comments don't document this parameter. Consider updating the JavaDoc to include a description for the tenant parameter./** * Load parsed protocol. * * @param bucket The bucket of the protocol file. * @param path The path of the protocol file. + * @param tenant The tenant identifier for multi-tenant support. * @return The parsed protocol entries. */dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/application/port/in/MarkFileFinishedInPort.java (1)
15-17: Good use of a structured object instead of multiple parameters.Using a structured
FileEventobject is a better design approach than using multiple separate parameters. The validation annotations ensure proper validation.Consider enhancing the JavaDoc description to provide more detail about what information the FileEvent contains and its significance:
/** * Mark a file as finished processing. * - * @param event The event for the file to marks as finished. + * @param event The event for the file to mark as finished, containing useCase, presignedUrl, and metadata information. */dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/application/usecase/MarkFileFinishedUseCase.java (1)
33-39: Good addition of metadata file handlingThe new code to handle metadata files is a valuable improvement. It properly checks if the metadata URL exists before processing it, verifies the URL, and finishes the file with tenant awareness.
Consider adding a log statement before processing the metadata file to aid in debugging, such as:
if (Strings.isNotBlank(event.metadataPresignedUrl())) { + log.debug("Processing metadata file from URL: {}", event.metadataPresignedUrl()); // verify presigned url and extract metadata File final File metadataFile = this.fileSystemOutPort.verifyAndResolvePresignedUrl(event.metadataPresignedUrl()); // finish metadata file fileHandlingHelper.finishFile(useCase, metadataFile.tenant(), metadataFile.bucket(), metadataFile.path()); }dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/application/usecase/ProtocolProcessingUseCase.java (1)
88-89: Check for potential repeated calls.
You're callinggetMatchingFilesWithTagsmultiple times for similar operations. Consider extracting a helper method to reduce duplication and improve maintainability.dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/application/usecase/ErrorHandlerUseCase.java (1)
39-39: Fallback error notification.
Providing a fallback address and sending the presigned URL is a good fallback mechanism; just ensure no sensitive info is exposed in production logs.dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/adapter/out/s3/S3Properties.java (1)
79-92: Tenant lookup by URL.
ThefindTenantByUrlmethod is straightforward. Consider normalizing URLs (e.g., removing trailing slashes) if needed to avoid mismatches. Otherwise, this is fine.dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/adapter/out/s3/S3Adapter.java (1)
209-226: Refactor GET to HEAD for lighter verification (optional).
Using GET withRange=0-0works but has some overhead. A HEAD request might be more efficient for verifying presence.-connection.setRequestMethod("GET"); +connection.setRequestMethod("HEAD");
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/adapter/in/streaming/FileEventDTO.java(0 hunks)dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/adapter/in/streaming/StreamingInAdapter.java(2 hunks)dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/adapter/out/s3/S3Adapter.java(17 hunks)dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/adapter/out/s3/S3Properties.java(1 hunks)dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/adapter/out/streaming/FileEventDTO.java(0 hunks)dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/adapter/out/streaming/StreamingOutAdapter.java(2 hunks)dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/application/port/in/ErrorHandlerInPort.java(2 hunks)dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/application/port/in/MarkFileFinishedInPort.java(1 hunks)dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/application/port/out/FileSystemOutPort.java(8 hunks)dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/application/port/out/ReadProtocolOutPort.java(1 hunks)dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/application/usecase/DispatcherUseCase.java(6 hunks)dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/application/usecase/ErrorHandlerUseCase.java(3 hunks)dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/application/usecase/MarkFileFinishedUseCase.java(2 hunks)dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/application/usecase/ProtocolProcessingUseCase.java(4 hunks)dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/application/usecase/helper/FileHandlingHelper.java(2 hunks)dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/domain/model/File.java(1 hunks)dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/domain/model/FileEvent.java(1 hunks)dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/domain/model/UseCase.java(1 hunks)dispatch-service/src/main/resources/application-local.yml(1 hunks)dispatch-service/src/test/java/de/muenchen/oss/swim/dispatcher/TestConstants.java(2 hunks)dispatch-service/src/test/java/de/muenchen/oss/swim/dispatcher/application/usecase/DispatcherUseCaseTest.java(9 hunks)dispatch-service/src/test/java/de/muenchen/oss/swim/dispatcher/application/usecase/ErrorHandlerUseCaseTest.java(3 hunks)dispatch-service/src/test/java/de/muenchen/oss/swim/dispatcher/application/usecase/MarkFileFinishedUseCaseTest.java(3 hunks)dispatch-service/src/test/java/de/muenchen/oss/swim/dispatcher/application/usecase/ProtocolProcessingUseCaseTest.java(5 hunks)dispatch-service/src/test/java/de/muenchen/oss/swim/dispatcher/domain/model/FileTest.java(2 hunks)dispatch-service/src/test/resources/application-test.yml(3 hunks)
💤 Files with no reviewable changes (2)
- dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/adapter/out/streaming/FileEventDTO.java
- dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/adapter/in/streaming/FileEventDTO.java
🧰 Additional context used
🧬 Code Definitions (1)
dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/adapter/out/s3/S3Properties.java (1)
dispatch-service/src/test/java/de/muenchen/oss/swim/dispatcher/TestConstants.java (1)
NoArgsConstructor(10-31)
🪛 GitHub Actions: build all
dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/application/usecase/ProtocolProcessingUseCase.java
[warning] 130-130: Error file processing test/inProcess/path/path.csv for use case test-meta
dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/application/usecase/ErrorHandlerUseCase.java
[error] 38-38: Error while handling error: java.lang.NullPointerException: Cannot invoke "de.muenchen.oss.swim.dispatcher.domain.model.File.tenant()" because "file" is null
dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/application/usecase/DispatcherUseCase.java
[warning] 105-105: Error while processing file test/inProcess/path/example2.pdf for use case test-meta
🔇 Additional comments (72)
dispatch-service/src/main/resources/application-local.yml (1)
70-74: Multi-Tenant S3 Configuration Structured CorrectlyThe new nested structure under the
s3key—with tenant-specific settings undertenants.swm—is correctly implemented to support multi-tenant operations. Please ensure that the application components responsible for reading these configuration values have been updated accordingly to traverse this new hierarchy. Additionally, consider updating the configuration documentation so that it clearly reflects these changes for future maintainability and ease of onboarding.dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/domain/model/FileEvent.java (1)
5-9: Well-designed record for the file event domain model.The introduction of this record as part of the domain model is appropriate for representing file events in a multi-tenant context. The validation constraints on required fields ensure proper input validation while keeping the structure immutable and concise.
dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/domain/model/UseCase.java (1)
17-21: Good addition of tenant field to support multi-tenancy.The new tenant field with proper validation and documentation is essential for the multi-tenant S3 functionality. The
@NotBlankannotation ensures that tenant information will always be provided, which is critical for tenant isolation.dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/adapter/out/streaming/StreamingOutAdapter.java (2)
4-4: Good update of import to use domain model.The import now correctly references the new FileEvent domain model, which aligns with good Domain-Driven Design practices.
17-17: Successfully migrated to the new FileEvent domain model.The code now correctly instantiates the new FileEvent record instead of the previous FileEventDTO, while maintaining the same parameter structure for backward compatibility.
dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/domain/model/File.java (1)
5-5:Details
✅ Verification successful
Tenant addition supports multi-tenancy requirement.
The addition of the tenant field to the File record is appropriate for implementing multi-tenant functionality. The @notblank annotation ensures that a valid tenant value is always provided.
Since the static
fromPresignedUrlmethod has been removed (according to the AI summary), ensure there's a clear alternative way to create File objects from URLs that incorporates tenant information, possibly in a service or helper class.
🏁 Script executed:
#!/bin/bash # Check if there is an alternative method to create File objects from presigned URLs rg -A 10 "fromPresignedUrl|File\s+.*\s+.*presigned|presignedUrl.*File" --no-headingLength of output: 16154
Multi-Tenancy Verified – Alternative URL Parsing in Place
The tenant field addition in the File record is correctly enforced with the @notblank annotation. As discussed, the removal of the static
fromPresignedUrlmethod has been addressed: file creation from presigned URLs now happens in the S3Adapter (and via the FileSystemOutPort) where tenant information is extracted from the URL. The grep search confirms this alternative mechanism is actively used in multiple use cases (e.g., in the MarkFileFinishedUseCase and ErrorHandlerUseCase).Please ensure that this alternative URL parsing logic is clearly documented so that future developers can readily understand how File objects are created with tenant data.
dispatch-service/src/test/java/de/muenchen/oss/swim/dispatcher/application/usecase/MarkFileFinishedUseCaseTest.java (3)
48-55: Tests properly updated for multi-tenant support.The test has been correctly updated to use the new
FileEventmodel and include tenant information in the verifications. The change fromverifyPresignedUrltoverifyAndResolvePresignedUrlsuggests enhanced functionality to handle tenant-specific resolution.Good job ensuring that the verification steps include explicit checks for the tenant parameter.
60-61: Exception test properly updated.The test for
PresignedUrlExceptionhas been correctly modified to use the new method nameverifyAndResolvePresignedUrland to pass the file event object.
66-67: UseCaseException test properly updated.The test now creates a new
FileEventwith an unknown use case, which is the right approach for testing the exception case with the new parameter structure.dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/application/port/in/ErrorHandlerInPort.java (1)
4-4: Good refactoring to use FileEvent for error handlingThe switch from individual parameters to a consolidated
FileEventobject improves the interface design by encapsulating related data. This change aligns well with the multi-tenant implementation by containing tenant-specific context within a single object, reducing method complexity and enhancing maintainability.Also applies to: 14-15, 17-17
dispatch-service/src/test/java/de/muenchen/oss/swim/dispatcher/application/usecase/DispatcherUseCaseTest.java (2)
8-8: Added tenant support and updated test dataAdding the TENANT constant import and updating the metadata path from "test.json" to "example.json" maintains test consistency with other changes in the implementation.
Also applies to: 57-57
84-86: Well-implemented tenant support across all file operationsThe consistent addition of the
TENANTparameter to all file system operations (getSubDirectories, getMatchingFilesWithTags, fileExists, getPresignedUrl, tagFile, copyFile) effectively implements multi-tenant functionality. The parameter ordering is logical, with tenant preceding bucket, which aligns with the hierarchical relationship between these concepts.Also applies to: 101-103, 120-123, 129-130, 149-150, 158-158, 165-165, 183-184, 201-203
dispatch-service/src/test/java/de/muenchen/oss/swim/dispatcher/application/usecase/ProtocolProcessingUseCaseTest.java (2)
7-7: Added tenant support and improved test data consistencyThe addition of the TENANT import and updating test file constructors to include the tenant parameter correctly implements multi-tenant functionality. The update of protocol entry file names from "test.pdf" to "example.pdf" improves consistency with other test data changes throughout the codebase.
Also applies to: 27-27, 67-72
76-80: Comprehensive tenant integration in protocol processing testsThe method signature change to include PresignedUrlException handling and the consistent addition of the TENANT parameter across all protocol-related operations ensures proper multi-tenant support throughout the protocol processing flow. All file operations (loadProtocol, getMatchingFilesWithTags, readFile, tagFile, moveFile) now correctly consider tenant context.
Also applies to: 84-88, 92-92, 95-97, 98-102, 104-105, 112-116, 124-134, 139-142, 150-151
dispatch-service/src/test/java/de/muenchen/oss/swim/dispatcher/application/usecase/ErrorHandlerUseCaseTest.java (2)
3-5: Updated imports to support tenant functionalityThe import changes properly support the implementation of tenant-aware error handling by including FILE1, TEST_FILE_EVENT, when for mocking, and PresignedUrlException for exception handling.
Also applies to: 12-12, 19-19
54-60: Enhanced error handling with tenant-aware file eventsThe test now properly uses the consolidated TEST_FILE_EVENT object instead of separate parameters and includes verification of presigned URLs. The addition of PresignedUrlException handling ensures the test correctly validates the error path for URL verification failures, which is critical for the multi-tenant implementation.
Also applies to: 63-63, 72-73
dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/application/usecase/MarkFileFinishedUseCase.java (1)
25-32: Method properly updated for multi-tenant supportThe method signature has been successfully updated to accept a
FileEventparameter instead of separate parameters. The implementation correctly extracts the useCase and file information from the event, and properly includes the tenant information withfile.tenant()when finishing the file.dispatch-service/src/test/java/de/muenchen/oss/swim/dispatcher/TestConstants.java (3)
17-18: Good addition of tenant constantAdding the
TENANTconstant is a good practice for maintaining test consistency in the multi-tenant implementation.
22-24: Properly updated File constants for multi-tenant supportThe
FILE1andFILE2constants have been correctly updated to include tenant information.
28-31:Details
❓ Verification inconclusive
Verify presigned URL consistency with tenant information
The presigned URL constant doesn't seem to include the tenant information, only the bucket name. While this might be intentional based on your URL structure, ensure it's consistent with how your S3 client uses tenant information in the actual implementation.
🏁 Script executed:
#!/bin/bash # Check how presigned URLs are generated in the codebase # Find implementations of URL generation rg -n "presignedUrl|generatePresigned" --type javaLength of output: 6863
Attention: Verify Tenant Inclusion in Test Presigned URL
It appears that production code (e.g., in
DispatcherUseCase.javawhere the URL is built usingfile.tenant(),file.bucket(), andfile.path()) constructs the presigned URL with tenant details. However, the test constant inTestConstants.java(used to createFileEvent) only provides a file path and seems to omit any tenant information. Please verify whether the test presigned URL should mimic the complete structure—including the tenant identifier—or if tenant details are incorporated elsewhere in the test setup. Adjust the test constant or document the handling accordingly.dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/adapter/in/streaming/StreamingInAdapter.java (2)
24-33: Successfully updated to use FileEventThe
finished()method has been properly updated to work with the newFileEventmodel instead ofFileEventDTO. The code is now cleaner, passing the entire event object rather than extracting individual fields.
36-42: Error handling updated for FileEventThe
dlq()method has been properly updated to use theFileEventmodel and correctly passes it to the error handler.dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/application/usecase/helper/FileHandlingHelper.java (2)
34-44: Successfully updated for multi-tenant supportThe
markFileErrormethod has been correctly updated to include tenant information when tagging files.
46-64: Method signature and implementation updated for multi-tenant supportThe
finishFilemethod has been properly updated to include the tenant parameter in its signature and implementation. All necessary method calls now include tenant information, and the documentation has been updated accordingly.dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/application/usecase/ProtocolProcessingUseCase.java (6)
47-47: Tenant retrieval logic looks consistent.
The explicit usage ofuseCase.getTenant()aligns with the new multi-tenant approach, ensuring the correct tenant is passed to file system operations. No immediate issues detected.
84-84: Confirm tenant handling inloadProtocolcall.
Passingfile.tenant()toloadProtocolis correct for multi-tenant support. Verify that thereadProtocolOutPorthandles missing or invalid tenants gracefully.
92-93: Use consistent filtering logic for finished paths.
Usingfile.tenant(), file.bucket(), finishedPathis sound. Confirm that recursive flags in these calls match your intended search scope.
108-108: Tenant-specific readFile operation looks correct.
No functional or syntax issues found here.
123-123: Tagging files with tenant context is appropriate.
Ensure the tag keys and values do not exceed S3 or system-defined size limits.
128-128: Multi-tenant move operation.
Moving files withfile.tenant()is aligned with the new architecture. No immediate issues found.dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/application/usecase/ErrorHandlerUseCase.java (5)
10-10: NewFileEventimport.
The introduction ofFileEventimproves clarity by bundling relevant file and use-case data, removing the need for multiple parameters.
28-29: Handling error with new event-based signature.
Switching toFileEventis correct for multi-tenant expansions. Log usage is clear and includes the cause details.
36-36: Error notification references newFileEvent.
The updated call tosendFileErrorusesuseCase.getMailAddresses()and thefile.path(). This is well-aligned with the multi-tenant approach and the new event model.
42-42: Metric increment for error tracking.
dispatchMeter.incrementErroris effectively parameterized byuseCase()andcause.source(). This is consistent with multi-tenant error tracking.
56-56: Tagging error on file may trigger NPE.
As noted above, iffileis null, callingfile.tenant()will fail. Ensure a null-check to avoid runtime errors.dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/adapter/out/s3/S3Properties.java (4)
3-4: New imports forUseCaseandMinioClient.
These additions are valid for the revised approach, storing tenant-based Minio clients.
7-13: Refactor to standard imports and Lombok annotations.
Adopting@NoArgsConstructor,AllArgsConstructor,Getter,Setter, andToStringfosters clarity. Make sure to handle any sensitive fields with caution intoString().
17-21: Consolidated Lombok usage.
These annotations are consistent with the rest of the codebase, removing the older@Dataapproach.
29-34: Tenant map structure is appropriate.
StoringConnectionOptionsper tenant in a nested property is a clean approach for multi-tenant S3 configurations.dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/application/usecase/DispatcherUseCase.java (6)
59-59: Ensure the tenant matches the intended context.
The additional tenant parameter is passed correctly togetSubDirectories. This seems consistent with multi-tenant support.
86-86: Tenant parameter usage looks correct.
The parameter is passed here without issues.
193-205: Verifyfile.tenant()vs.useCase.getTenant()usage.
Here, the code usesfile.tenant()calls. In other methods,useCase.getTenant()is used. Ensure that the file’s tenant always aligns with the use case tenant to prevent unintended mismatches or file-access errors.
223-223: Safely reading metadata file.
Passingfile.tenant()toreadFileis consistent with the multi-tenant approach. No immediate issues.
245-249: Check for consistency of tenant usage.
In the if-condition,useCase.getTenant()is checked, whereasfinishFilecalls usefile.tenant(). Confirm that these two always match.
283-283: Copying file with tenant-specific logic looks good.
Usingfile.tenant()here is consistent with how the file was instantiated.dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/application/port/out/FileSystemOutPort.java (9)
26-32: New tenant parameter ingetMatchingFilesWithTags.
This addition aligns well with multi-tenant needs.
41-43: Tenant parameter ingetSubDirectories.
Looks correct; no concerns.
52-52: Tenant parameter intagFile.
No immediate issues with the added parameter.
61-61: Tenant parameter infileExists.
Implementation is consistent with the interface changes.
70-70: Tenant parameter inreadFile.
Matches the overall multi-tenant strategy.
79-79: Tenant parameter ingetPresignedUrl.
No issues.
82-87: Renamed method and changed return type.
verifyAndResolvePresignedUrlreturning aFileobject is a clean approach to unify verification and resolution steps.
96-96: Tenant parameter inmoveFile.
Continues the tenant-aware pattern.
108-109: Tenant parameter incopyFile.
Method signature changes are consistent.dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/adapter/out/s3/S3Adapter.java (16)
32-32: No issues with the new import.
Thejakarta.validation.constraints.NotBlankimport is appropriate.
52-52: ImportedStringsutility.
This helps with blank-checking.
81-87: Addedtenantparameter ingetMatchingFilesWithTags.
Implementation looks cohesive for multi-tenant support.
89-93: ConstructingFileobjects with tenant.
Ensures each file is tenant-scoped. Good consistency.
100-100: Retrieving file tags with tenant context.
Properly delegated togetTagsOfFile.
115-119: Tenant-based subdirectory retrieval.
Logic seems consistent and straightforward.
125-128: Tenant parameter intagFile.
Loading and updating tags in a tenant-specific context looks correct.
142-142: InvokingsetObjectTagson the tenant-specific client.
Implementation is correct.
151-157: Tenant parameter infileExists.
Remains consistent with the approach.
176-182: Reading file with tenant context.
No issues identified.
191-199: Generating presigned URL for tenant.
Implementation looks solid.
229-244: Tenant-basedmoveFile.
Properly copies and removes the object using the tenant-specific client.
255-269: Tenant-basedcopyFile.
No issues spotted; the logic is straightforward.
287-296: Listing objects with tenant-based client.
Ensures correct scoping for multi-tenant data.
318-324: Tenant-specific object tags retrieval.
Implementation looks correct and exception handling is sensible.
372-401: Validate path parsing infileFromPresignedUrl.
The substring logic to isolate bucket and path may fail if any unexpected slashes appear. Consider stricter validation or error handling for unusual URLs.dispatch-service/src/test/java/de/muenchen/oss/swim/dispatcher/domain/model/FileTest.java (2)
3-4: Good refactoring to use constants instead of hardcoded values.Importing constants from a centralized
TestConstantsclass improves maintainability and consistency across tests. This aligns well with the multi-tenant architecture changes.
13-13: Proper tenant parameter added to File constructor.The File constructor now accepts a tenant parameter as the first argument, correctly implementing the multi-tenant functionality required by this PR. This change ensures that file operations are now tenant-aware, which is a key objective of the PR.
# Conflicts: # dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/application/usecase/ProtocolProcessingUseCase.java
simonhir
changed the title
# Conflicts: # dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/adapter/in/streaming/StreamingInAdapter.java # dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/adapter/out/s3/S3Adapter.java # dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/domain/model/File.java # dispatch-service/src/main/resources/application-local.yml # dispatch-service/src/test/java/de/muenchen/oss/swim/dispatcher/application/usecase/DispatcherUseCaseTest.java
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (2)
dispatch-service/src/test/java/de/muenchen/oss/swim/dispatcher/application/usecase/ErrorHandlerUseCaseTest.java (1)
68-77: Missing mock setup for presigned URL verification failure.The test
handleError_PresignedUrlExceptiondoesn't mock thefileSystemOutPort.verifyAndResolvePresignedUrlmethod to throw aPresignedUrlException. This test appears to be testing the error scenario but may not properly simulate the exception case.Apply this diff to properly simulate the exception scenario:
@Test void handleError_PresignedUrlException() { final UseCase useCase = swimDispatcherProperties.getUseCases().getFirst(); + // setup + when(fileSystemOutPort.verifyAndResolvePresignedUrl(eq(useCase), eq(TEST_PRESIGNED_URL))) + .thenThrow(new PresignedUrlException("Invalid presigned URL")); // call errorHandlerUseCase.handleError(TEST_FILE_EVENT, TEST_ERROR_DETAILS);dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/application/port/out/FileSystemOutPort.java (1)
102-113: Consider supporting cross-tenant copy operations.The
copyFilemethod only accepts a single tenant parameter, which may limit cross-tenant file operations. If use cases can have different tenants (as suggested by the multi-tenant architecture), copying files between use cases would require separate source and destination tenant parameters.Consider updating the signature to support cross-tenant operations:
- void copyFile(@NotBlank String tenant, @NotBlank String srcBucket, @NotBlank String srcPath, @NotBlank String destBucket, @NotBlank String destPath, - boolean clearTags); + void copyFile(@NotBlank String srcTenant, @NotBlank String srcBucket, @NotBlank String srcPath, + @NotBlank String destTenant, @NotBlank String destBucket, @NotBlank String destPath, + boolean clearTags);
♻️ Duplicate comments (1)
dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/application/usecase/ErrorHandlerUseCase.java (1)
32-34: Add null check for file resolution.The
verifyAndResolvePresignedUrlmethod could return null, but line 34 uses thefileobject without checking. This could cause a NullPointerException.Add a null check as suggested in the previous review:
final File file = this.fileSystemOutPort.verifyAndResolvePresignedUrl(useCase, event.presignedUrl()); +if (file == null) { + log.warn("File resolution returned null for presigned URL {}", event.presignedUrl()); + return; +} // tag file this.markFileError(file, cause);
🧹 Nitpick comments (1)
dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/application/port/out/ReadProtocolOutPort.java (1)
17-17: Update Javadoc to document the tenant parameter.The method signature correctly adds the tenant parameter with proper validation, but the Javadoc should be updated to document this new parameter.
/** * Load parsed protocol. * + * @param tenant The tenant context for S3 operations. * @param bucket The bucket of the protocol file. * @param path The path of the protocol file. * @return The parsed protocol entries. */
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/adapter/in/streaming/FileEventDTO.java(0 hunks)dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/adapter/in/streaming/StreamingInAdapter.java(2 hunks)dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/adapter/out/s3/S3Adapter.java(18 hunks)dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/adapter/out/s3/S3Properties.java(1 hunks)dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/adapter/out/streaming/FileEventDTO.java(0 hunks)dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/adapter/out/streaming/StreamingOutAdapter.java(2 hunks)dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/application/port/in/ErrorHandlerInPort.java(2 hunks)dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/application/port/in/MarkFileFinishedInPort.java(1 hunks)dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/application/port/out/FileSystemOutPort.java(9 hunks)dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/application/port/out/ReadProtocolOutPort.java(1 hunks)dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/application/usecase/DispatcherUseCase.java(6 hunks)dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/application/usecase/ErrorHandlerUseCase.java(3 hunks)dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/application/usecase/MarkFileFinishedUseCase.java(2 hunks)dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/application/usecase/ProtocolProcessingUseCase.java(4 hunks)dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/application/usecase/helper/FileHandlingHelper.java(2 hunks)dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/configuration/SwimDispatcherProperties.java(2 hunks)dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/domain/model/File.java(1 hunks)dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/domain/model/FileEvent.java(1 hunks)dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/domain/model/UseCase.java(1 hunks)dispatch-service/src/main/resources/application-local.yml(2 hunks)dispatch-service/src/test/java/de/muenchen/oss/swim/dispatcher/TestConstants.java(2 hunks)dispatch-service/src/test/java/de/muenchen/oss/swim/dispatcher/adapter/out/s3/S3AdapterTest.java(1 hunks)dispatch-service/src/test/java/de/muenchen/oss/swim/dispatcher/application/usecase/DispatcherUseCaseTest.java(8 hunks)dispatch-service/src/test/java/de/muenchen/oss/swim/dispatcher/application/usecase/ErrorHandlerUseCaseTest.java(3 hunks)dispatch-service/src/test/java/de/muenchen/oss/swim/dispatcher/application/usecase/MarkFileFinishedUseCaseTest.java(3 hunks)dispatch-service/src/test/java/de/muenchen/oss/swim/dispatcher/application/usecase/ProtocolProcessingUseCaseTest.java(5 hunks)dispatch-service/src/test/java/de/muenchen/oss/swim/dispatcher/domain/model/FileTest.java(2 hunks)dispatch-service/src/test/resources/application-test.yml(3 hunks)
💤 Files with no reviewable changes (2)
- dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/adapter/out/streaming/FileEventDTO.java
- dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/adapter/in/streaming/FileEventDTO.java
🧰 Additional context used
🧬 Code Graph Analysis (2)
dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/application/port/out/FileSystemOutPort.java (1)
dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/domain/exception/PresignedUrlException.java (1)
SuppressWarnings(3-12)
dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/adapter/out/s3/S3Adapter.java (1)
dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/domain/exception/PresignedUrlException.java (1)
SuppressWarnings(3-12)
⏰ 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). (6)
- GitHub Check: Analyze Java source files (./dispatch-service)
- GitHub Check: Analyze Java source files (./invoice-service)
- GitHub Check: Analyze Java source files (./matching-service)
- GitHub Check: Analyze Java source files (./dms-service)
- GitHub Check: Analyze Java source files (./dipa-service)
- GitHub Check: Analyze Java source files (./handler-core)
🔇 Additional comments (61)
dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/domain/model/UseCase.java (1)
18-22: LGTM! Tenant field addition is well-implemented.The new tenant field follows established patterns in the class with appropriate validation (
@NotBlank) and clear documentation. This change effectively enables multi-tenant support as intended.dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/adapter/out/streaming/StreamingOutAdapter.java (2)
4-4: LGTM! Clean transition from DTO to domain record.The import update aligns with the broader refactoring to use domain records instead of DTOs.
17-17: LGTM! FileEvent instantiation is correct.The new FileEvent constructor properly maps the existing method parameters, maintaining backward compatibility while using the new domain model.
dispatch-service/src/test/java/de/muenchen/oss/swim/dispatcher/domain/model/FileTest.java (2)
3-4: LGTM! Improved test constants usage.Using imported constants instead of hardcoded values improves test maintainability and consistency.
13-13: LGTM! File constructor updated correctly.The File instantiation properly includes the tenant parameter, aligning with the new multi-tenant domain model.
dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/domain/model/FileEvent.java (1)
1-9: LGTM! Well-designed domain record.The FileEvent record is cleanly implemented with:
- Proper validation annotations for required fields
- Immutable design using Java records
- Clear field naming
- Appropriate optional field handling for metadataPresignedUrl
This design effectively replaces the previous DTO while maintaining type safety.
dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/application/usecase/ProtocolProcessingUseCase.java (5)
49-49: LGTM! Proper tenant context addition.The tenant parameter is correctly extracted from the useCase for file system operations, maintaining consistency with the multi-tenant architecture.
86-86: LGTM! Tenant-aware protocol loading.The loadProtocol method correctly uses the tenant from the file context for tenant-specific protocol operations.
90-91: LGTM! Consistent tenant usage in file operations.Both getMatchingFilesWithTags calls properly use the tenant from the file context, ensuring tenant-aware file discovery in both dispatch and finished folders.
Also applies to: 94-95
110-110: LGTM! Tenant parameter in file reading.The readFile operation correctly includes the tenant parameter for tenant-specific file access.
125-127: LGTM! Complete tenant integration.Both tagFile and moveFile operations properly include the tenant parameter, ensuring all file system operations are tenant-aware throughout the protocol processing workflow.
Also applies to: 130-130
dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/configuration/SwimDispatcherProperties.java (1)
82-82: LGTM: Proper validation enhancement for multi-tenant support.The addition of
@Validannotation ensures recursive validation ofUseCaseinstances, which is essential now that they contain tenant fields requiring validation. This change properly enforces tenant constraints at configuration load time.dispatch-service/src/test/resources/application-test.yml (2)
2-5: LGTM: Clean multi-tenant configuration structure.The new
s3.tenantsconfiguration properly defines tenant-specific S3 endpoints, providing a clean foundation for multi-tenant support.
11-11: LGTM: Consistent tenant assignment across use cases.All use cases consistently reference
test-tenant, ensuring proper test isolation and alignment with the new tenant-aware domain model.Also applies to: 22-22, 32-32
dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/domain/model/File.java (1)
5-5: LGTM: Clean tenant-aware domain model.The addition of
tenantas the first parameter with proper validation enhances the File record to be tenant-aware while maintaining its immutable structure. This establishes tenant as a fundamental part of file identity, which aligns well with the multi-tenant architecture.dispatch-service/src/test/java/de/muenchen/oss/swim/dispatcher/application/usecase/MarkFileFinishedUseCaseTest.java (2)
48-56: LGTM: Comprehensive test adaptation for tenant-aware API.The test properly adapts to the new
FileEvent-based API and consistently includes tenant context in all file system operations. The mock setup and verifications align well with the refactored method signatures.
59-68: LGTM: Exception handling tests properly updated.Both exception test scenarios are correctly updated to use the new
FileEventparameter and updated verification methods, ensuring comprehensive error case coverage.dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/application/port/in/MarkFileFinishedInPort.java (2)
5-7: LGTM! Good consolidation with proper validation.The addition of proper validation imports and the consolidation of parameters into a single
FileEventobject improves the interface design and ensures validation consistency.
17-17: LGTM! Improved method signature with validation.The method signature change from multiple string parameters to a single validated
FileEventparameter is a good architectural improvement that provides better encapsulation and validation.dispatch-service/src/main/resources/application-local.yml (1)
46-51: LGTM! Well-structured multi-tenant configuration.The restructuring of S3 properties under
tenants.localprovides a clean foundation for multi-tenant support.dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/application/usecase/helper/FileHandlingHelper.java (3)
40-40: LGTM! Proper tenant context propagation.The addition of
file.tenant()as the first parameter correctly implements tenant-aware file tagging operations.
49-49: LGTM! Consistent tenant parameter addition.The method signature change to include the
tenantparameter and its usage in subsequent operations properly implements multi-tenant support.Also applies to: 54-54
56-56: LGTM! Tenant-aware file operations.Both
tagFileandmoveFileoperations correctly use the tenant parameter, ensuring proper multi-tenant file handling.Also applies to: 60-60
dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/application/port/in/ErrorHandlerInPort.java (2)
4-4: LGTM! Consistent with architectural refactoring.The import addition supports the consolidation of parameters into the
FileEventobject.
17-17: LGTM! Improved error handling interface.The method signature change from multiple parameters to a single validated
FileEventparameter improves consistency with the event-driven architecture and provides better encapsulation.dispatch-service/src/test/java/de/muenchen/oss/swim/dispatcher/application/usecase/DispatcherUseCaseTest.java (3)
8-8: LGTM! Proper test constant import.The import of
TENANTconstant enables proper tenant-aware testing.
57-57: LGTM! Consistent test data naming.The path update from
"test.json"to"example.json"maintains consistency with other test data naming conventions.
84-86: LGTM! Comprehensive tenant-aware test updates.All file system operation calls have been properly updated to include the tenant parameter as the first argument, ensuring tests validate the multi-tenant functionality correctly.
Also applies to: 101-103, 120-122, 139-143, 154-154, 161-161, 179-179, 197-199
dispatch-service/src/test/java/de/muenchen/oss/swim/dispatcher/adapter/out/s3/S3AdapterTest.java (1)
33-40: LGTM: Well-structured success test.The test correctly validates the tenant-aware file parsing from presigned URLs, asserting the expected tenant, bucket, and path values.
dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/application/usecase/ErrorHandlerUseCase.java (2)
28-29: LGTM: Clean refactoring to use FileEvent.The method signature has been properly updated to use the new
FileEventobject, and the logging correctly extracts the use case and presigned URL from the event.
56-59: LGTM: Tenant-aware file tagging.The
tagFilemethod call correctly includes the tenant parameter as the first argument, aligning with the multi-tenant architecture.dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/application/usecase/MarkFileFinishedUseCase.java (3)
30-32: Add null check for file resolution.Similar to the ErrorHandlerUseCase, the
verifyAndResolvePresignedUrlmethod could return null, but the code proceeds to use thefileobject without checking.final File file = this.fileSystemOutPort.verifyAndResolvePresignedUrl(useCase, event.presignedUrl()); +if (file == null) { + log.warn("File resolution returned null for presigned URL {}", event.presignedUrl()); + throw new PresignedUrlException("Could not resolve file from presigned URL: " + event.presignedUrl()); +}
34-39: Add null check for metadata file resolution.The metadata file resolution also lacks a null check. Additionally, consider the logic flow - if the main file resolution fails, should metadata processing still proceed?
if (Strings.isNotBlank(event.metadataPresignedUrl())) { // verify presigned url and extract metadata File final File metadataFile = this.fileSystemOutPort.verifyAndResolvePresignedUrl(useCase, event.metadataPresignedUrl()); + if (metadataFile == null) { + log.warn("Metadata file resolution returned null for presigned URL {}", event.metadataPresignedUrl()); + throw new PresignedUrlException("Could not resolve metadata file from presigned URL: " + event.metadataPresignedUrl()); + } // finish metadata file fileHandlingHelper.finishFile(useCase, metadataFile.tenant(), metadataFile.bucket(), metadataFile.path()); }
26-28: LGTM: Clean method signature update.The method has been properly refactored to accept a
FileEventparameter, maintaining consistency with the new event-driven architecture.dispatch-service/src/test/java/de/muenchen/oss/swim/dispatcher/TestConstants.java (3)
17-17: LGTM: Added tenant constant for testing.The new
TENANTconstant provides consistent tenant identification across test scenarios.
22-23: LGTM: Updated File constructors with tenant parameter.The File constructors have been correctly updated to include the tenant as the first parameter, maintaining consistency with the new multi-tenant File record structure.
29-30: LGTM: Added FileEvent test constant.The new
TEST_FILE_EVENTconstant provides a standardized FileEvent instance for testing, properly initialized with use case, presigned URL, and null metadata URL.dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/adapter/out/s3/S3Properties.java (4)
28-28: LGTM: Thread-safe client caching.The use of
ConcurrentHashMapfor theCLIENTSmap addresses the concurrency concern raised in previous reviews, ensuring thread-safe access to cached MinioClient instances.
67-77: LGTM: Proper tenant validation and client creation.The
getClientmethod correctly validates tenant existence and creates clients on-demand. The logic condition has been fixed from the previous review to properly throw an exception when the tenant doesn't exist.
49-57: LGTM: Secure ConnectionOptions implementation.The
ConnectionOptionsclass properly excludes sensitive fields (secretKey,accessKey) from thetoStringmethod, preventing accidental logging of credentials.
32-34: LGTM: Well-documented tenant configuration.The
tenantsfield is properly annotated and documented, clearly indicating its relationship with theUseCase#getTenant()method.dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/adapter/in/streaming/StreamingInAdapter.java (2)
23-33: LGTM! Clean refactoring to use FileEvent.The simplified approach of passing the entire
FileEventobject improves code maintainability and aligns well with the multi-tenant architecture changes.
35-42: LGTM! Consistent error handling with FileEvent.The DLQ handler correctly uses the new
FileEventstructure for error processing.dispatch-service/src/test/java/de/muenchen/oss/swim/dispatcher/application/usecase/ErrorHandlerUseCaseTest.java (1)
54-66: Test properly adapted for FileEvent and tenant-aware operations.The test correctly mocks the new
verifyAndResolvePresignedUrlmethod and verifies tenant-aware file tagging.dispatch-service/src/test/java/de/muenchen/oss/swim/dispatcher/application/usecase/ProtocolProcessingUseCaseTest.java (1)
67-92: Excellent tenant-aware test setup.The test correctly incorporates tenant parameters in all file system operations and properly handles the new exception type.
dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/application/usecase/DispatcherUseCase.java (2)
46-71: Proper tenant context propagation in main dispatch loop.The implementation correctly uses
useCase.getTenant()for all file system operations within each use case iteration.
276-283: Verify tenant handling in cross–use–case file copyEnsure the correct tenant is used when copying files between use cases. The current code in
DispatcherUseCase.java(around lines 276–283) always passes the source file’s tenant for both source and destination:fileSystemOutPort.copyFile( file.tenant(), file.bucket(), file.path(), targetUseCase.getBucket(), destPath, true );If target use cases can belong to different tenants, this may cause data to land in the wrong tenant’s namespace. Please:
- Confirm whether your
UseCasemodel exposes agetTenant()(or equivalent) method ontargetUseCase.- Verify that the
FileSystemOutPort.copyFile(…)signature supports a separate destination‐tenant parameter.- If both are true, update the call to:
- fileSystemOutPort.copyFile( - file.tenant(), - file.bucket(), - file.path(), - targetUseCase.getBucket(), - destPath, - true - ); + fileSystemOutPort.copyFile( + file.tenant(), // source tenant + file.bucket(), + file.path(), + targetUseCase.getTenant(), // destination tenant + targetUseCase.getBucket(), + destPath, + true + );
- Otherwise, document that cross–use–case copies require all use cases to share the same tenant.
dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/application/port/out/FileSystemOutPort.java (2)
26-34: Consistent tenant-aware file matching.The method signature properly includes tenant context for scoped file operations.
84-90: Good refactoring of presigned URL verification.The renamed method with
Filereturn type better encapsulates the verification and resolution logic, providing a complete file object with tenant context.dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/adapter/out/s3/S3Adapter.java (12)
82-103: LGTM! Tenant parameter correctly propagated.The addition of the tenant parameter and its propagation through
getObjectsInPath,Fileconstructor, andgetTagsOfFilecalls properly implements multi-tenant support for this method.
118-122: LGTM! Consistent tenant parameter addition.The tenant parameter is correctly added and passed to
getObjectsInPath.
128-145: LGTM! Tenant-aware tagging implementation.The tenant parameter is correctly used to retrieve the tenant-specific S3 client and passed to
getTagsOfFile, ensuring proper tenant isolation for tagging operations.
154-160: LGTM! Tenant-specific file existence check.The tenant parameter is correctly used to retrieve the appropriate S3 client for checking file existence.
179-185: LGTM! Tenant-aware file reading.The tenant parameter is correctly used to retrieve the tenant-specific S3 client for reading files.
194-202: LGTM! Tenant-specific presigned URL generation.The tenant parameter is correctly used to generate presigned URLs from the appropriate tenant's S3 client.
212-228: LGTM! Enhanced presigned URL verification with file resolution.The refactored method correctly:
- Verifies the presigned URL is accessible
- Resolves and returns the File object based on the UseCase
- Throws clear exceptions for failure cases
This aligns with the requirement to resolve tenant via UseCase rather than presigned URL.
233-248: LGTM! Tenant-aware file moving.The tenant parameter is correctly used for both the copy and remove operations, ensuring proper tenant isolation when moving files.
259-276: LGTM! Tenant-specific file copying.The tenant parameter is correctly used to retrieve the appropriate S3 client for copying files.
294-303: LGTM! Tenant-aware object listing.The protected helper method correctly uses the tenant parameter to retrieve objects from the appropriate tenant's S3 storage.
325-331: LGTM! Tenant-specific tag retrieval.The protected helper method correctly uses the tenant parameter to retrieve tags from the appropriate tenant's S3 storage.
446-454: LGTM! Tenant-aware protocol loading.The tenant parameter is correctly passed to
readFilefor loading protocol data from the appropriate tenant's storage.
| /** | ||
| * Resolves a File from a presigned URL. | ||
| * Needs to be formated as: {@code <tenant url>/<bucket>/<path>} | ||
| * | ||
| * @param presignedUrl The presigned URL to resolve. | ||
| * @return The resolved File. | ||
| * @throws PresignedUrlException If the presigned URL can't be parsed. | ||
| */ | ||
| protected File fileFromPresignedUrl(@NotNull final UseCase useCase, @NotBlank final String presignedUrl) throws PresignedUrlException { | ||
| // check input has content | ||
| if (Strings.isBlank(presignedUrl)) { | ||
| throw new PresignedUrlException("Empty presigned url can't be parsed"); | ||
| } | ||
| // parse presigned url | ||
| final URI uri; | ||
| try { | ||
| uri = new URI(presignedUrl); | ||
| } catch (final URISyntaxException e) { | ||
| throw new PresignedUrlException("Presigned url could not be parsed", e); | ||
| } | ||
| // create File object from presigned url | ||
| final String tenantUrl = String.format("%s://%s%s", uri.getScheme(), uri.getHost(), uri.getPort() == -1 ? "" : ":" + uri.getPort()); | ||
| final String uriPath = uri.getPath().replaceFirst("^/", ""); | ||
| final int slashIndex = uriPath.indexOf('/'); | ||
| final String bucket = uriPath.substring(0, slashIndex); | ||
| final String filePath = uriPath.substring(slashIndex + 1); | ||
| // compare with UseCase | ||
| this.verifyPresignedUrlForUseCase(useCase, tenantUrl, bucket, filePath); | ||
| return new File(useCase.getTenant(), useCase.getBucket(), filePath, null); | ||
| } |
There was a problem hiding this comment.
Potential StringIndexOutOfBoundsException in URL parsing.
The method correctly implements resolving tenant from UseCase rather than the presigned URL. However, there's a potential issue at line 409:
final int slashIndex = uriPath.indexOf('/');
final String bucket = uriPath.substring(0, slashIndex);If slashIndex is -1 (no slash found), substring(0, -1) will throw a StringIndexOutOfBoundsException.
Add validation to handle URLs without a file path:
final String uriPath = uri.getPath().replaceFirst("^/", "");
final int slashIndex = uriPath.indexOf('/');
+if (slashIndex == -1) {
+ throw new PresignedUrlException("Invalid presigned URL format: missing file path");
+}
final String bucket = uriPath.substring(0, slashIndex);
final String filePath = uriPath.substring(slashIndex + 1);🤖 Prompt for AI Agents
In
dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/adapter/out/s3/S3Adapter.java
around lines 385 to 414, the code extracts the bucket and file path from the URI
path by finding the first slash. If no slash is found, substring(0, -1) causes a
StringIndexOutOfBoundsException. To fix this, add a check after getting
slashIndex to verify it is not -1. If it is -1, throw a PresignedUrlException
indicating the presigned URL format is invalid or missing the file path. This
validation prevents the exception and ensures only correctly formatted URLs are
processed.
| /** | ||
| * Verify the given presigned URL is valid for a UseCase. | ||
| * | ||
| * @param useCase The UseCase the presigned URL should be for. | ||
| * @param tenantUrl The resolved tenant URL from the presigned URL. | ||
| * @param bucket The resolved bucket from the presigned URL. | ||
| * @param filePath The resolved filePath URL from the presigned URL. | ||
| * @throws PresignedUrlException If the presigned URL is not valid for the UseCase. | ||
| */ | ||
| protected void verifyPresignedUrlForUseCase(final UseCase useCase, final String tenantUrl, final String bucket, final String filePath) | ||
| throws PresignedUrlException { | ||
| // tenant | ||
| final S3Properties.ConnectionOptions tenantOptions = this.s3Properties.getTenants().get(useCase.getTenant()); | ||
| if (!tenantOptions.getUrl().startsWith(tenantUrl) && !tenantUrl.startsWith(tenantOptions.getUrl())) { | ||
| throw new PresignedUrlException(String.format("Presigned URL: UseCase %s tenant URL %s doesn't match presigned URL %s", | ||
| useCase.getName(), tenantOptions.getUrl(), tenantUrl)); | ||
| } | ||
| // bucket | ||
| if (!useCase.getBucket().equals(bucket)) { | ||
| throw new PresignedUrlException(String.format("Presigned URL: Bucket %s from UseCase %s doesn't match bucket from presigned URL %s", | ||
| useCase.getBucket(), useCase.getName(), bucket)); | ||
| } | ||
| // path | ||
| if (!filePath.startsWith(useCase.getPath())) { | ||
| throw new PresignedUrlException( | ||
| String.format("Presigned URL: Filepath %s isn't in UseCase %s path %s", filePath, useCase.getName(), useCase.getPath())); | ||
| } | ||
| } |
There was a problem hiding this comment.
Improve URL validation to prevent potential security issues.
The bidirectional startsWith check on line 429 could be too permissive and might allow unintended URL matches:
if (!tenantOptions.getUrl().startsWith(tenantUrl) && !tenantUrl.startsWith(tenantOptions.getUrl()))This could incorrectly validate URLs like http://example.com.evil.com against http://example.com.
Consider normalizing both URLs and using exact host matching:
-if (!tenantOptions.getUrl().startsWith(tenantUrl) && !tenantUrl.startsWith(tenantOptions.getUrl())) {
+// Normalize URLs for comparison (remove trailing slashes, compare hosts)
+URI expectedUri = new URI(tenantOptions.getUrl());
+URI actualUri = new URI(tenantUrl);
+if (!expectedUri.getHost().equals(actualUri.getHost()) ||
+ expectedUri.getPort() != actualUri.getPort() ||
+ !expectedUri.getScheme().equals(actualUri.getScheme())) {
throw new PresignedUrlException(String.format("Presigned URL: UseCase %s tenant URL %s doesn't match presigned URL %s",
useCase.getName(), tenantOptions.getUrl(), tenantUrl));
}🤖 Prompt for AI Agents
In
dispatch-service/src/main/java/de/muenchen/oss/swim/dispatcher/adapter/out/s3/S3Adapter.java
between lines 416 and 443, the tenant URL validation uses a bidirectional
startsWith check that can incorrectly validate malicious URLs. To fix this,
parse both tenantOptions.getUrl() and tenantUrl into URL objects, then compare
their hosts exactly instead of using startsWith. This ensures only exact host
matches are accepted, preventing security issues from permissive URL matching.
| fallback-mail: swim@example.com | ||
| use-cases: | ||
| - name: test-meta | ||
| tenant: local |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify tenant assignment consistency.
Some use cases have tenant: local specified while others (like test-inbox-incoming and test-dipa) don't. Ensure this is intentional and that use cases without tenant assignment are handled correctly.
Also applies to: 78-78, 86-86
🏁 Script executed:
#!/bin/bash
# Description: Check if use cases without tenant assignment are handled properly in the codebase
# Expected: Find how use cases without tenant are processed
# Search for how use cases without tenant field are handled
ast-grep --pattern 'class $_ {
$$$
tenant($$$) {
$$$
}
$$$
}'
# Look for default tenant handling logic
rg -A 5 "tenant.*null|default.*tenant" --type java
# Check UseCase class for tenant handling
rg -A 10 "class UseCase" --type javaLength of output: 13227
Ensure Tenant Is Defined for Every Use Case
I couldn’t find any default‐tenant fallback in the code (the UseCase model’s tenant field is @NotBlank and there’s no “if null → use default” logic), so any use case entry without a tenant: will fail validation or behave unexpectedly at startup. Please add tenant: local (or another appropriate value) to all use cases in application-local.yml, for example:
• Line 57 – test-inbox-incoming
• Line 78 – test-dipa
• Line 86 – [next use case]
Alternatively, implement a default-tenant mechanism in the code if leaving these blank was intentional.
🤖 Prompt for AI Agents
In dispatch-service/src/main/resources/application-local.yml at line 57 and
similarly at lines 78 and 86, the use case entries lack a tenant field, which is
mandatory and currently has no default fallback in the code. To fix this, add
tenant: local (or another appropriate tenant value) to each use case entry to
ensure they pass validation and behave correctly at startup.
| @Test | ||
| void fileFromPresignedUrl_Exception() { | ||
| final UseCase useCase = swimDispatcherProperties.getUseCases().get(1); | ||
| assertThrows(PresignedUrlException.class, () -> s3Adapter.fileFromPresignedUrl(useCase, TEST_PRESIGNED_URL)); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify test configuration setup for different use cases.
The test uses getUseCases().get(1) to trigger an exception, implying the second use case should have different tenant configuration that causes fileFromPresignedUrl to fail. Ensure your test configuration properly sets up this scenario.
🏁 Script executed:
#!/bin/bash
# Description: Verify test configuration has multiple use cases with different tenant setups
# Search for test configuration files that define use cases
fd "application.*test.*\.ya?ml$" --exec cat {} \; | grep -A 20 -B 5 "use.*cases\|useCases"Length of output: 2212
🏁 Script executed:
#!/bin/bash
# Locate application-*.yml/.yaml in test resources for dispatch-service
echo "Test property files in dispatch-service/src/test/resources:"
find dispatch-service/src/test/resources -maxdepth 1 -type f \( -name "application*.yml" -o -name "application*.yaml" \)
# Print contents of each found file for inspection
for file in $(find dispatch-service/src/test/resources -maxdepth 1 -type f \( -name "application*.yml" -o -name "application*.yaml" \)); do
echo -e "\n==== $file ===="
sed -n '1,200p' "$file"
doneLength of output: 1679
Ensure a distinct tenant is mis-configured to trigger the exception
The test at S3AdapterTest#fileFromPresignedUrl_Exception uses
swimDispatcherProperties.getUseCases().get(1)but both use-cases in application-test.yml reference the same, correctly configured tenant (test-tenant). As a result, calling fileFromPresignedUrl(...) will not throw.
To fix:
- In dispatch-service/src/test/resources/application-test.yml, under
swim.s3.tenants, add a second tenant entry (e.g.invalid-tenant) with no or an invalidurl. - Update the second use-case to reference this new tenant:
swim: s3: tenants: test-tenant: url: https://s3.muenchen.de invalid-tenant: # ← missing or malformed URL # … use-cases: - name: test-meta tenant: test-tenant … - name: test-meta-dest tenant: invalid-tenant # ← now points to invalid config …
- This ensures that
fileFromPresignedUrl()is invoked with a tenant lacking a valid endpoint and will throw the expected PresignedUrlException.
🤖 Prompt for AI Agents
In
dispatch-service/src/test/java/de/muenchen/oss/swim/dispatcher/adapter/out/s3/S3AdapterTest.java
around lines 42 to 46, the test uses a use-case referencing a correctly
configured tenant, so the expected PresignedUrlException is not thrown. To fix
this, modify dispatch-service/src/test/resources/application-test.yml by adding
a second tenant under swim.s3.tenants with an invalid or missing URL (e.g.,
invalid-tenant), then update the second use-case to reference this invalid
tenant. This ensures that calling fileFromPresignedUrl() with this use-case
triggers the exception as intended.
1 participant
Description
Dispatch implement s3 multi tenant support.
Reference
Issues closes #143
Summary by CodeRabbit
New Features
FileEventdata structure for improved file event processing.Refactor
These improvements enhance the overall robustness of file management and provide more scalable, tenant-aware operations.