Add a new base type EventGridEventData for all system event data types #3309
Conversation
…that it is easy to programmatically determine which types in this namespace correspond to eventdata.
Automation for azure-sdk-for-javaNothing to generate for azure-sdk-for-java |
Automation for azure-sdk-for-pythonA PR has been created for you: |
Automation for azure-sdk-for-rubyEncountered a Subprocess error: (azure-sdk-for-ruby)
Command: ['/usr/local/bin/autorest', '/tmp/tmp4wcrfazu/rest/specification/eventgrid/data-plane/readme.md', '--multiapi', '--ruby', '--ruby-sdks-folder=/tmp/tmp4wcrfazu/sdk', '[email protected]/[email protected]', '--version=preview'] AutoRest code generation utility [version: 2.0.4280; node: v7.10.1]
(C) 2018 Microsoft Corporation.
https://aka.ms/autorest
Loading AutoRest core '/root/.autorest/@[email protected]/node_modules/@microsoft.azure/autorest-core/dist' (2.0.4280)
Loading AutoRest extension '@microsoft.azure/autorest.ruby' (3.0.20->3.0.20)
Loading AutoRest extension '@microsoft.azure/autorest.modeler' (2.1.22->2.1.22)
Processing batch task - {"tag":"package-2018-01"} .
FATAL: The schema's 'StorageBlobCreatedEventData' ancestors should have at lease one property
FATAL: AutoRest.Core.Logging.CodeGenerationException: The schema's 'StorageBlobCreatedEventData' ancestors should have at lease one property
at AutoRest.Modeler.SwaggerModeler.BuildCompositeTypes() in C:\Users\ci\AppData\Local\Temp\PUBLISHv6236\20_20171009T223650\autorest.modeler\src\SwaggerModeler.cs:line 312
at AutoRest.Modeler.SwaggerModeler.Build(ServiceDefinition serviceDefinition) in C:\Users\ci\AppData\Local\Temp\PUBLISHv6236\20_20171009T223650\autorest.modeler\src\SwaggerModeler.cs:line 69
at AutoRest.Modeler.Program.<ProcessInternal>d__2.MoveNext() in C:\Users\ci\AppData\Local\Temp\PUBLISHv6236\20_20171009T223650\autorest.modeler\src\Program.cs:line 53
--- End of stack trace from previous location where exception was thrown ---
at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
at NewPlugin.<Process>d__15.MoveNext()
FATAL: ruby/modeler - FAILED
FATAL: Error: Plugin modeler reported failure.
Process() cancelled due to exception : Plugin modeler reported failure.
Failure during batch task - {"tag":"package-2018-01"} -- Error: Plugin modeler reported failure..
Error: Plugin modeler reported failure. |
Automation for azure-sdk-for-nodeA PR has been created for you based on this PR content. Once this PR will be merged, content will be added to your service PR: |
Automation for azure-sdk-for-goA PR has been created for you: |
|
cc @marstr |
| @@ -119,9 +119,19 @@ | |||
| } | |||
| } | |||
| }, | |||
| "EventGridEventData": { | |||
There was a problem hiding this comment.
What is the intent with this type? From the description it sounds like it's intended to be polymorphic? If that's the case have you looked into using discriminated types?
There was a problem hiding this comment.
The idea behind the new EventGridEventData type is just to serve as a common base across all the first party eventdata types. More info on how I arrived at this is in approach 2 below.
The existing EventGridEvent type contains the common properties of an event (id, eventType, eventTime etc.) + a "data" portion which is of type object. With this, there are two possible approaches:
Approach 1. [Evaluated but not used]: Specify the eventType property in EventGridEvent as the discriminator (e.g. "Microsoft.Storage.BlobCreated" -> StorageBlobCreatedEvent, "Microsoft.Storage.BlobDeleted" -> StorageBlobDeletedEvent. Remove the "Data" property from the EventGridEvent type and have new types such as StorageBlobCreatedEvent which contain a property called Data which is of the appropriate type such as StorageBlobCreatedEventData. However, in this approach, handling for 3rd party types is not as seamless: if have a 3rd party application publishes an event with eventType "Contoso.Items.ItemReceived", they will have to have special attributes/annotations for their custom type in order to fit into this discriminator model. Further, for 3rd party events, we also support other types for the data (strings/numeric/arrays etc.), which doesn't work well with this approach. In addition, we have already GA'ed with the approach where EventGridEvent contains a Data property, so I guess it will also be a breaking change to remove it now (in order to add it to the inherited types).
Approach 2 (Current approach): The "Data" property is part of the EventGridEvent type itself and it is of type object. The consuming code calls an EventGrid SDK function to deserialize EventGridEvents. This SDK function will internally maintain a mapping of eventType string -> eventData type and will use this mapping to look at the eventType property in the event(e.g. "Microsoft.Storage.BlobCreated") and will deserialize the Data property into the right data type such as StorageBlobCreatedEventData. The same SDK function will also support adding custom 3rd party mappings (e.g. "Contoso.Items.ItemReceived" -> typeof(ContosoItemReceivedEventData)). While creating this SDK function, I wanted to have a unit test to verify that for every system eventData type, there exists a corresponding mapping in this internal dictionary. However, without a common base class such as this newly introduced EventGridEventData, there's no way to programmatically determine which are all the eventData classes. This is the intent behind adding this new type.
Please let me know you have questions/comments.
There was a problem hiding this comment.
So if I understand correctly, the fact that some type derives from EventGridEventData indicates means that it's an eventData class correct (presumably you use reflection to make this determination)? It seems like a very C#-specific implementation. E.g. for Go this just generates a useless type that pollutes the package namespace (see this). I don't know how this would work in other languages.
There was a problem hiding this comment.
Yes, the fact that a type derives from EventGridEventData indicates means that it's an eventData class. For Go, if this type is not useful, can we customize the code generation to not generate it (e.g. through the --generate-empty-classes option)? For Java, Python, and C# this would show up as a base class.
There was a problem hiding this comment.
Presumably there's a way to omit it but ideally we should have a solution that works for all languages. How do you plan on supporting this in Go?
There was a problem hiding this comment.
What code will be used to programmatically determine the eventData classes, is this something your team will provide? Do you have a sample you can share that consumes this new functionality?
There was a problem hiding this comment.
I think we can close this for now and bring it back if/when there are additional product scenarios.
Currently, it is only unit test code for a SDK function to verify that there exists a mapping for each such eventData type (for which it uses reflection to determine the count of these classes) - the idea was to catch bugs whereby somebody introduces a new eventData type but forgets to add it to the mapping dictionary.
This PR introduces a new base type EventGridEventData for all system event data types. Currently, there's no common base type across EventGrid event data across different publishers (e.g. Storage / MediaServices / IoTHub / ARM etc.). We now define this base type called EventGridEventData and use the allOf semantics to tie the different event data types together. This should be a non-breaking change (verified for C# and Python) as the newly base type doesn't have any properties. Also, added the generate-empty-classes to true in the global configuration.
PR information
api-versionin the path should match theapi-versionin the spec).Quality of Swagger