[Schema Registry Avro] New Encoder Design

[deyaaeldeen]

Fixes #20061

Overview

Revamps the schema registry encoder to work on messages instead of buffers based on the recommendation of the Azure messaging architect.

This changes the APIs as follows:

  const buffer: NodeJS.Buffer = await serializer.serialize(value, schema);

becomes

  const message: MessageWithMetadata = await encoder.encodeMessageData(value, schema);

where MessageWithMetadata has a body field as well as a contentType field. The latter's format is avro/binary+<Schema ID>

For derserializing, the change is as follows:

  const deserializedValue = await serializer.deserialize(buffer);

becomes:

  const decodedObject = await encoder.decodeMessageData(message);

Improvement upon #15959

This design introduces a new messageAdapter option in the encoder constructor to support processing of any message type (e.g. cloud event):

  const encoder = new SchemaRegistryAvroEncoder(schemaRegistryClient, {
    groupName,
    messageAdapter: adapter
  });

where adapter is a message adapter that follows the following contract:

interface MessageAdapter<MessageT> {
  produceMessage: (messageWithMetadata: MessageWithMetadata) => MessageT;
  consumeMessage: (message: MessageT) => MessageWithMetadata;
}

 interface MessageWithMetadata {
  body: Uint8Array;
  contentType: string;
}

For convenience, the PR adds a couple of convenience adapter factories for Event Hubs's EventData and Event Grid's SendCloudEventInput<Uint8Array>. For example, the createCloudEventAdapter factory can be called to construct an adapter for the latter as follows:

const adapter = createCloudEventAdapter({
      type: "azure.sdk.eventgrid.samples.cloudevent",
      source: "/azure/sdk/schemaregistry/samples/withEventGrid",
    }),

Note that these adapter factories are exported by their respective messaging package without explicitly implementing the contract and the PR adds new encoder tests that check whether the produced adapters follow the contract. This organization could change in the future if we create a new core place for the contract to be imported from.

See the newly added samples for how to send such messages with Event Hubs and Event Grid.

Schema Registry commitment tracking: #15959
Tracking issue: #18608
First iteration design: #18365

[azure-sdk]

API changes have been detected in @azure/schema-registry-avro. You can review API changes here

[azure-sdk]

API changes have been detected in @azure/schema-registry-avro. You can review API changes here

[azure-sdk]

API changes have been detected in @azure/event-hubs. You can review API changes here

API changes

+ export declare function createEventDataAdapter(params?: EventDataAdapterParameters): MessageAdapter<EventData>;
+             [key: string]: any;
+         };
+ }
+ export interface EventDataAdapterParameters {
+     correlationId?: string | number | Buffer;
+     messageId?: string | number | Buffer;
+     properties?: {

[deyaaeldeen]

@ellismg Hey Matt, I am updating @azure/event-grid to export cloud event adapter for the Schema Registry Avro encoder, could you take a look? Also, the APIView warnings are because I am not exporting the adapter interface for now. Eventually, we could move it to a place where all messaging libraries could import it from.

Also, could you take a look at the withEventGrid sample being added here?

[xirzec]

New API looks great!

Can you also file an issue for tracking moving the common interfaces to some core package if you haven't already?

[xirzec]

dataSchema? Or is this an unfortunate external standard?

[deyaaeldeen]

It appears that this particular field is not coming from the cloud event spec but I noticed that all fields in the SendCloudEventInput<T> follow the same all-lower-case pattern. /cc @ellismg

[JoshLove-msft]

https://github.com/cloudevents/spec/blob/v1.0.1/spec.md#dataschema

[deyaaeldeen]

@ellismg Hey Matt, @JoshLove-msft and I talked and we agreed on updating Event Grid later when we hear more from Event Hubs customers. I moved Event Grid changes to a draft in #20037.

[azure-sdk]

API changes have been detected in @azure/event-hubs. You can review API changes here

[JoshLove-msft]

How did we choose these properties for the adapter parameters? Do you have an example of how they are used?

[deyaaeldeen]

They're chosen by selecting everything from the message type sane the payload and content type fields. See the EventData type for reference.

[JoshLove-msft]

So this will need to be kept in sync with the EventData surface manually? Is there a way to define it once and use it in both places?

[deyaaeldeen]

We could take out those fields from EventData and make it extend this new interface instead. The only drawbacks I can think of for doing this is if the customer wants to look at the type, they'll have to hop into another place to see the rest of it. @ramya-rao-a should we expect EventData to gain new fields in the future? if so, how do you feel about splitting EventData interface into two like that?

[ramya-rao-a]

I would prefer to keep EventData as is and you can use the Omit magic to omit payload and content type

[deyaaeldeen]

That is fair. I remember that @bterlson does not like Omit to show up in our public APIs so I will just keep things as they're assuming that we do not expect EventData to evolve. I can add a comment to point out the coupling between it and this interface.

[deyaaeldeen]

I added a unit test that checks whether EventDataAdapterParameters has the same keys as Omit<Omit<EventData, "body">, "contentType">.

[xirzec]

This is the way 👍

[xirzec]

I added a unit test that checks whether EventDataAdapterParameters has the same keys as Omit<Omit<EventData, "body">, "contentType">.

Nit: Omit<Omit<EventData, "body">, "contentType"> can be expressed as Omit<EventData, "body" | "contentType">.

[jeremymeng]

Looks good to me. I added some minor comments

[jeremymeng]

nit: is it okay for the samples to depend on a beta version of event hubs? Like in the case of samples of this lib are published but event hubs hasn't released yet. Probably fine if they are published not too long apart.

[deyaaeldeen]

I will double check, but I believe the release pipeline will not let me release @azure/schema-registry-avro without releasing @azure/event-hubs@5.7.0-beta.2 first since the semver for the EH dep is ^5.7.0-beta.2.

[xirzec]

The new API surface looks good!

[deyaaeldeen]

Can you also file an issue for tracking moving the common interfaces to some core package if you haven't already?

I created #20086