refact(integrations/s3)!: rm service and replace minio with awssdk (BREAKING) #742
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
📝 WalkthroughWalkthroughMigrates S3 integration from Minio to AWS SDK v2, refactors public APIs to FileReference/FileMetadata/PresignedUrl, removes REST client/server & legacy client modules, adds an example Spring Boot app and tests (including Testcontainers MinIO), and updates docs and build modules accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant UseCase
participant FileOpsPort
participant S3Adapter
participant S3Client
participant S3Presigner
UseCase->>FileOpsPort: saveFile(FileReference, InputStream, length)
FileOpsPort->>S3Adapter: saveFile(fileReference, InputStream, length)
alt presign required
S3Adapter->>S3Presigner: presign PutObjectRequest
S3Presigner-->>S3Adapter: PresignedRequest
end
S3Adapter->>S3Client: putObject(request, RequestBody.fromInputStream(...))
S3Client-->>S3Adapter: PutObjectResponse / SdkException
S3Adapter-->>FileOpsPort: void / throws S3Exception
FileOpsPort-->>UseCase: completed / exception
sequenceDiagram
participant UseCase
participant FileOpsPort
participant S3Adapter
participant S3Presigner
UseCase->>FileOpsPort: getPresignedUrl(FileReference, Action, Duration)
FileOpsPort->>S3Adapter: getPresignedUrl(fileReference, action, lifetime)
alt action == GET
S3Adapter->>S3Presigner: presign GetObjectRequest
else action == PUT/DELETE/HEAD
S3Adapter->>S3Presigner: presign corresponding request
end
S3Presigner-->>S3Adapter: PresignedRequest
S3Adapter-->>FileOpsPort: PresignedUrl(URL, path, Action)
FileOpsPort-->>UseCase: PresignedUrl
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
github-actions
bot
added
Component: Integrations
simonhir
changed the title
4 tasks
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
...tion-core/src/test/java/de/muenchen/refarch/integration/s3/adapter/out/s3/S3AdapterTest.java
Fixed
Show fixed
Hide fixed
...t/java/de/muenchen/refarch/integration/s3/application/usecase/FileOperationsUseCaseTest.java
Fixed
Show fixed
Hide fixed
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In
`@refarch-integrations/refarch-s3-integration/refarch-s3-integration-core/src/main/java/de/muenchen/refarch/integration/s3/adapter/out/s3/S3Adapter.java`:
- Around line 79-89: The thrown S3Exception in S3Adapter.getFileMetadata
currently uses a misleading message "Error while downloading %s"; update the
catch block that throws new S3Exception(...) to use a correct message like
"Error while retrieving metadata for %s" (including the fileReference) so it
accurately reflects the HeadObject/metadata operation performed; keep the same
exception chaining (pass the caught SdkException as the cause).
In
`@refarch-integrations/refarch-s3-integration/refarch-s3-integration-core/src/main/java/de/muenchen/refarch/integration/s3/application/usecase/FolderOperationsUseCase.java`:
- Around line 19-27: The method getFilesInFolder currently calls
s3OutPort.getFilesWithPrefix(bucket, pathToFolder, 1000, null) with a hardcoded
maxKeys and no pagination, which can silently truncate results; update
getFilesInFolder to iterate/paginate using the continuation token (or
equivalent) returned by getFilesWithPrefix until isTruncated is false (or fetch
all pages) and aggregate results before applying ensureTrailingSlash and
isImmediateChild filtering for non-recursive calls, or alternatively expose
maxKeys/continuation parameters from getFilesInFolder so callers can control
pagination; reference s3OutPort.getFilesWithPrefix, getFilesInFolder,
ensureTrailingSlash and isImmediateChild when making the change.
In
`@refarch-integrations/refarch-s3-integration/refarch-s3-integration-core/src/test/java/de/muenchen/refarch/integration/s3/adapter/out/s3/S3AdapterTest.java`:
- Around line 209-215: The test S3AdapterTest.testGetFileContent currently mocks
s3Client.getObject to return null which violates the port contract InputStream
getFileContent(...) (no `@Nullable`); instead update the test to either (A) mock
s3Client.getObject(...) to return a non-null InputStream (e.g., a
ByteArrayInputStream) and assert adapter.getFileContent(ref) returns that
stream, or (B) assert that s3Client.getObject(...) returning null is treated as
an error by the adapter by mocking it to throw an exception (similar to handling
NoSuchKeyException in fileExists) and asserting the adapter propagates/throws
the expected exception; locate the behavior in S3AdapterTest.testGetFileContent,
the adapter.getFileContent(...) call and the s3Client.getObject(...) mock to
implement the chosen fix.
In
`@refarch-integrations/refarch-s3-integration/refarch-s3-integration-example/src/main/java/de/muenchen/refarch/integration/s3/example/S3ExampleApplication.java`:
- Around line 22-26: The ApplicationReadyEvent handler sendTestMail currently
calls SpringApplication.exit(context) which only closes the Spring context;
update sendTestMail to call System.exit(SpringApplication.exit(context)) so the
JVM terminates after the exampleService.testS3() run; locate the sendTestMail
method in S3ExampleApplication and replace the standalone
SpringApplication.exit(context) call with
System.exit(SpringApplication.exit(context)).
In
`@refarch-integrations/refarch-s3-integration/refarch-s3-integration-example/src/main/java/de/muenchen/refarch/integration/s3/example/S3ExampleService.java`:
- Line 33: Fix the typo in the log message inside S3ExampleService (where
log.info is called) by changing "Testing S3 inetgration" to "Testing S3
integration" so the log reads correctly; locate the log.info invocation in the
S3ExampleService class and update the string literal accordingly.
- Around line 3-54: S3ExampleService.testS3 uses JUnit assertions and fixed
constants (BUCKET, FOLDER, FILE_NAME) which pull test deps into production and
risk clobbering real objects; replace assertEquals/assertTrue calls with
explicit runtime checks that throw a checked exception or return a meaningful
result/log on failure (referencing method testS3 and
FileOperationsInPort/FolderOperationsInPort interactions), and make the
bucket/folder/file names non-fixed by injecting BUCKET via configuration and
prefixing FOLDER or FILE_NAME with a per-run unique id (e.g., UUID or timestamp)
to isolate test artifacts.
In
`@refarch-integrations/refarch-s3-integration/refarch-s3-integration-example/src/main/resources/application-local.yaml`:
- Around line 16-20: Replace the hardcoded S3 credentials under the refarch.s3
keys: remove literal values for access-key and secret-key and use
environment-variable placeholders (e.g., refarch.s3.access-key and
refarch.s3.secret-key should read from env vars like S3_ACCESS_KEY/S3_SECRET or
reference a properties placeholder) while retaining the local url default
(refarch.s3.url). Alternatively move real credentials to a separate .env or
.example file and ensure the checked-in YAML only contains placeholders or
comments guiding developers to set S3_ACCESS_KEY and S3_SECRET locally.
In
`@refarch-integrations/refarch-s3-integration/refarch-s3-integration-starter/src/main/java/de/muenchen/refarch/integration/s3/configuration/S3IntegrationAutoConfiguration.java`:
- Around line 97-105: The fileOperationsInPort bean currently accepts a concrete
S3Adapter while folderOperationsInPort accepts the S3OutPort interface, causing
inconsistent injection; change fileOperationsInPort to accept S3OutPort (same
interface used by folderOperationsInPort) and pass that interface instance into
the FileOperationsUseCase constructor (update the parameter name from s3Adapter
to s3OutPort if needed) so both beans rely on S3OutPort rather than a concrete
S3Adapter.
🧹 Nitpick comments (8)
refarch-integrations/refarch-s3-integration/refarch-s3-integration-core/pom.xml (1)
33-53: Appropriate HTTP client configuration for synchronous S3 operations.Excluding
netty-nio-clientandapache-clientand usingurl-connection-clientis a valid lightweight approach for synchronous S3 operations. This reduces transitive dependencies significantly.Consider using AWS SDK BOM for version management.
For easier maintenance and consistency across AWS SDK modules, consider importing the AWS SDK BOM instead of managing versions explicitly:
♻️ Optional: Use AWS SDK BOM
Add to
<dependencyManagement>in a parent POM:<dependencyManagement> <dependencies> <dependency> <groupId>software.amazon.awssdk</groupId> <artifactId>bom</artifactId> <version>${awssdk.s3.version}</version> <type>pom</type> <scope>import</scope> </dependency> </dependencies> </dependencyManagement>Then remove explicit versions from individual AWS SDK dependencies.
docs/support/migration.md (1)
38-51: Add a pointer to the new S3 example module for migration validation.The migration guide should mention the example service introduced for runtime verification (issue
#664), so users have a quick smoke-test path after moving to v3.refarch-integrations/refarch-s3-integration/refarch-s3-integration-core/src/main/java/de/muenchen/refarch/integration/s3/domain/model/PresignedUrl.java (1)
7-10: Consider usingURIinstead ofURL.
java.net.URLhas known issues: itsequals()andhashCode()methods perform DNS lookups, which can cause unexpected blocking behavior and inconsistent results.java.net.URIis the preferred type for representing URLs as data since it provides purely syntactic comparison.♻️ Suggested change
-import java.net.URL; +import java.net.URI; public record PresignedUrl( - `@NotNull` URL url, + `@NotNull` URI url, `@NotBlank` String path, `@NotNull` Action action) {refarch-integrations/refarch-s3-integration/refarch-s3-integration-core/src/main/java/de/muenchen/refarch/integration/s3/adapter/out/s3/S3Mapper.java (1)
7-15: Consider null safety for AWS SDK response fields.AWS SDK response objects may have null fields in edge cases (e.g.,
eTagorlastModifiedcould be null for certain object types or incomplete responses). SinceFileMetadatauses@NotBlankand@NotNullconstraints, null values would fail validation downstream.Consider defensive null checks or documenting the assumption that these methods should only be called with complete responses.
Additionally, consider making this class a Spring
@Componentif it will be injected, or making the methodsstaticif it's a stateless utility.refarch-integrations/refarch-s3-integration/refarch-s3-integration-starter/src/main/java/de/muenchen/refarch/integration/s3/properties/S3IntegrationProperties.java (1)
49-53: Minor Javadoc inconsistency.The Javadoc states "Must not be {
@codenull}" butinitialConnectionTestis a primitiveboolean, which cannot be null. Consider removing that phrase from the documentation.📝 Suggested documentation fix
/** * Whether to perform a connectivity check to the configured S3 endpoint during application startup. - * Defaults to {`@code` true}. Must not be {`@code` null}. + * Defaults to {`@code` true}. */ private boolean initialConnectionTest = true;refarch-integrations/refarch-s3-integration/refarch-s3-integration-starter/src/main/java/de/muenchen/refarch/integration/s3/configuration/S3IntegrationAutoConfiguration.java (1)
35-81: Consider extracting duplicated credentials provider.The
StaticCredentialsProvidercreation is duplicated in boths3Client()ands3Presigner(). Extracting to a private method would improve maintainability.Proposed refactor
+private StaticCredentialsProvider credentialsProvider() { + return StaticCredentialsProvider.create( + AwsBasicCredentials.create( + s3IntegrationProperties.getAccessKey(), + s3IntegrationProperties.getSecretKey())); +} + `@Bean` `@ConditionalOnMissingBean` public S3Client s3Client() { final S3Configuration cfg = S3Configuration.builder() .pathStyleAccessEnabled(s3IntegrationProperties.isPathStyleAccessEnabled()) .build(); - final StaticCredentialsProvider creds = StaticCredentialsProvider.create( - AwsBasicCredentials.create( - s3IntegrationProperties.getAccessKey(), - s3IntegrationProperties.getSecretKey())); - final S3Client client = S3Client.builder() .endpointOverride(URI.create(s3IntegrationProperties.getUrl())) .region(Region.of(s3IntegrationProperties.getRegion())) - .credentialsProvider(creds) + .credentialsProvider(credentialsProvider()) .serviceConfiguration(cfg) .build(); // ... rest of method }refarch-integrations/refarch-s3-integration/refarch-s3-integration-core/src/main/java/de/muenchen/refarch/integration/s3/adapter/out/s3/S3Adapter.java (2)
146-155: Document caller's responsibility to close the returned InputStream.The returned
InputStream(actuallyResponseInputStream) wraps an HTTP connection that must be closed by the caller to avoid resource leaks. This should be documented in the method or interface Javadoc.Suggested Javadoc
/** * Returns the content of the file as an InputStream. * <p><strong>Important:</strong> The caller is responsible for closing the returned * InputStream to release the underlying HTTP connection.</p> * * `@param` fileReference the file to retrieve * `@return` an InputStream containing the file content * `@throws` S3Exception if the file cannot be retrieved */
168-185: Consider using ListObjectsV2 API.The code uses
ListObjectsRequest(v1 API). AWS officially recommendsListObjectsV2Requestfor new development because it provides a safer pagination model using opaqueContinuationTokeninstead of lexicographic markers (which have edge cases). ListObjectsV2 also offers additional features likeFetchOwner,KeyCount, andStartAfterparameters. The current implementation using v1 works correctly but should be migrated to v2 for better alignment with AWS best practices.
...egration-core/src/main/java/de/muenchen/refarch/integration/s3/adapter/out/s3/S3Adapter.java
Show resolved
Hide resolved
...ain/java/de/muenchen/refarch/integration/s3/application/usecase/FolderOperationsUseCase.java
Show resolved
Hide resolved
...tion-core/src/test/java/de/muenchen/refarch/integration/s3/adapter/out/s3/S3AdapterTest.java
Show resolved
Hide resolved
...n-example/src/main/java/de/muenchen/refarch/integration/s3/example/S3ExampleApplication.java
Show resolved
Hide resolved
...ation-example/src/main/java/de/muenchen/refarch/integration/s3/example/S3ExampleService.java
Outdated
Show resolved
Hide resolved
...ation-example/src/main/java/de/muenchen/refarch/integration/s3/example/S3ExampleService.java
Outdated
Show resolved
Hide resolved
...arch-s3-integration/refarch-s3-integration-example/src/main/resources/application-local.yaml
Show resolved
Hide resolved
...in/java/de/muenchen/refarch/integration/s3/configuration/S3IntegrationAutoConfiguration.java
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
`@refarch-integrations/refarch-s3-integration/refarch-s3-integration-core/src/main/java/de/muenchen/refarch/integration/s3/application/port/in/FolderOperationsInPort.java`:
- Around line 37-45: Add Bean Validation bounds to the interface method
parameter and enable validation on the implementation: annotate the maxKeys
parameter on FolderOperationsInPort.getFilesInFolder with the appropriate
validation annotations (e.g., `@Min`(1) and `@Max`(1000) from
javax/jakarta.validation.constraints) so the 1–1000 range is enforced on the
interface, and annotate the implementation class (FolderOperationsUseCase or the
concrete class implementing FolderOperationsInPort) with `@Validated` so Spring
will apply the interface-level constraints at runtime; also add the necessary
imports for the annotations.
In
`@refarch-integrations/refarch-s3-integration/refarch-s3-integration-example/src/main/java/de/muenchen/refarch/integration/s3/example/S3ExampleService.java`:
- Around line 39-41: The validation currently throws incorrectly because it uses
"!files.isEmpty() || !files.getFirst().path().equals(filePath)" and can
dereference getFirst() on an empty list; change the condition to
"files.isEmpty() || !files.getFirst().path().equals(filePath)" so it throws when
the list is empty or the first file's path doesn't match filePath, keeping the
short-circuit order; update the check where files is assigned from
folderOperationsInPort.getFilesInFolder(BUCKET, FOLDER, false).
In
`@refarch-integrations/refarch-s3-integration/refarch-s3-integration-starter/src/main/java/de/muenchen/refarch/integration/s3/configuration/S3IntegrationAutoConfiguration.java`:
- Around line 54-62: The startup S3 connectivity check in
S3IntegrationAutoConfiguration currently calls client.listBuckets() which fails
under least‑privilege IAM; replace it by performing a bucket‑scoped check using
the configured bucket from s3IntegrationProperties (e.g. call HeadBucket or
ListObjectsV2 against s3IntegrationProperties.getBucketName()) instead of
client.listBuckets(), log success with s3IntegrationProperties.getUrl() and the
bucket name, and on exception throw the same IllegalStateException wrapping the
caught exception to preserve behavior; ensure you locate and update the block
that references s3IntegrationProperties.isInitialConnectionTest() and
client.listBuckets().
♻️ Duplicate comments (3)
refarch-integrations/refarch-s3-integration/refarch-s3-integration-example/src/main/java/de/muenchen/refarch/integration/s3/example/S3ExampleService.java (1)
21-24: Avoid fixed bucket/folder names in example runs.Hard-coded bucket/folder values can clobber real objects and make the list/delete checks flaky when the folder already contains data. Use a per-run unique prefix and make the bucket configurable.
♻️ Possible direction (unique per-run prefix)
+import java.util.UUID; @@ - private static final String FOLDER = "test"; + private static final String FOLDER_PREFIX = "test"; @@ - final String filePath = "%s/%s".formatted(FOLDER, FILE_NAME); + final String runId = UUID.randomUUID().toString(); + final String folder = "%s/%s".formatted(FOLDER_PREFIX, runId); + final String filePath = "%s/%s".formatted(folder, FILE_NAME); final FileReference fileReference = new FileReference(BUCKET, filePath); @@ - final List<FileMetadata> files = folderOperationsInPort.getFilesInFolder(BUCKET, FOLDER, false); + final List<FileMetadata> files = folderOperationsInPort.getFilesInFolder(BUCKET, folder, false); @@ - final List<FileMetadata> files2 = folderOperationsInPort.getFilesInFolder(BUCKET, FOLDER, false); + final List<FileMetadata> files2 = folderOperationsInPort.getFilesInFolder(BUCKET, folder, false); if (!files2.isEmpty()) { - throw new IllegalStateException("S3 folder not empty after delete: " + FOLDER); + throw new IllegalStateException("S3 folder not empty after delete: " + folder); }Also applies to: 32-55
refarch-integrations/refarch-s3-integration/refarch-s3-integration-core/src/main/java/de/muenchen/refarch/integration/s3/adapter/out/s3/S3Adapter.java (1)
168-184: Expose pagination info (or switch to ListObjectsV2) to avoid silent truncation.The method drops
isTruncated/nextMarker, so callers can’t reliably detect more pages. Large prefixes may be silently truncated. Consider returning a page object (items + next marker/isTruncated) or switching to ListObjectsV2 with continuation tokens exposed in the API.In AWS SDK v2, does ListObjectsResponse expose isTruncated/nextMarker, and is ListObjectsV2 recommended over ListObjects for pagination?refarch-integrations/refarch-s3-integration/refarch-s3-integration-core/src/main/java/de/muenchen/refarch/integration/s3/application/usecase/FolderOperationsUseCase.java (1)
18-34: Single-page listing can truncate results (default maxKeys=1000).For prefixes with more than
maxKeysobjects, results are incomplete and non-recursive filtering can miss immediate children. Consider iterating pages via marker/continuation tokens or returning pagination info to the caller.How does S3 list pagination signal truncation and next-page tokens for ListObjects/ListObjectsV2?
🧹 Nitpick comments (2)
refarch-integrations/refarch-s3-integration/refarch-s3-integration-starter/src/main/java/de/muenchen/refarch/integration/s3/configuration/S3IntegrationAutoConfiguration.java (2)
95-99: Rename parameter to match its type for clarity.
The parameter name suggests a concrete adapter even though the type isS3OutPort.✏️ Small naming tweak
- public FileOperationsInPort fileOperationsInPort(final S3OutPort s3Adapter) { - return new FileOperationsUseCase(s3Adapter); + public FileOperationsInPort fileOperationsInPort(final S3OutPort s3OutPort) { + return new FileOperationsUseCase(s3OutPort); }
35-80: Extract credentials provider and endpoint as helper methods to avoid duplication.
Credentials provider, endpoint, and region configuration are duplicated acrosss3Client()ands3Presigner()bean methods. Creating two private helper methods—credentialsProvider()andendpoint()—reduces maintenance risk and ensures consistency.♻️ Suggested refactor
+ private StaticCredentialsProvider credentialsProvider() { + return StaticCredentialsProvider.create( + AwsBasicCredentials.create( + s3IntegrationProperties.getAccessKey(), + s3IntegrationProperties.getSecretKey())); + } + + private URI endpoint() { + return URI.create(s3IntegrationProperties.getUrl()); + } + `@Bean` `@ConditionalOnMissingBean` public S3Client s3Client() { final S3Configuration cfg = S3Configuration.builder() .pathStyleAccessEnabled(s3IntegrationProperties.isPathStyleAccessEnabled()) .build(); - - final StaticCredentialsProvider creds = StaticCredentialsProvider.create( - AwsBasicCredentials.create( - s3IntegrationProperties.getAccessKey(), - s3IntegrationProperties.getSecretKey())); final S3Client client = S3Client.builder() - .endpointOverride(URI.create(s3IntegrationProperties.getUrl())) + .endpointOverride(endpoint()) .region(Region.of(s3IntegrationProperties.getRegion())) - .credentialsProvider(creds) + .credentialsProvider(credentialsProvider()) .serviceConfiguration(cfg) .build(); @@ `@Bean` `@ConditionalOnMissingBean` public S3Presigner s3Presigner() { - final StaticCredentialsProvider creds = StaticCredentialsProvider.create( - AwsBasicCredentials.create( - s3IntegrationProperties.getAccessKey(), - s3IntegrationProperties.getSecretKey())); - return S3Presigner.builder() - .endpointOverride(URI.create(s3IntegrationProperties.getUrl())) + .endpointOverride(endpoint()) .region(Region.of(s3IntegrationProperties.getRegion())) - .credentialsProvider(creds) + .credentialsProvider(credentialsProvider()) .build(); }
...main/java/de/muenchen/refarch/integration/s3/application/port/in/FolderOperationsInPort.java
Show resolved
Hide resolved
...ation-example/src/main/java/de/muenchen/refarch/integration/s3/example/S3ExampleService.java
Show resolved
Hide resolved
...in/java/de/muenchen/refarch/integration/s3/configuration/S3IntegrationAutoConfiguration.java
Show resolved
Hide resolved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull Request
Changes
Reference
Issue: closes #721, closes #664, closes #693
Checklist
Note: If some checklist items are not relevant for your PR, just remove them.
General
Code
console.log), see code quality toolingSummary by CodeRabbit
New Features
Documentation
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.