[EventGrid] Initial Track 2 Client #10059
Conversation
In preperation for rewriting the client using the track two guidelines, remove the existing client.
ellismg
requested review from
KarishmaGhiya,
bterlson,
danieljurek,
mikeharder,
ramya-rao-a and
xirzec
as code owners
Initial commit of the Track 2 EventGrid client. Compared to the previous client, there are a few new high level features: - Sending events using the CloudEvent 1.0 schema, or a custom schema. - Authentication using a SAS Token - Generating SAS tokens - Converters to decode and consume events sent by Azure Services.
ellismg
force-pushed
the
ellismg/event-grid-client
branch
from
37d7a26 to
152bf8e
Compare
The test code to generate a date was using `new Date()` which does a local to UTC conversion. When run in CI we have a different local timezone and so the expect data does not match the actual data. Rewrite the test to use `Date.UTC` to construct a time that is in UTC.
xirzec
left a comment
xirzec
left a comment
There was a problem hiding this comment.
Overall it looks great!
Left some fit/finish comments
This would need to be downleveled for earlier versions of TypeScript. The only reason we followed the pattern of an interface with a member property and then a class with that property as a getter was because that's what we had done with `KeyCredential` and `AzureKeyCredential` but we can just save ourselves the pain by small tweaks to `SignatureCredential` and friends
Move extension attributes to be on a property of the `CloudEvent` object instead of just being defined at top level. This makes the TypeScript experience a little nicer as now you you typo the name of one of the know properties, the compiler will now issue an error.
This will make evolving the `EventGridConsumer` type more straight forward in the future.
Move the validation helpers out of `consumer.ts` and into `util.ts` and expose some high level functions for doing the validation.
This change takes some of the utility code from the Cosmos SDK which allows doing an SHA256 HMAC inside the browser (in addition to Node). In order to support this, we need to make `generateSharedAccessSignature` be async, since on the browser side some of the APIs we use are async themselves.
The feature crew has decided to use `EventGridPublisherClient` as the name for our client.
We are using `deserialize` instead of `decode` in other languages. This is a more common name overall.
We have reasonable defaults for cases where an ID or time are not present when an event is being generated (a new random UUID and the current time, respectively). Make the parameters on the model types optional and apply defaults as we are sending the events.
Previously this was an instance method on `EventGridClient`. We've decided that we would prefer it to be a top level function (where you explicitly pass the endpoint and key) instead of an instance method that re-uses these properties from the client (and fails when the client was constructed with a shared access signiture itself, instead of a key).
Instead of generating individual type guard methods for every system
event (which would lead to a bunch of exported functions which would
clutter up the documentation and like), we've adopted a model now where
we have a single `isSystemEvent` function. This takes two arguments,
the first is a string which represents the type name of a System Event
supported by the SDK (and we have an union type with all these strings
in it) and the second is the CloudEvent or EventGridEvent object to
test.
In practice this means instead of:
```typescript
if (isContainerRegistryChartPushedEvent(event)) {
// ...
} else if (isContainerRegistryChartDeletedEvent(event)) {
// ...
}
```
You now write:
```typescript
if (isSystemEvent("Microsoft.ContainerRegistry.ChartPushed", event)) {
// ...
} else if (isSystemEvent("Microsoft.ContainerRegistry.ChartDeleted", event)) {
// ...
}
```
The `isSystemEvent` function is authored in such a way that auto
completion will show you the set of strings that could be passed into
isSystemEvent.
This change does a few things: 1. Correctly handles binary data for a cloud event. The strategy here is that when the data value in the event is an `instanceof` `Uint8Array`, the convinece layer does a base64 encoding of the data and sets the `data_base64` property on the underlying model we send to the generated client. 2. When decoding, we do the opposite transform and set data in the returned model to be an instace of UInt8Array. 3. We now validate that the names of extension attributes are valid according to the cloud events spec and that the names of known attributes are not used as extension attributes. As part of this work a few more tests were written to confirm some of the stuff that we do when encoding events to send over the wire (like the above transformations, but also setting a default id and date).
ellismg
force-pushed
the
ellismg/event-grid-client
branch
from
5e1ebda to
d36e4f7
Compare
Co-authored-by: Laurent Mazuel <[email protected]>
|
@xirzec Can you take another look here? I responded to the feedback we had from the arch board meetings and finished all the TODO's and added some more tests. We think this is ready for preview1. |
xirzec
left a comment
xirzec
left a comment
There was a problem hiding this comment.
Looking good; I left some small comments.
It's too bad we don't have some way to namespace away all the system event clutter, but as we talked about before making an entire separate package feels like overkill.
I do quibble with the way most of the system events are defined though - seems like they all have every property as optional, meaning {} is assignable to any of them which feels pretty useless. Surely there are some required properties on these system events?
I'm also a little uncomfortable returning RestResponse as our default client operation return value, though maybe I'm thinking too much on it.
Instead, we return a custom interface that has the `_response` property (as is our current pattern). This will allow us to return additional information in the future if we want to, in a compatible way
This list matches our general guidence, from https://azure.github.io/azure-sdk/typescript_design.html#ts-platform-support
|
Responded to all your feedback, @xirzec, ready for another pass! |
xirzec
left a comment
xirzec
left a comment
There was a problem hiding this comment.
Left a few last remarks, thanks for the quick turnaround!
| - Browser JavaScript | ||
| - Apple Safari: latest two versions | ||
| - Google Chrome: latest two versions | ||
| - Microsoft Edge: all supported versions |
There was a problem hiding this comment.
might have to clarify if you mean Spartan or Chromium-based Edge
Split the input and output shapes for CloudEvent and EventGridEvent. On input, we don't require either `id` or `eventTime` for EventGridEvent or CloudEvent (since the client will use default values of a new UUID and the current time), where as the outputs correctly reflect what properties will always be set when consuming these events.
| [Package (NPM)](https://www.npmjs.com/package/@azure/eventgrid) | | ||
| [API reference documentation](https://aka.ms/azsdk-js-eventgrid-ref-docs) | | ||
| [Product documentation](https://docs.microsoft.com/en-us/azure/event-grid/) | | ||
| [Samples](https://github.com/Azure/azure-sdk-for-js/tree/master/sdk/eventgrid/eventgrid/samples) |
There was a problem hiding this comment.
@ellismg this directory does not exist and causing link verification in CI to fail.
There was a problem hiding this comment.
@ellismg this directory does not exist and causing link verification in CI to fail.
Huh! Indeed - I'm not sure where the samples are (I don't see them locally either, perhaps I missed git add'ing them at some point and they got blown away and I didn't notice.
Let me rewrite them and re-submit.
For my own edification, should this have been caught during the PR? All the checks had passed but I'm not sure if there's something else I should have been looking at.
There was a problem hiding this comment.
@ellismg Thanks! I think it should have been caught in the PR pipeline, so it is weird that it was all green. cc @praveenkuttappan @KarishmaGhiya.
6 participants
Initial commit of the Track 2 EventGrid client. Compared to the previous
client, there are a few new high level features:
schema.
There are still some //TODO's in the code, but should be reviewable as is. In case it will be helpful, I separated the "delete all the old code" and "implement the track 2 client" as separate commits, since there's no real relationship between the "new" and "old" files.