[Azure Search] FieldBuilder improvements #6833
Merged
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
added 11 commits
July 5, 2019 19:08
The logic for detecting different kinds of data types will get more complex in the future. To prepare, this change replaces the DataTypeInfo struct with the C# equivalent of a "sum type" (a.k.a. "discriminated union"), implemented via the IDataTypeInfo interface and its companion classes. This will allow modeling of more mutually-exclusive cases for data types that may carry different properties. This change also necessitated removing one of the "continue" statements from the inner loop. It belongs to a case that only applies to complex fields, so logically the "continue" belongs in one of the lambdas passed to Match, but since that's not possible, CreateComplexField just returns null for that case instead.
…pipeline This change cleans up the core logic of FieldBuilder, removing the last "continue" and making it easier to reason about and extend.
We can't just assume that properties of types we don't recognize must be complex types. Instead, FieldBuilder now uses the JSON contract resolver to determine whether a property is of a complex type. If not, it throws an exception with a helpful error message. Rather than repeatedly call the contract resolver (once at the beginning of BuildForTypeRecursive and once in GetDataTypeInfo), DataTypeInfo.Complex has been extended to carry the JsonObjectContract for the type as a performance optimization. The signature of BuildForTypeRecursive has been modified accordingly so it's now the caller's responsibility to provide the JsonObjectContract.
Now FieldBuilder will fail with a helpful error message if a type that clearly isn't a DTO is passed. This is determined by whether or not the type resolves to a JsonObjectContract.
Callbacks make tests harder to reason about. They were there primarily to support calling FieldBuilder with and without a custom contract resolver. Instead, a flag is now passed to each test to indicate whether to pass a contract resolver to FieldBuilder. Also, the tests now more explicitly call FieldBuilder and then assert on the result using a new helper class, FieldMap. This makes the data flow in the tests more obvious and more closely follows the arrange-act-assert pattern.
Sometimes model classes have properties of types (like enums) that make sense for application code, but don't automatically map to an Azure Search data type. Using [JsonIgnore] is not an option in scenarios when such properties actually are a part of the index. This commit introduces [FieldBuilderIgnore] which FieldBuilder treats just like [JsonIgnore]. That way, you can opt out of FieldBuilder without opting out of JSON serialization/deserialization.
brjohnstmsft
requested review from
weshaggard,
ishansrivastava90,
updixit,
natinimni,
shmed and
arv100kri
|
I highly recommend reviewing commit-by-commit |
3 tasks
brjohnstmsft
commented
Jul 11, 2019
...earch/Microsoft.Azure.Search.Service/src/Customizations/Indexes/FieldBuilder/FieldBuilder.cs
Outdated
Show resolved
Hide resolved
brjohnstmsft
commented
Jul 12, 2019
.../Microsoft.Azure.Search.Service/src/Customizations/Indexes/FieldBuilder/AnalyzerAttribute.cs
Show resolved
Hide resolved
brjohnstmsft
changed the title
|
@weshaggard Please merge at your convenience. Thanks! |
weshaggard
reviewed
Jul 12, 2019
sdk/search/Microsoft.Azure.Search.Data/src/Microsoft.Azure.Search.Data.csproj
Show resolved
Hide resolved
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.
Fixes #6380
FieldBuilderis the class that generates an index schema (list of fields) from a CLR type. Adding support for complex types introduced some regressions, captured in the issue above. This PR addresses those regressions and makes some improvements based on feedback:FieldBuildernow fails gracefully with helpful error messages when passed a type that it can't process. For example, passing a type that isn't a DTO toBuildForType, or passing a DTO where one of the properties is an enum.[Key]attribute is now ignored when found on a sub-field. This is a valid scenario when re-using a model type that maps to one index as a complex type in a different index.[FieldBuilderIgnore]attribute that you can use instead of[JsonIgnore]in situations whereFieldBuildercan't generate aFieldfrom a property, but you still want the property serialized and included in the index (e.g. -- enum properties that correspond to string fields in the index). In this scenario, you need to manually createFieldobjects for the ignored properties and add them to the list of fields returned byFieldBuilder.FieldBuildercorrectly mapsICollectionproperties to collection-typed fields in the index definition. No code changes were required to make these tests pass.FieldBuilderTeststo make them easier to understand and maintain.FieldBuilder-relatedAttributeclasses.FYI @weshaggard for project file changes based on earlier PR feedback. Please don't merge until at least one person from my team approves.
FYI @ishansrivastava90 @arv100kri @natinimni @shmed @updixit to review the functional changes.