JAMES-4182 Proposal for changes in BlobStoreDAO interface

[chibenwa]

Opened to collect initial feedback

See https://issues.apache.org/jira/browse/JAMES-4182 for context

Remaining points:

• better validation for metadata
• Store metadata in blob stores
  • xattr for files
  • extra collumn for PG, cassandra
  • Object metadata for S3
  • Direct storage for memory
  • Passing those around for other blobStore DAO
  • AES blob store should support metadata

[jeantil]

* [ ]  better validation for metadata 
* [ ]  Store metadata in blob stores       
  * [ ]  xattr for files
  * [ ]  extra collumn for PG, cassandra
  * [ ]  Object metadata for S3
  * [ ]  Direct storage for memory
  * [ ]  Passing those around for other blobStore DAO

I think we should only use dedicated properties to store acitvely managed metadata, the remaining metadata as a Map<String,String> can be serializaed with the payload. this avoids possible capability conflicts between blog stores.

[jeantil]

Looks like a good direction to me, I left some comments, I'm sure there are which you are aware of and will addres, feel free to close on sight

[jeantil]

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

[chibenwa]

@jeantil do we have an agreement on continuing this work with an open metadata model?

[jeantil]

@jeantil do we have an agreement on continuing this work with an open metadata model?

I'm not comfortable working with only the open metadata model. I understand the extension point argument but
I would much prefer an hybrid approach where metadata which is actively used by james is materialized as such in the metadata object and accessed through structured accessors instead of iterating through a map comparing strings every time we need the information
The rest of available metadata can be left as an unstructured map as some kind of extension point

[chibenwa]

I would much prefer an hybrid approach where metadata which is actively used by james is materialized as such in the metadata object and accessed through structured accessors instead of iterating through a map comparing strings every time we need the information

I propose:

• Keeping the undlying data as (typed) Map<String, String>
• Adding convenience factory methods for manipulating known value

Essentially:

record BlobMetadata(Map<BlobMetadataName, BlobMetadataValue> metadata) {
   public static BlobMetadata empty() {...}
   
   public ContentTransferEncoding contentTransferEncoding() {
       return ContentTransferEncoding.fromBlobMetadataValue(metatdata.get(BlobMetadataName.CONTENT_TRANSFER_ENCODING));
   }
   
   public BlobMetadata withMetadata(BlobMetadataName name, BlobMetadataValue value) {
       return new BlobMetadata(ImmutableList.builder()
           .putAll(metadata)
           .put(name, value)
           .build());
   }
   
   public BlobMetadata withContentTransferEncoding(ContentTransferEncoding encoding) {
       return withMetadata(BlobMetadataName.CONTENT_TRANSFER_ENCODING, encoding.asBlobMetadataValue());
   }
}

I think it keeps the underlying data model simple, extensible, easy to work with in DAOs, and it address your concerns and makes James-used metadata a prime citizen.

[jeantil]

this looks ok for me, ideally we can enforce usage of an immutable map especially if it remains accessible from outside

[chibenwa]

ideally we can enforce usage of an immutable map especially if it remains accessible from outside

Of course ;-)

[chibenwa]

I'd like to thank you for the review effort @jeantil

I will put together the effort to align the code with the comments, and continue the implementation likely next week.

[chibenwa]

Hello @jeantil

I did rework the metadata model and wrote memory implementation.

I'll carry other with other impelementation when I have time, but wanted to push now to enable early review, in case people have time.

[jeantil]

I'll use this one as an example to let you pick the variant you prefer, I'm fine with both :D

Suggested change

	assertThat(Mono.from(testee.readBytes(TEST_BUCKET_NAME, TEST_BLOB_ID)).block().metadata().metadata())
	assertThat(Mono.from(testee.readBytes(TEST_BUCKET_NAME, TEST_BLOB_ID)).block().metadata().values())
	assertThat(Mono.from(testee.readBytes(TEST_BUCKET_NAME, TEST_BLOB_ID)).block().metadata().underlyingMap())

[quantranhong1999]

Hello,

I would like to help to move on this work.

I rebased this work on master, fixed the compilation, adapted review comments, and fixed some blob store tests.

Status: BlobStoreDAO's API refactoring + memory storage for blob's metadata + test contract for metadata should be ready. (Hopefully, we have a green build)

You can review the appended commits since the JAMES-4182 Fix compilation commit.

I propose to split the blob metadata storage for other implementations (S3, file, Cassandra, Postgres) into other PRs.

Also, should we fire a mailing list thread to draw the potential community's attention and feedback on this topic, if any?

[chibenwa]

Good with me!

Thanks for the help on this @quantranhong1999

[Arsnael]

Is this related? https://ci-builds.apache.org/job/james/job/ApacheJames/job/PR-2960/4/testReport/

[quantranhong1999]

Is this related? https://ci-builds.apache.org/job/james/job/ApacheJames/job/PR-2960/4/testReport/

It seems YES. I will have a look tomorrow.

[quantranhong1999]

Green build.

Let's wait for @jeantil review on this :-).

[chibenwa]

+1 OK with me

[quantranhong1999]

I propose to split the blob metadata storage for other implementations (S3, file, Cassandra, Postgres) into other PRs.

See WIP: #3001

[chibenwa]

Thanks a lot for continuing this @quantranhong1999

@jeantil I propose to merge this and move forward the implementation of metadata across the DAOs. If there is complementary feedback, we would handle the comments ;-)

Cheers,

Benoit

[jeantil]

Hello

A couple comments

• i don't think there is a test documenting the metadata name format
• I'm aware this is not currently the case but adding comments or some kind of documentation explaining the design of BlobStoreDAO would be helpful. It always takes me quite some time to recontextualise this code.

Something to explain that this is james virtual blobstore abstraction. That the bucketname is a james specific logical bucket which may be represented in various ways in actual storage connectors. In particular it should not be conflated with an S3 bucket name.
Then an explanation over why metadata is exposed (use case which justified the implementation and potential use cases ) possibly explain the distinction between metadata actively used by james which has direct helpers (and thus new helpers should be added if james starts to use more metadata) vs the rest which is exposed as an extension point for users of james libraries.
I know that technically this information is available in the JIRA and in this MR's discussion but this requires extra effort to discover or rediscover over having the why directly exposed in the contract.

The class documentation should be kept rather high level to limit it's tendency to rot but still explain the major design choices.

cheers

[quantranhong1999]

Hi Jean,

i don't think there is a test documenting the metadata name format

Likely we can add it in BlobMetadataTest.

I'm aware this is not currently the case but adding comments or some kind of documentation explaining the design of BlobStoreDAO would be helpful. It always takes me quite some time to recontextualise this code.

+1. We would be happy to plan to add this documentation.

Thank you for your review.

Quan