ACC-3102 Switch from minio-java-sdk for s3 to aws-sdk v2 by michyspiri · Pull Request #335 · xenit-eu/contentgrid-appserver

michyspiri · 2026-07-31T08:47:45Z

No description provided.

AndreasVAmexio · 2026-07-31T11:44:28Z

+    S3Client s3Client(S3Properties properties) {
+        // connection-pool-size has no equivalent in the Apache http client: it was the maximum number of *idle*
+        // connections OkHttp kept around (0 by default: no connection reuse, cfr. ACC-2697). The same net effect
+        // is preserved by evicting idle connections after the keep-alive period.


Having a short keepAlive isn't the same as not pooling connections. Also, the git commit on which the comment is based mentions the wrong ticket number, the connection re-use ticket was https://xenitsupport.jira.com/browse/ACC-2696.
Can you try setting the HttpHeader Connection to close? That should make the client not re-use the connections.

vierbergenlars · 2026-08-03T08:03:12Z

+import software.amazon.awssdk.services.s3.S3Client;
+
+/**
+ * Creates {@link S3Client} instances configured the way the previously used minio client was, so that the


Referencing the "previously used minio client" everywhere is not useful. We don't care about it being configured exactly the same, as long as it works the same

vierbergenlars · 2026-08-03T08:03:54Z

+ *     <li>Path-style access is used unless the endpoint is an AWS endpoint (minio derived this the same way).
+ *     The AWS SDK would otherwise default to virtual-host style, which most S3-compatible stores do not
+ *     support.</li>
+ *     <li>Without an access key + secret key pair, requests are sent unsigned (anonymous), like minio did.</li>


We have never used anonymous access

vierbergenlars · 2026-08-03T08:04:31Z

+ * Creates {@link S3Client} instances configured the way the previously used minio client was, so that the
+ * existing configuration properties keep their exact semantics after the switch to the AWS SDK:
+ * <ul>
+ *     <li>An endpoint without scheme gets {@code https://} prepended, and a path in the endpoint is


The endpoint is always supplied with a protocol, so this prefixing is useless

vierbergenlars · 2026-08-03T08:05:54Z

+ *     rejected.</li>
+ *     <li>Path-style access is used unless the endpoint is an AWS endpoint (minio derived this the same way).
+ *     The AWS SDK would otherwise default to virtual-host style, which most S3-compatible stores do not
+ *     support.</li>


Minio did not make path-style access configurable.

In the past (in the spring-content based system), we had a configuration parameter to enable/disable path-style access. We can make that explicitly configurable again. Because maybe some systems other than AWS don't support path-style access either.

vierbergenlars · 2026-08-03T08:07:02Z

+ *     The AWS SDK would otherwise default to virtual-host style, which most S3-compatible stores do not
+ *     support.</li>
+ *     <li>Without an access key + secret key pair, requests are sent unsigned (anonymous), like minio did.</li>
+ *     <li>Without a region, {@code us-east-1} is used. minio would look up the bucket region with a


Instead of setting the region to an existing AWS region (which can be confusing when forgetting to supply a region), configure it to none instead.

vierbergenlars · 2026-08-03T08:08:35Z

+ *     <li>The default profile file is replaced with an empty one, so the SDK never reads {@code ~/.aws/config}
+ *     or {@code ~/.aws/credentials}. minio never read those files, and a broken file there (e.g. written by a
+ *     newer aws CLI) would otherwise break client construction.</li>


A broken file is not really the reason why we don't want it to read the profile file.

We don't want to read that, because when running it for local development, we don't want it to pick up any potential AWS configuration and credentials that are present on the user account. That may result in accidentally connecting to some production environment, which we certainly don't want.

vierbergenlars · 2026-08-03T08:08:49Z

+ *     or {@code ~/.aws/credentials}. minio never read those files, and a broken file there (e.g. written by a
+ *     newer aws CLI) would otherwise break client construction.</li>
+ * </ul>
+ * Internal to the appserver auto-configuration; not intended as public API.


Then make it package-private

vierbergenlars · 2026-08-03T08:10:54Z

    implementation project(':contentgrid-appserver-blueprintartifact-impl-utils')
-    implementation 'io.minio:minio'
+    implementation('software.amazon.awssdk:s3') {
+        // Only the synchronous ApacheHttpClient is used; keep the Netty-based async client off the classpath


Why are we explicilty choosing a different option than the supplied default?

@michyspiri Did you miss this question?

vierbergenlars · 2026-08-03T08:16:54Z

+    private void writeObject(ContentReference contentReference, InputStream inputStream) throws IOException {
+        // readNBytes allocates proportionally to the stream size, so small objects don't cost a full part buffer
+        var firstPart = inputStream.readNBytes(PART_SIZE);
+        if (firstPart.length < PART_SIZE) {
+            client.putObject(PutObjectRequest.builder()
+                            .bucket(bucketName)
+                            .key(contentReference.getValue())
+                            .contentType(CONTENT_TYPE)
+                            .build(),
+                    requestBodyFor(firstPart, firstPart.length));
+        } else {
+            writeObjectMultipart(contentReference, inputStream, firstPart);
+        }
+    }
+
+    /**
+     * @param buffer completely filled with the first part; reused as read buffer for every following part
+     */
+    private void writeObjectMultipart(ContentReference contentReference, InputStream inputStream, byte[] buffer)
+            throws IOException {
+        var uploadId = client.createMultipartUpload(CreateMultipartUploadRequest.builder()
+                        .bucket(bucketName)
+                        .key(contentReference.getValue())
+                        .contentType(CONTENT_TYPE)
+                        .build())
+                .uploadId();
+        try {
+            var completedParts = new ArrayList<CompletedPart>();
+            var bytesInBuffer = buffer.length;
+            while (bytesInBuffer > 0) {
+                var partNumber = completedParts.size() + 1;
+                var uploadPartResponse = client.uploadPart(UploadPartRequest.builder()
+                                .bucket(bucketName)
+                                .key(contentReference.getValue())
+                                .uploadId(uploadId)
+                                .partNumber(partNumber)
+                                .build(),
+                        requestBodyFor(buffer, bytesInBuffer));
+                completedParts.add(CompletedPart.builder()
+                        .partNumber(partNumber)
+                        .eTag(uploadPartResponse.eTag())
+                        .build());
+                if (bytesInBuffer < PART_SIZE) {
+                    break; // The stream ended inside this part, so it was the last one
+                }
+                bytesInBuffer = inputStream.readNBytes(buffer, 0, buffer.length);
+            }
+            client.completeMultipartUpload(CompleteMultipartUploadRequest.builder()
+                    .bucket(bucketName)
+                    .key(contentReference.getValue())
+                    .uploadId(uploadId)
+                    .multipartUpload(CompletedMultipartUpload.builder().parts(completedParts).build())
+                    .build());
+        } catch (SdkException | IOException e) {
+            abortMultipartUpload(contentReference, uploadId, e);
+            throw e;
+        }
+    }
+
+    private void abortMultipartUpload(ContentReference contentReference, String uploadId, Exception cause) {
+        try {
+            client.abortMultipartUpload(AbortMultipartUploadRequest.builder()
+                    .bucket(bucketName)
+                    .key(contentReference.getValue())
+                    .uploadId(uploadId)
+                    .build());
+        } catch (SdkException abortFailure) {
+            cause.addSuppressed(abortFailure);
+        }
+    }
+
+    private static RequestBody requestBodyFor(byte[] buffer, int length) {
+        return RequestBody.fromInputStream(new ByteArrayInputStream(buffer, 0, length), length);
+    }


The async S3 client can handle multipart uploads natively, without us having to write logic to handle it.

It also supports parallel uploads, which make the upload process faster.

Why not use that client instead? https://docs.aws.amazon.com/sdk-for-java/latest/developer-guide/best-practices-s3-uploads.html

…credentials, configurable path-style, region none, native multipart via S3AsyncClient

sonarqubecloud · 2026-08-04T10:12:14Z

Quality Gate passed

Issues
6 New issues
0 Accepted issues

Measures
0 Security Hotspots
95.4% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Code was updated

vierbergenlars · 2026-08-05T11:05:24Z

+    private final S3Client client;
+
+    @NonNull
+    private final S3AsyncClient asyncClient;


Why use 2 clients? Can't everything be be done with the S3AsyncClient without any problems?

vierbergenlars · 2026-08-05T11:07:12Z

I don't understand why this suddenly has to be moved to a separate autoconfiguration.

Consequence of the package-private factory

vierbergenlars · 2026-08-05T11:23:19Z

-    private void writeObjectMultipart(ContentReference contentReference, InputStream inputStream, byte[] buffer)
-            throws IOException {
-        var uploadId = client.createMultipartUpload(CreateMultipartUploadRequest.builder()
+        var requestBody = AsyncRequestBody.forBlockingInputStream(null); // length unknown


Wny not use AsyncRequestBody.fromInputStream(inputStream, null); instead of managing a separate thread ourselves?

That makes use of an internal threadpool, which saves on spawning threads and on the complexity of this code.

vierbergenlars · 2026-08-05T11:28:16Z

+                .responseChecksumValidation(ResponseChecksumValidation.WHEN_REQUIRED)
+                .multipartEnabled(true)
+                .multipartConfiguration(MultipartConfiguration.builder()
+                        .thresholdInBytes(PART_SIZE)


According to the javadoc, this defaults to the same value as minimumPartSizeInBytes, so there is no reason to specify this.

vierbergenlars · 2026-08-05T11:32:16Z

    implementation project(':contentgrid-appserver-blueprintartifact-impl-utils')
-    implementation 'io.minio:minio'
+    implementation('software.amazon.awssdk:s3') {
+        // Only the synchronous ApacheHttpClient is used; keep the Netty-based async client off the classpath


@michyspiri Did you miss this question?

vierbergenlars · 2026-08-05T11:34:06Z

 | Property | Description | Default | Required |
 |---|---|---|---|
-| `contentgrid.appserver.content.s3.url` | S3 endpoint URL (e.g. `https://s3.amazonaws.com` or a MinIO URL). | — | Yes |
+| `contentgrid.appserver.content.s3.url` | S3 endpoint URL, including the scheme (e.g. `https://s3.amazonaws.com` or a MinIO URL). | — | Yes |


Well yes, a URL includes the scheme, otherwise it is not a URL.

Suggested change

| `contentgrid.appserver.content.s3.url` | S3 endpoint URL, including the scheme (e.g. `https://s3.amazonaws.com` or a MinIO URL). | — | Yes |

| `contentgrid.appserver.content.s3.url` | S3 endpoint URL (e.g. `https://s3.amazonaws.com` or a MinIO URL). | — | Yes |

michyspiri added 3 commits July 31, 2026 10:31

ACC-3102 Migrate S3 content store from minio-java-sdk to AWS SDK v2

d5ba152

ACC-3102 Migrate S3 blueprint artifact from minio-java-sdk to AWS SDK v2

2db5a47

ACC-3102 Drop the io.minio dependency

9e27cd2

michyspiri requested a review from a team as a code owner July 31, 2026 08:47

michyspiri requested review from AndreasVAmexio and NielsCW July 31, 2026 08:47

AndreasVAmexio reviewed Jul 31, 2026

View reviewed changes

AndreasVAmexio previously requested changes Jul 31, 2026

View reviewed changes

ACC-3102 Enforce no connection re-use with a Connection close header

002e8ae

vierbergenlars reviewed Aug 3, 2026

View reviewed changes

ACC-3102 Address second review round: drop minio references, require …

bde3934

…credentials, configurable path-style, region none, native multipart via S3AsyncClient

AndreasVAmexio self-requested a review August 4, 2026 13:45

vierbergenlars reviewed Aug 5, 2026

View reviewed changes

	\| `contentgrid.appserver.content.s3.url` \| S3 endpoint URL, including the scheme (e.g. `https://s3.amazonaws.com` or a MinIO URL). \| — \| Yes \|
	\| `contentgrid.appserver.content.s3.url` \| S3 endpoint URL (e.g. `https://s3.amazonaws.com` or a MinIO URL). \| — \| Yes \|

Uh oh!

Conversation

michyspiri commented Jul 31, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud Bot commented Aug 4, 2026

Quality Gate passed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants