jeantil commented on code in PR #2960:
URL: https://github.com/apache/james-project/pull/2960#discussion_r2914127919
##########
server/blob/blob-api/src/main/java/org/apache/james/blob/api/BlobStoreDAO.java:
##########
@@ -19,86 +19,128 @@
package org.apache.james.blob.api;
+import java.io.ByteArrayInputStream;
+import java.io.IOException;
import java.io.InputStream;
-import java.nio.ByteBuffer;
import java.nio.charset.StandardCharsets;
import java.util.Collection;
+import java.util.Map;
import org.reactivestreams.Publisher;
+import com.google.common.collect.ImmutableMap;
import com.google.common.io.ByteSource;
+import com.google.common.io.FileBackedOutputStream;
public interface BlobStoreDAO {
- class ReactiveByteSource {
- private final long size;
- private final Publisher<ByteBuffer> content;
+ record BlobMetadataName(String name) {
Review Comment:
how about using an enum instead of a record ?
it would mean we support less metadata fields but we would have stronger
semantics,
##########
server/blob/blob-api/src/main/java/org/apache/james/blob/api/BlobStoreDAO.java:
##########
@@ -19,86 +19,128 @@
package org.apache.james.blob.api;
+import java.io.ByteArrayInputStream;
+import java.io.IOException;
import java.io.InputStream;
-import java.nio.ByteBuffer;
import java.nio.charset.StandardCharsets;
import java.util.Collection;
+import java.util.Map;
import org.reactivestreams.Publisher;
+import com.google.common.collect.ImmutableMap;
import com.google.common.io.ByteSource;
+import com.google.common.io.FileBackedOutputStream;
public interface BlobStoreDAO {
- class ReactiveByteSource {
- private final long size;
- private final Publisher<ByteBuffer> content;
+ record BlobMetadataName(String name) {
+ // TODO validation (a-z,A-Z,0-9 -_) &length 128 char & non empty
+ }
+ record BlobMetadataValue(String value) {
+
+ }
+
+ sealed interface Blob {
+ Map<BlobMetadataName, BlobMetadataValue> metadata();
+
+ // Have the POJOs encode some conversions ?
Review Comment:
:+1:
##########
server/blob/blob-api/src/main/java/org/apache/james/blob/api/BlobStoreDAO.java:
##########
@@ -19,86 +19,128 @@
package org.apache.james.blob.api;
+import java.io.ByteArrayInputStream;
+import java.io.IOException;
import java.io.InputStream;
-import java.nio.ByteBuffer;
import java.nio.charset.StandardCharsets;
import java.util.Collection;
+import java.util.Map;
import org.reactivestreams.Publisher;
+import com.google.common.collect.ImmutableMap;
import com.google.common.io.ByteSource;
+import com.google.common.io.FileBackedOutputStream;
public interface BlobStoreDAO {
- class ReactiveByteSource {
- private final long size;
- private final Publisher<ByteBuffer> content;
+ record BlobMetadataName(String name) {
+ // TODO validation (a-z,A-Z,0-9 -_) &length 128 char & non empty
+ }
+ record BlobMetadataValue(String value) {
+
+ }
+
+ sealed interface Blob {
+ Map<BlobMetadataName, BlobMetadataValue> metadata();
+
+ // Have the POJOs encode some conversions ?
+ InputStreamBlob asInputStream() throws IOException;
+
+ BytesBlob asBytes() throws IOException;
+
+ ByteSourceBlob asByteSource() throws IOException;
+ }
+
+ record BytesBlob(byte[] payload, Map<BlobMetadataName, BlobMetadataValue>
metadata) implements Blob {
+ public static BytesBlob of(byte[] payload) {
+ return of(payload, ImmutableMap.of());
+ }
+
+ public static BytesBlob of(String payload) {
+ return of(payload.getBytes(StandardCharsets.UTF_8),
ImmutableMap.of());
+ }
- public ReactiveByteSource(long size, Publisher<ByteBuffer> content) {
- this.size = size;
- this.content = content;
+ public static BytesBlob of(byte[] payload, Map<BlobMetadataName,
BlobMetadataValue> metadata) {
+ return new BytesBlob(payload, metadata);
}
- public long getSize() {
- return size;
+ @Override
+ public InputStreamBlob asInputStream() {
+ return new InputStreamBlob(new ByteArrayInputStream(payload),
metadata);
}
- public Publisher<ByteBuffer> getContent() {
- return content;
+ @Override
+ public BytesBlob asBytes() {
+ return this;
+ }
+
+ @Override
+ public ByteSourceBlob asByteSource() {
+ return new ByteSourceBlob(ByteSource.wrap(payload), metadata);
}
}
+ record InputStreamBlob(InputStream payload, Map<BlobMetadataName,
BlobMetadataValue> metadata) implements Blob {
+ public static InputStreamBlob of(InputStream payload) {
+ return of(payload, ImmutableMap.of());
+ }
- /**
- * Reads a Blob based on its BucketName and its BlobId.
- *
- * @throws ObjectNotFoundException when the blobId or the bucket is not
found
- * @throws ObjectStoreIOException when an unexpected IO error occurs
- */
- InputStream read(BucketName bucketName, BlobId blobId) throws
ObjectStoreIOException, ObjectNotFoundException;
+ public static InputStreamBlob of(InputStream payload,
Map<BlobMetadataName, BlobMetadataValue> metadata) {
+ return new InputStreamBlob(payload, metadata);
+ }
- Publisher<InputStream> readReactive(BucketName bucketName, BlobId blobId);
+ @Override
+ public InputStreamBlob asInputStream() {
+ return this;
+ }
- /**
- * Reads a Blob based on its BucketName and its BlobId
- *
- * @return a Mono containing the content of the blob or
- * an ObjectNotFoundException in its error channel when the blobId or the
bucket is not found
- * or an IOObjectStoreException when an unexpected IO error occurs
- */
- Publisher<byte[]> readBytes(BucketName bucketName, BlobId blobId);
+ @Override
+ public BytesBlob asBytes() throws IOException {
+ return new BytesBlob(payload.readAllBytes(), metadata);
+ }
+ @Override
+ public ByteSourceBlob asByteSource() throws IOException {
+ try (FileBackedOutputStream fileBackedOutputStream = new
FileBackedOutputStream(100 * 1024)) {
Review Comment:
`100*1024` magic number
if I understand correctly this will write the stream to a file if the length
is about 100kbits
##########
server/blob/blob-api/src/main/java/org/apache/james/blob/api/BlobStoreDAO.java:
##########
@@ -19,86 +19,128 @@
package org.apache.james.blob.api;
+import java.io.ByteArrayInputStream;
+import java.io.IOException;
import java.io.InputStream;
-import java.nio.ByteBuffer;
import java.nio.charset.StandardCharsets;
import java.util.Collection;
+import java.util.Map;
import org.reactivestreams.Publisher;
+import com.google.common.collect.ImmutableMap;
import com.google.common.io.ByteSource;
+import com.google.common.io.FileBackedOutputStream;
public interface BlobStoreDAO {
- class ReactiveByteSource {
- private final long size;
- private final Publisher<ByteBuffer> content;
+ record BlobMetadataName(String name) {
+ // TODO validation (a-z,A-Z,0-9 -_) &length 128 char & non empty
+ }
+ record BlobMetadataValue(String value) {
+
+ }
+
+ sealed interface Blob {
+ Map<BlobMetadataName, BlobMetadataValue> metadata();
Review Comment:
As mentionned above it would be better for keys to be known,
alternatively this could be an object with kmown fields which james cares
about and a map for extra fields which are not interpreted by james
##########
server/blob/blob-memory/src/main/java/org/apache/james/blob/memory/MemoryBlobStoreDAO.java:
##########
@@ -62,12 +62,18 @@ public Publisher<InputStream> readReactive(BucketName
bucketName, BlobId blobId)
.map(ByteArrayInputStream::new);
}
- @Override
public Mono<byte[]> readBytes(BucketName bucketName, BlobId blobId) {
return Mono.fromCallable(() -> blobs.get(bucketName, blobId))
.switchIfEmpty(Mono.error(() -> new
ObjectNotFoundException(String.format("blob '%s' not found in bucket '%s'",
blobId.asString(), bucketName.asString()))));
}
+ @Override
+ public Publisher<BytesBlob> readBytesBlob(BucketName bucketName, BlobId
blobId) {
+ return Mono.fromCallable(() -> blobs.get(bucketName, blobId))
+ .switchIfEmpty(Mono.error(() -> new
ObjectNotFoundException(String.format("blob '%s' not found in bucket '%s'",
blobId.asString(), bucketName.asString()))))
+ .map(BytesBlob::of);
Review Comment:
🤔 shouldn't the in memory be adapted to actually store the metadata ? the
MemoryBlobStoreDao will always loose all metadata.
I know we can derive the size from the content but why have a Map<X,Y> for
metadatas if all the stores canx extract is the size>?
same comment applied to all leafs implementations
##########
server/blob/blob-api/src/main/java/org/apache/james/blob/api/BlobStoreDAO.java:
##########
@@ -143,14 +143,7 @@ public ByteSourceBlob asByteSource() {
Publisher<InputStream> readReactive(BucketName bucketName, BlobId blobId);
- /**
- * Reads a Blob based on its BucketName and its BlobId
- *
- * @return a Mono containing the content of the blob or
- * an ObjectNotFoundException in its error channel when the blobId or the
bucket is not found
- * or an IOObjectStoreException when an unexpected IO error occurs
- */
- Publisher<byte[]> readBytes(BucketName bucketName, BlobId blobId);
+ Publisher<BytesBlob> readBytesBlob(BucketName bucketName, BlobId blobId);
Review Comment:
while the description was useless the details in the return had some value
the whole blob library probably deserves more documentation instead of less
😬
##########
server/blob/blob-aes/src/main/java/org/apache/james/blob/aes/AESBlobStoreDAO.java:
##########
@@ -102,9 +102,11 @@ public InputStream read(BucketName bucketName, BlobId
blobId) throws ObjectStore
}
@Override
- public Publisher<InputStream> readReactive(BucketName bucketName, BlobId
blobId) {
- return Mono.from(underlying.readReactive(bucketName, blobId))
- .map(Throwing.function(this::decrypt));
+ public Publisher<InputStreamBlob> readBlobReactive(BucketName bucketName,
BlobId blobId) {
+ return Mono.from(underlying.readBlobReactive(bucketName, blobId))
+ .map(InputStreamBlob::payload)
+ .map(Throwing.function(this::decrypt))
+ .map(InputStreamBlob::of);
Review Comment:
this is stripping any metadata returned by the underlying store
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
