Enable configuring dynamic behavior through client options #36887
Conversation
|
Need to incorporate comments from the original PR for this: #36868 |
|
API change check APIView has identified API level changes in this PR and created following API reviews. |
| public enum PropertyNamingConvention | ||
| { | ||
| None = 0, | ||
| CamelCase = 1, |
There was a problem hiding this comment.
I worry these names aren't clear enough. I thought this was how we're managing dynamic binding resolution (i.e., allowing pascal case), but it looks like we're also using this on RequestContent.Create to control how we serialize?
There was a problem hiding this comment.
For what it's worth, I still really like [Flags] public enum DynamicBindingFlags { None = 0, AllowPascalCase = 1, IgnoreCase = 2, IgnoreSeparators = 4, ... } as a separate concept to really hammer home the dynamic renaming half of the equation.
There was a problem hiding this comment.
We explored the flags enum approach, and it had a number of negatives. This is the approach I'd like to take because I think it sets us up best to evolve the type in response to customer feedback. It follows precedents set in a variety of other .NET APIs, for databases and serialization, so I have higher confidence that it will be both consistent and evolvable over other approaches we've considered.
...gnitivelanguage/Azure.AI.Language.Conversations/tests/ConversationAnalysisClientLiveTests.cs
Show resolved
Hide resolved
|
What happens when client options say one thing and ToDynamic says another? I assume the ToDynamic wins, but it might be good to describe it in the PR descriptions. |
The precedence rules are:
This is documented in the samples, but I'm happy to add it to any refdoc comments where you think it would be helpful. |
Property naming conventions for DynamicData
Previously, in order to write code that used dynamic content the same way as an Azure model type, users needed to pass a
DynamicCaseMappingenum value toToDynamicFromJson().This approach had a number of concerns, from the verbosity of the code that customers needed to write to achieve the intended usage of the type, to concerns about how applicable the enum was to other domains.
This new approach maintains the principle that changing the naming convention of used with dynamic content is 100% opt-in -- by default dynamic property names are mapped exactly to content property names. Following .NET precedent used in other APIs that do this type of case-mapping, it moves the opt-in to a one-time configuration parameter, so it doesn't propagate across every instance where dynamic content is used.
Summary of the API changes
DynamicCaseMappingis renamed toPropertyNamingConvention, with values that align with names from STJ JsonNamingPolicy. The intention is that we can evolve this to use other policies provided in STJ in the future, based on customer need.PropertyNamingConventionis moved toAzure.Core.Serialization. With this shift, it can be used to createRequestContentand in other serialization domains, e.g. to replace the PropertyNamingPolicy enum on CosmosSerializationOptions in their Track 2 library.ProtocolOptionsis added toClientOptions-- callers set the naming convention to be used with response content on the client -- this scopes the convention to a given service. So, for example, one service can use camel case and another can use snake case, and it will all function correctly when users write code substitutable for code written at the convenience layer, without needing to pass parameters toToDynamicFromJson().Approaches considered
We spent a lot of time exploring a number of different approaches to solving this problem before arriving at this solution. For completeness, here is the list of explorations done:
Adresses #36642