Well known mime types and Composite metadata extension

[simonbasle]

This PR intends to cover the basics of the composite metadata and well known mime types spec extensions, currently incubating and originally defined in rsocket/rsocket#254

Essentially, the mime types are enumerated, while the composite metadata encoding and decoding is implemented as a package-private flyweight, all under the metadata package.

The CompositeMetadata exposes the composite components, although the iteration / access API can probably be improved.

It also exposes methods to encode individual metadata components into a ByteBuf, as well as to decode a CompositeMetadata. The flyweight decodeNext method could also be made public as an even lower-level API.

Open questions and improvement points:

• should the flyweight decodeNext method be public API?
• WellKnownMimeType lookups should probably avoid throwing exceptions?
• should CompositeMetadata#get* accessors throw exceptions too?
• should we introduce a more generic class instead of Entry, something like Micrometer's Tag and Tags, but with a generic value: Keyed<T> would have a String key and a <T> value, ByteBuf for the composite metadata. => matter for another issue
• ...

[robertroeser]

this should be done this with an if/else instead of catching an exception

[robertroeser]

This should return ByteBufs, and the Entry type should be a flyweight. We shouldn't need to allocate any objects when we read this

[simonbasle]

so ByteBuf[2]? in this case I'm thinking we can expose the mime buffer this way:

• if the mime is an encoded id, a 1 byte long ByteBuf that represents the still encoded id (raw slice from the original buffer, need to do further bit manipulation)
• if the mime is as custom string, a 2 to 129 bytes long ByteBuf, a slice of the underlying buffer that encompasses both the length byte and the mime string itself.

