From 6b90b730e39e029e7ce3644ee9ded0a647343f95 Mon Sep 17 00:00:00 2001 From: Matthieu Baechler Date: Sat, 14 Oct 2023 11:06:32 +0200 Subject: [PATCH 1/2] Introduce a ResolvedBucketName type to avoid temporal coupling (and enhance readability) --- .../james/blob/api/ResolvedBucketName.java | 66 +++++++++++++++++++ .../objectstorage/aws/BucketNameResolver.java | 23 ++++--- .../objectstorage/aws/S3BlobStoreDAO.java | 40 ++++++----- .../aws/BucketNameResolverTest.java | 17 ++--- 4 files changed, 109 insertions(+), 37 deletions(-) create mode 100644 server/blob/blob-api/src/main/java/org/apache/james/blob/api/ResolvedBucketName.java diff --git a/server/blob/blob-api/src/main/java/org/apache/james/blob/api/ResolvedBucketName.java b/server/blob/blob-api/src/main/java/org/apache/james/blob/api/ResolvedBucketName.java new file mode 100644 index 00000000000..7fd47d01ce7 --- /dev/null +++ b/server/blob/blob-api/src/main/java/org/apache/james/blob/api/ResolvedBucketName.java @@ -0,0 +1,66 @@ +/**************************************************************** + * Licensed to the Apache Software Foundation (ASF) under one * + * or more contributor license agreements. See the NOTICE file * + * distributed with this work for additional information * + * regarding copyright ownership. The ASF licenses this file * + * to you under the Apache License, Version 2.0 (the * + * "License"); you may not use this file except in compliance * + * with the License. You may obtain a copy of the License at * + * * + * http://www.apache.org/licenses/LICENSE-2.0 * + * * + * Unless required by applicable law or agreed to in writing, * + * software distributed under the License is distributed on an * + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY * + * KIND, either express or implied. See the License for the * + * specific language governing permissions and limitations * + * under the License. * + ****************************************************************/ + +package org.apache.james.blob.api; + +import com.google.common.base.MoreObjects; +import com.google.common.base.Preconditions; +import org.apache.commons.lang3.StringUtils; + +import java.util.Objects; + +public final class ResolvedBucketName { + public static ResolvedBucketName of(String value) { + return new ResolvedBucketName(value); + } + + private final String value; + + private ResolvedBucketName(String value) { + Preconditions.checkNotNull(value); + Preconditions.checkArgument(StringUtils.isNotBlank(value), "`value` cannot be blank"); + + this.value = value; + } + + public String asString() { + return value; + } + + @Override + public final boolean equals(Object o) { + if (o instanceof ResolvedBucketName) { + ResolvedBucketName that = (ResolvedBucketName) o; + return Objects.equals(this.value, that.value); + } + return false; + } + + @Override + public final int hashCode() { + return Objects.hash(value); + } + + @Override + public String toString() { + return MoreObjects.toStringHelper(this) + .add("value", value) + .toString(); + } +} diff --git a/server/blob/blob-s3/src/main/java/org/apache/james/blob/objectstorage/aws/BucketNameResolver.java b/server/blob/blob-s3/src/main/java/org/apache/james/blob/objectstorage/aws/BucketNameResolver.java index 7b13ad3a9dd..d6b92aec6fd 100644 --- a/server/blob/blob-s3/src/main/java/org/apache/james/blob/objectstorage/aws/BucketNameResolver.java +++ b/server/blob/blob-s3/src/main/java/org/apache/james/blob/objectstorage/aws/BucketNameResolver.java @@ -24,6 +24,7 @@ import org.apache.james.blob.api.BucketName; import com.google.common.base.Preconditions; +import org.apache.james.blob.api.ResolvedBucketName; public class BucketNameResolver { static class Builder { @@ -84,20 +85,20 @@ private BucketNameResolver(Optional namespace, Optional pref this.prefix = prefix; } - BucketName resolve(BucketName bucketName) { + ResolvedBucketName resolve(BucketName bucketName) { Preconditions.checkNotNull(bucketName); if (isNameSpace(bucketName)) { - return bucketName; + return ResolvedBucketName.of(bucketName.asString()); } return prefix - .map(bucketPrefix -> BucketName.of(bucketPrefix + bucketName.asString())) - .orElse(bucketName); + .map(bucketPrefix -> ResolvedBucketName.of(bucketPrefix + bucketName.asString())) + .orElse(ResolvedBucketName.of(bucketName.asString())); } - Optional unresolve(BucketName bucketName) { + Optional unresolve(ResolvedBucketName bucketName) { if (isNameSpace(bucketName)) { - return Optional.of(bucketName); + return Optional.of(BucketName.of(bucketName.asString())); } return prefix.map(p -> { @@ -105,12 +106,18 @@ Optional unresolve(BucketName bucketName) { return Optional.of(BucketName.of(bucketName.asString().substring(p.length()))); } return Optional.empty(); - }).orElse(Optional.of(bucketName)); + }).orElse(Optional.of(BucketName.of(bucketName.asString()))); } private boolean isNameSpace(BucketName bucketName) { return namespace - .map(existingNamespace -> existingNamespace.equals(bucketName)) + .map(existingNamespace -> existingNamespace.asString().equals(bucketName.asString())) + .orElse(false); + } + + private boolean isNameSpace(ResolvedBucketName bucketName) { + return namespace + .map(existingNamespace -> existingNamespace.asString().equals(bucketName.asString())) .orElse(false); } } diff --git a/server/blob/blob-s3/src/main/java/org/apache/james/blob/objectstorage/aws/S3BlobStoreDAO.java b/server/blob/blob-s3/src/main/java/org/apache/james/blob/objectstorage/aws/S3BlobStoreDAO.java index 725cf8dcd8c..1bd96fc2077 100644 --- a/server/blob/blob-s3/src/main/java/org/apache/james/blob/objectstorage/aws/S3BlobStoreDAO.java +++ b/server/blob/blob-s3/src/main/java/org/apache/james/blob/objectstorage/aws/S3BlobStoreDAO.java @@ -38,11 +38,7 @@ import javax.net.ssl.TrustManagerFactory; import org.apache.commons.io.IOUtils; -import org.apache.james.blob.api.BlobId; -import org.apache.james.blob.api.BlobStoreDAO; -import org.apache.james.blob.api.BucketName; -import org.apache.james.blob.api.ObjectNotFoundException; -import org.apache.james.blob.api.ObjectStoreIOException; +import org.apache.james.blob.api.*; import org.apache.james.lifecycle.api.Startable; import org.apache.james.util.ReactorUtils; import org.reactivestreams.Publisher; @@ -192,7 +188,7 @@ public void close() { @Override public InputStream read(BucketName bucketName, BlobId blobId) throws ObjectStoreIOException, ObjectNotFoundException { - BucketName resolvedBucketName = bucketNameResolver.resolve(bucketName); + ResolvedBucketName resolvedBucketName = bucketNameResolver.resolve(bucketName); return ReactorUtils.toInputStream(getObject(resolvedBucketName, blobId) .onErrorMap(NoSuchBucketException.class, e -> new ObjectNotFoundException("Bucket not found " + resolvedBucketName.asString(), e)) @@ -203,7 +199,7 @@ public InputStream read(BucketName bucketName, BlobId blobId) throws ObjectStore @Override public Publisher readReactive(BucketName bucketName, BlobId blobId) { - BucketName resolvedBucketName = bucketNameResolver.resolve(bucketName); + ResolvedBucketName resolvedBucketName = bucketNameResolver.resolve(bucketName); return getObject(resolvedBucketName, blobId) .onErrorMap(NoSuchBucketException.class, e -> new ObjectNotFoundException("Bucket not found " + resolvedBucketName.asString(), e)) @@ -217,7 +213,7 @@ private static class FluxResponse { Flux flux; } - private Mono getObject(BucketName bucketName, BlobId blobId) { + private Mono getObject(ResolvedBucketName bucketName, BlobId blobId) { return Mono.fromFuture(() -> client.getObject( builder -> builder.bucket(bucketName.asString()).key(blobId.asString()), @@ -253,7 +249,7 @@ public void onStream(SdkPublisher publisher) { @Override public Mono readBytes(BucketName bucketName, BlobId blobId) { - BucketName resolvedBucketName = bucketNameResolver.resolve(bucketName); + ResolvedBucketName resolvedBucketName = bucketNameResolver.resolve(bucketName); return Mono.fromFuture(() -> client.getObject( @@ -268,7 +264,7 @@ public Mono readBytes(BucketName bucketName, BlobId blobId) { @Override public Mono save(BucketName bucketName, BlobId blobId, byte[] data) { - BucketName resolvedBucketName = bucketNameResolver.resolve(bucketName); + ResolvedBucketName resolvedBucketName = bucketNameResolver.resolve(bucketName); return Mono.fromFuture(() -> client.putObject( @@ -300,7 +296,7 @@ private Mono uploadUsingFile(BucketName bucketName, BlobId blobId, InputSt @Override public Mono save(BucketName bucketName, BlobId blobId, ByteSource content) { - BucketName resolvedBucketName = bucketNameResolver.resolve(bucketName); + ResolvedBucketName resolvedBucketName = bucketNameResolver.resolve(bucketName); return Mono.fromCallable(content::size) .flatMap(contentLength -> @@ -314,7 +310,7 @@ public Mono save(BucketName bucketName, BlobId blobId, ByteSource content) .then(); } - private Mono save(BucketName resolvedBucketName, BlobId blobId, InputStream stream, long contentLength) { + private Mono save(ResolvedBucketName resolvedBucketName, BlobId blobId, InputStream stream, long contentLength) { int chunkSize = Math.min((int) contentLength, CHUNK_SIZE); return Mono.fromFuture(() -> client.putObject(builder -> builder @@ -333,7 +329,7 @@ private Flux chunkStream(int chunkSize, InputStream stream) { .subscribeOn(Schedulers.boundedElastic()); } - private RetryBackoffSpec createBucketOnRetry(BucketName bucketName) { + private RetryBackoffSpec createBucketOnRetry(ResolvedBucketName bucketName) { return RetryBackoffSpec.backoff(MAX_RETRIES, FIRST_BACK_OFF) .maxAttempts(MAX_RETRIES) .doBeforeRetryAsync(retrySignal -> { @@ -349,7 +345,7 @@ private RetryBackoffSpec createBucketOnRetry(BucketName bucketName) { @Override public Mono delete(BucketName bucketName, BlobId blobId) { - BucketName resolvedBucketName = bucketNameResolver.resolve(bucketName); + ResolvedBucketName resolvedBucketName = bucketNameResolver.resolve(bucketName); return Mono.fromFuture(() -> client.deleteObject(delete -> delete.bucket(resolvedBucketName.asString()).key(blobId.asString()))) @@ -360,7 +356,9 @@ public Mono delete(BucketName bucketName, BlobId blobId) { @Override public Publisher delete(BucketName bucketName, Collection blobIds) { - return deleteObjects(bucketName, + ResolvedBucketName resolvedBucketName = bucketNameResolver.resolve(bucketName); + + return deleteObjects(resolvedBucketName, blobIds.stream() .map(BlobId::asString) .map(id -> ObjectIdentifier.builder().key(id).build()) @@ -370,12 +368,12 @@ public Publisher delete(BucketName bucketName, Collection blobIds) @Override public Mono deleteBucket(BucketName bucketName) { - BucketName resolvedBucketName = bucketNameResolver.resolve(bucketName); + ResolvedBucketName resolvedBucketName = bucketNameResolver.resolve(bucketName); return deleteResolvedBucket(resolvedBucketName); } - private Mono deleteResolvedBucket(BucketName bucketName) { + private Mono deleteResolvedBucket(ResolvedBucketName bucketName) { return emptyBucket(bucketName) .onErrorResume(t -> Mono.just(bucketName)) .flatMap(ignore -> Mono.fromFuture(() -> @@ -385,7 +383,7 @@ private Mono deleteResolvedBucket(BucketName bucketName) { .publishOn(Schedulers.parallel()); } - private Mono emptyBucket(BucketName bucketName) { + private Mono emptyBucket(ResolvedBucketName bucketName) { return Flux.from(client.listObjectsV2Paginator(builder -> builder.bucket(bucketName.asString()))) .flatMap(response -> Flux.fromIterable(response.contents()) .window(EMPTY_BUCKET_BATCH_SIZE) @@ -401,7 +399,7 @@ private Mono> buildListForBatch(Flux batch) { .collect(ImmutableList.toImmutableList()); } - private Mono deleteObjects(BucketName bucketName, List identifiers) { + private Mono deleteObjects(ResolvedBucketName bucketName, List identifiers) { return Mono.fromFuture(() -> client.deleteObjects(builder -> builder.bucket(bucketName.asString()).delete(delete -> delete.objects(identifiers)))); } @@ -411,7 +409,7 @@ public Mono deleteAllBuckets() { return Mono.fromFuture(client::listBuckets) .publishOn(Schedulers.parallel()) .flatMapIterable(ListBucketsResponse::buckets) - .flatMap(bucket -> deleteResolvedBucket(BucketName.of(bucket.name())), DEFAULT_CONCURRENCY) + .flatMap(bucket -> deleteResolvedBucket(ResolvedBucketName.of(bucket.name())), DEFAULT_CONCURRENCY) .then(); } @@ -420,7 +418,7 @@ public Publisher listBuckets() { return Mono.fromFuture(client::listBuckets) .flatMapIterable(ListBucketsResponse::buckets) .map(Bucket::name) - .handle((bucket, sink) -> bucketNameResolver.unresolve(BucketName.of(bucket)) + .handle((bucket, sink) -> bucketNameResolver.unresolve(ResolvedBucketName.of(bucket)) .ifPresent(sink::next)); } diff --git a/server/blob/blob-s3/src/test/java/org/apache/james/blob/objectstorage/aws/BucketNameResolverTest.java b/server/blob/blob-s3/src/test/java/org/apache/james/blob/objectstorage/aws/BucketNameResolverTest.java index 178830d4b1b..1894b5ad60d 100644 --- a/server/blob/blob-s3/src/test/java/org/apache/james/blob/objectstorage/aws/BucketNameResolverTest.java +++ b/server/blob/blob-s3/src/test/java/org/apache/james/blob/objectstorage/aws/BucketNameResolverTest.java @@ -23,6 +23,7 @@ import static org.assertj.core.api.Assertions.assertThatThrownBy; import org.apache.james.blob.api.BucketName; +import org.apache.james.blob.api.ResolvedBucketName; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; @@ -73,7 +74,7 @@ void unresolveShouldReturnPassedValue() { .namespace(BucketName.of("namespace")) .build(); - assertThat(resolver.unresolve(BucketName.of("bucketName"))) + assertThat(resolver.unresolve(ResolvedBucketName.of("bucketName"))) .contains(BucketName.of("bucketName")); } @@ -84,7 +85,7 @@ void unresolveShouldReturnValueWhenNamespace() { .namespace(BucketName.of("namespace")) .build(); - assertThat(resolver.unresolve(BucketName.of("namespace"))) + assertThat(resolver.unresolve(ResolvedBucketName.of("namespace"))) .contains(BucketName.of("namespace")); } } @@ -122,7 +123,7 @@ void unresolveShouldReturnPassedValueWithPrefix() { .noNamespace() .build(); - assertThat(resolver.unresolve(BucketName.of("bucketPrefix-bucketName"))) + assertThat(resolver.unresolve(ResolvedBucketName.of("bucketPrefix-bucketName"))) .contains(BucketName.of("bucketName")); } @@ -133,7 +134,7 @@ void unresolveShouldFilterValuesWithoutPrefix() { .noNamespace() .build(); - assertThat(resolver.unresolve(BucketName.of("bucketName"))) + assertThat(resolver.unresolve(ResolvedBucketName.of("bucketName"))) .isEmpty(); } } @@ -171,7 +172,7 @@ void unresolveShouldReturnPassedValue() { .noNamespace() .build(); - assertThat(resolver.unresolve(BucketName.of("bucketName"))) + assertThat(resolver.unresolve(ResolvedBucketName.of("bucketName"))) .contains(BucketName.of("bucketName")); } } @@ -221,7 +222,7 @@ void unresolveShouldFilterValuesWithoutPrefix() { .namespace(BucketName.of("namespace")) .build(); - assertThat(resolver.unresolve(BucketName.of("bucketName"))) + assertThat(resolver.unresolve(ResolvedBucketName.of("bucketName"))) .isEmpty(); } @@ -232,7 +233,7 @@ void unresolveShouldRemovePrefix() { .namespace(BucketName.of("namespace")) .build(); - assertThat(resolver.unresolve(BucketName.of("bucketPrefix-bucketName"))) + assertThat(resolver.unresolve(ResolvedBucketName.of("bucketPrefix-bucketName"))) .contains(BucketName.of("bucketName")); } @@ -243,7 +244,7 @@ void unresolveShouldReturnNamespaceWhenPassingNamespace() { .namespace(BucketName.of("namespace")) .build(); - assertThat(resolver.unresolve(BucketName.of("namespace"))) + assertThat(resolver.unresolve(ResolvedBucketName.of("namespace"))) .contains(BucketName.of("namespace")); } } From 0beadadb590b77f0f7b14fedce6ed42ef0af7e56 Mon Sep 17 00:00:00 2001 From: Matthieu Baechler Date: Fri, 24 Nov 2023 14:29:18 +0100 Subject: [PATCH 2/2] fixup! Introduce a ResolvedBucketName type to avoid temporal coupling (and enhance readability) --- .../blob/objectstorage/aws/BucketNameResolver.java | 1 - .../blob/objectstorage/aws}/ResolvedBucketName.java | 11 ++++++----- .../james/blob/objectstorage/aws/S3BlobStoreDAO.java | 6 +++++- .../objectstorage/aws/BucketNameResolverTest.java | 1 - 4 files changed, 11 insertions(+), 8 deletions(-) rename server/blob/{blob-api/src/main/java/org/apache/james/blob/api => blob-s3/src/main/java/org/apache/james/blob/objectstorage/aws}/ResolvedBucketName.java (96%) diff --git a/server/blob/blob-s3/src/main/java/org/apache/james/blob/objectstorage/aws/BucketNameResolver.java b/server/blob/blob-s3/src/main/java/org/apache/james/blob/objectstorage/aws/BucketNameResolver.java index d6b92aec6fd..8157b198be3 100644 --- a/server/blob/blob-s3/src/main/java/org/apache/james/blob/objectstorage/aws/BucketNameResolver.java +++ b/server/blob/blob-s3/src/main/java/org/apache/james/blob/objectstorage/aws/BucketNameResolver.java @@ -24,7 +24,6 @@ import org.apache.james.blob.api.BucketName; import com.google.common.base.Preconditions; -import org.apache.james.blob.api.ResolvedBucketName; public class BucketNameResolver { static class Builder { diff --git a/server/blob/blob-api/src/main/java/org/apache/james/blob/api/ResolvedBucketName.java b/server/blob/blob-s3/src/main/java/org/apache/james/blob/objectstorage/aws/ResolvedBucketName.java similarity index 96% rename from server/blob/blob-api/src/main/java/org/apache/james/blob/api/ResolvedBucketName.java rename to server/blob/blob-s3/src/main/java/org/apache/james/blob/objectstorage/aws/ResolvedBucketName.java index 7fd47d01ce7..af7c1004b20 100644 --- a/server/blob/blob-api/src/main/java/org/apache/james/blob/api/ResolvedBucketName.java +++ b/server/blob/blob-s3/src/main/java/org/apache/james/blob/objectstorage/aws/ResolvedBucketName.java @@ -17,15 +17,16 @@ * under the License. * ****************************************************************/ -package org.apache.james.blob.api; +package org.apache.james.blob.objectstorage.aws; + +import java.util.Objects; -import com.google.common.base.MoreObjects; -import com.google.common.base.Preconditions; import org.apache.commons.lang3.StringUtils; -import java.util.Objects; +import com.google.common.base.MoreObjects; +import com.google.common.base.Preconditions; -public final class ResolvedBucketName { +final class ResolvedBucketName { public static ResolvedBucketName of(String value) { return new ResolvedBucketName(value); } diff --git a/server/blob/blob-s3/src/main/java/org/apache/james/blob/objectstorage/aws/S3BlobStoreDAO.java b/server/blob/blob-s3/src/main/java/org/apache/james/blob/objectstorage/aws/S3BlobStoreDAO.java index 1bd96fc2077..f7b2c5e1d64 100644 --- a/server/blob/blob-s3/src/main/java/org/apache/james/blob/objectstorage/aws/S3BlobStoreDAO.java +++ b/server/blob/blob-s3/src/main/java/org/apache/james/blob/objectstorage/aws/S3BlobStoreDAO.java @@ -38,7 +38,11 @@ import javax.net.ssl.TrustManagerFactory; import org.apache.commons.io.IOUtils; -import org.apache.james.blob.api.*; +import org.apache.james.blob.api.BlobId; +import org.apache.james.blob.api.BlobStoreDAO; +import org.apache.james.blob.api.BucketName; +import org.apache.james.blob.api.ObjectNotFoundException; +import org.apache.james.blob.api.ObjectStoreIOException; import org.apache.james.lifecycle.api.Startable; import org.apache.james.util.ReactorUtils; import org.reactivestreams.Publisher; diff --git a/server/blob/blob-s3/src/test/java/org/apache/james/blob/objectstorage/aws/BucketNameResolverTest.java b/server/blob/blob-s3/src/test/java/org/apache/james/blob/objectstorage/aws/BucketNameResolverTest.java index 1894b5ad60d..77072f1659c 100644 --- a/server/blob/blob-s3/src/test/java/org/apache/james/blob/objectstorage/aws/BucketNameResolverTest.java +++ b/server/blob/blob-s3/src/test/java/org/apache/james/blob/objectstorage/aws/BucketNameResolverTest.java @@ -23,7 +23,6 @@ import static org.assertj.core.api.Assertions.assertThatThrownBy; import org.apache.james.blob.api.BucketName; -import org.apache.james.blob.api.ResolvedBucketName; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest;