[Azure Search] Changing codgen strategy for enums and fixing Skill type #5531
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ructs This is in preparation for migrating to AutoRest's support for auto-generated extensible enums. They are structs, so we are aligning to that design ahead of time to ease the future transition.
This change is the result of correcting how we model extensible enums in the Swagger spec. Previously, they were modeled as objects marked with "x-ms-external": true, but this was not faithfully representing the wire format, because on the wire these enums are strings, not objects. Since we are not ready to fully adopt AutoRest's "extensible enums" feature, we still have our hand-written enums, which are now structs per the previous commit. This change adds PowerShell script to delete the newly auto-generated enums (which are not extensible yet), but keeps the other codgen changes that are necessary to start treating these as value types instead of reference types. The previous commit broke the tests due to differences in nullability; This commit fixes them. One breaking change caused by this commit is that the SynonymMap type no longer takes a SynonymMapFormat as a constructor parameter, and the Format property is now a static read-only string. This is per AutoRest's policy of "inlining" required enum parameters that have only a single possible value.
The Description, Context, Inputs, and Outputs properties of Skill were marked as required in the Swagger spec, despite the fact that the REST API accepts skills without these properties, and some of these properties are set to null in the examples. This was causing Swagger validation errors.
It turns out these properties are required by the REST API. This is a breaking change to all Skill constructors, but it's behind a version bump.
This change was driven by the need to update Field in the Swagger to accurately reflect the REST API. The eventual goal is to generate Field for SDKs in other languages. For .NET, this means replacing the fully customized Field class with a partially-generated implementation that is mostly backwards compatible.
We're about to add support for complex types to the SDK. This requires
modeling sub-fields of complex fields. There are two potential approaches for
this:
1. Inheritance. Field would become a base class, with LeafField and
ComplexField derived classes.
2. Re-use. Field remains a concrete class, and gains a new Fields property,
making it a recursive data type.
The way this is modeled in the REST API itself is much closer to 2 than 1.
Modeling 1 in Swagger isn't really possible without extending AutoRest's
x-ms-discriminator-value extension in unnatural ways. Rather than fight
against the nature of our API, we'll instead "go with the flow" and use
approach 2 above. This means many existing properties of Field won't be
meaningful for complex fields, and thus, must be nullable to indicate that
they do not apply.
arv100kri
approved these changes
Mar 25, 2019
shmed
approved these changes
Mar 25, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See individual commits for details.
Corresponding Swagger PR: Azure/azure-rest-api-specs#5410
FYI @Yahnoosh @arv100kri @mgottein