Since the mime string slide would have no more readable bytes than necessary to decode the string, the first byte could simply be discarded. Yet without it there would be a corner case ambiguity for 1 byte long buffer`, which can either be:

1. the case of a 1 byte encoded id
2. the case of a 1 byte long String

So reading the mime buffer is a matter of checking its size is 1 byte and applying the mask OR size is 2+ bytes and skipping 1 byte then reading as CharSequence.

Does that sound good?

[simonbasle]

note: the second ByteBuf would be a simple sliced of the original part that contains the metadata content, so its readableBytes reflect the size of the "content" (not including the encoded length)

[robertroeser]

We shouldn't need an Entry object to create an entry. This should be done with a method so we don't need to allocate an object.

[robertroeser]

i.e.
encodeIncrementally(ByteBufAllocator allocator, CompositeByteBuf compositeMetadataBuffer, String key, String value)

encodeIncrementally(ByteBufAllocator allocator, CompositeByteBuf compositeMetadataBuffer, WellKnownMimeTypes type, String value)

and so on

[simonbasle]

@robertroeser I modified the PR to have cleaner separation of concerns between CompositeMetadata vs CompositeMetadataFlyweight, which I have made public. For low-level low-garbage manipulation like you suggested, the flyweight can now be used. For a higher level abstraction over the composite metadata buffers, one can use CompositeMetadata and Entry.

[simonbasle]

6a77dd2 : removed the highest abstraction CompositeMetadata. As a result, Entry is now the higher abstraction, but not terribly fit for a top-level class. So I moved it into the CompositeMetadataFlyweight.

• If you want to only deal with ByteBuf, use the direct static methods of the CompositeMetadataFlyweight.
• If you want a slightly higher abstraction, decode such buffers into multiple Entry (using Entry#decodeEntry)
  • each Entry can be directly encoded into a CompositeByteBuf
  • each type of Entry can be manually created if needed, although the Entry#rawCompressedMime should generally be avoided.

[simonbasle]

@robertroeser @rstoyanchev I think this is finally in a good state, taking your respective comments into account.

[linux-china]

@simonbasle a quick question, could we add some open classes to operate metadata easy. I found Metadata class from Spring Cloud gateway, https://github.com/spring-cloud/spring-cloud-gateway/blob/master/spring-cloud-gateway-rsocket/src/main/java/org/springframework/cloud/gateway/rsocket/support/Metadata.java, but this Metadata.java just for routing, not abstract Metadata. The following just my thoughts. :)

public interface Metadata<T> {
    WellKnownMimeType mimeType();

    ByteBuffer encode();

     void load(ByteBuffer buffer); 

    T decode();

}

class CompositeMetadata {
   List<Metadata<?>> metadataList;
  
   void add(Metadata<?> metadata);
   List<Metadata<?>> getAllMetadataList();
   ByteBuffer encode();
   public static CompositeMetadata decode(ByteBuffer buffer);
}

class RoutingMetadata implements Metadata<String> {

}

[robertroeser]

Checkout out how FrameType looks up FrameType via its code:
https://github.com/rsocket/rsocket-java/blob/develop/rsocket-core/src/main/java/io/rsocket/frame/FrameType.java#L209

You can pre-compute the values into an array so you can look up using an array index instead of a for loop

[simonbasle]

thanks for the pointer, done 👍

it looks like the same for the String based lookup could be improved with a static Map<String, WellKnownMimeType>:

Benchmark                                          Mode  Cnt         Score         Error  Units
WellKnownMimeTypePerf.fromStringMapLookup         thrpt   10  43906830.064 ± 1775994.988  ops/s
WellKnownMimeTypePerf.fromStringValuesLoopLookup  thrpt   10   2041048.116 ±   42704.445  ops/s

wdyt?

[robertroeser]

yeah - that's a big difference

[robertroeser]

Does this adjust the readIndex? One the RSocket frames do is not adjust the read index to the call because it leads to weird problems when use the frames else.

Also I thought the first bit not the first byte was what in was what determined length- so if the first bit is 0 do you need to skip anything?

[simonbasle]

it indeed increases the readerIndex(). the first bit encodes the flag wether or not the remaining 7 bits represent a length or a mime id (if I interpreted the spec correctly, cc @nebhale). But since the previous step in decoding produced a buffer slice that has exactly stringMimeTypeLength + 1 readableBytes(), we can assume this whole byte is A) starting with 1 and B) irrelevant to decode again.

The idea behind including this extra byte in the buffer was to avoid ambiguity between 2 possible 1-byte cases: 1 byte representing a compressed mime id vs 1 byte representing a 1-char mime string. By including it, the minimum length for the string case is 2 bytes, and thus we can differentiate by simply looking at readableBytes().

[simonbasle]

note that I assumed it was ok to move the readerIndex() when decoding and slicing?
(I rely on said index having moved when further decoding the next entry in the composite metadata buffer)

[robertroeser]

Generally we've found its not okay to leave it changed once you're done with the flyweight. If you look at the RSocket frames they all reset the index afterwards to leave the bytebuf in the same state they found it.

[robertroeser]

It's probably easier to use ByteBufUtil.writeAscii(allocator, customMime); to create the mimeBuffer

[simonbasle]

I pushed a change that simplifies that encode method, avoiding the use of a CompositeByteBuf. But I think UTF8 writing is still required to detect non-ASCII chars, as writeAscii silently removes such characters (see commit message)

[robertroeser]

Where is the flyweight for the items in this array?

[simonbasle]

only the first of the two buffers needs to be decoded (IF it exists, as the array can be empty in case of malformed composite), and the methods to do so are in the same CompositeMetadataFlyweight:

• decodeMimeIdFromMimeBuffer in case readableBytes() == 1
• decodeMimeTypeFromMimeBuffer otherwise

[robertroeser]

The way I was thinking about doing this was Iterator that kept track of indexes and then a flyweight you would place over the ByteBuf slices the iterator emitted. You'd have an EntryFlyweight with a couple static methods.

[robertroeser]

Ok I see - the CompositeMetadata is the flyweight for an indiviual entry(?). How do you tell the demaraction between CompositeMetadata? Like if you have 8 metadatas and you want to iterator over them?

[simonbasle]

that's where the new CompositeMetadata comes in, with hasNext / decodeNext. alternatively the flyweight method returns an empty array when all was decoded

[robertroeser]

ok I see - you mutate the read index. That doesn't work like any of the other frames/payloads in RSocket. They all reset there read indexes are finished- they change the state of the ByteBuf to back where it was. We've found it's a lot easier to deal with ByteBufs that way, and update the read indexes as a side-effect makes the code harder to debug.

[simonbasle]

does that apply to slices? decodeMime[Type|Id]FromMimeBuffer both work from a slice of the original whole metadata buffer, so that skipBytes(1) in decodeMimeTypeFromMimeBuffer doesn't move the readerIndex of the whole composite metadata...

That said, the main decoding method does indeed move the full metadata buffer's readerIndex(). I thought it was unavoidable since the approach was to decode a subset of the whole buffer, without state keeping, and the length of the slices (metaheader and metacontent) can't be known without decoding.

But maybe that state can be externalized, as a startFromIndex parameter to decodeMimeAndContentBuffers? It would be a burden put on the user to keep track of the next startFromIndex to read from, though... She would have to do something like index = index + slices[0].readableBytes() + slices[1].readableBytes() + 1... (see below for comment on edit)

This can of course be done inside the CompositeMetadata "iterator", but for low level decoding it can be more work.

[simonbasle]

note The best way to compute the next entry index is to provide a flyweight util method, since the decoded slices are not necessarily covering contiguous portions of the orignal full metadata buffer (namely currently they don't include the 3 bytes content length)

[simonbasle]

@simonbasle a quick question, could we add some open classes to operate metadata easy.

There's an Entry under CompositeMetadataFlyweight that is intended as the sort of high level abstraction. Probably not a good name outside of the context of CompositeMetadataFlyweight though... The CompositeMetadata itself was part of first iterations but it was felt it didn't bring much to the table.
Currently for dealing with multiple Entry there is an List<Entry> decodeAll(ByteBuf) method, but after discussing with @rstoyanchev it might make more sense to reintroduce a simpler form of CompositeMetadata that would be Iterator-like:

CompositeMetadata composite = new CompositeMetadata(fullByteBuf);
while(composite.hasNext()) {
  composite.decodeNext();
  String mimeType = composite.getCurrentMimeType(); //returns null if reserved compressed id
  byte mimeId = composite.getCurrentMimeId(); //returns -1 if not compressed
  ByteBuf metadata = composite.getCurrentMetadata();
  
  //do something with each element, like decode the actual metadata content
  if (WellKnownMimeType.APPLICATION_JSON.getIdentifier() == mimeId) {
    addDecodedMetadata(decodeJsonMetadata(metadata));
  }
}

[simonbasle]

@robertroeser what do you think about CompositeMetadataFlyweight#encodeAndAddMetadata(..., String mimeType, ...) checking if the String matches a WellKnownMimeType? Maybe making that behavior opt-in via an additional boolean parameter?

[simonbasle]

@linux-china @rstoyanchev @robertroeser the two changes discussed in my last comments have now been implemented.

[robertroeser]

looks good - probably last thing is to switch the mime-types to hex numbers to match @nebhale's pull request - rsocket/rsocket#270

[nebhale]

@robertroeser I've been working with @simonbasle and @rwinch on the usage side of the CompositeMetadata type. After reviewing, we realized that at that level of abstraction (specifically not the Flyweight) it was important for it to be Iterable (for loop in imperative code, Flux.fromIterable() in reactive code). Therefore, we changed the CompositeMetadata API to be Iterable while still preserving the late-slicing functionality that the Flyweight provides us. The only stored data continues to be the next index.