-
Notifications
You must be signed in to change notification settings - Fork 491
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
LINQ: Adds support for Nullable<T>.Value or Nullable<T>.HasValue when using camelCase serialization #2917
Conversation
I could use some assistance with direction on where I should add some unit tests for this. It was looking like perhaps LinqSQLTranslationBaselineTests, LinqTranslationBaselineTests would be the place for this, but I can't figure out how the tests are asserting the correct query output. |
@timsander1 or @khdang can you please take a look? |
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
@ssa3512 you can consider adding unit tests to CosmosItemLinqTests.cs. The LinqSQLTranslationBaselineTests, LinqTranslationBaselineTests tests are also the right place. They are baseline tests which produce xml files as output and they assert by comparing the output files with the existing baseline files in https://github.com/Azure/azure-cosmos-dotnet-v3/tree/master/Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/BaselineTest/TestBaseline |
/azp run |
Commenter does not have sufficient privileges for PR 2917 in repo Azure/azure-cosmos-dotnet-v3 |
/azp run |
@khdang I added tests to the CosmosItemLinqTests to verify the translation of HasValue to IS_DEFINED. Can you please review? |
Azure Pipelines successfully started running 2 pipeline(s). |
Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/CosmosItemLinqTests.cs
Outdated
Show resolved
Hide resolved
Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/CosmosItemLinqTests.cs
Outdated
Show resolved
Hide resolved
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
@j82w can you re-run the "Preview Tests Release" job? The test failure there is not occurring locally, and does not appear to be at all related to my changes. |
Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/CosmosItemLinqTests.cs
Outdated
Show resolved
Hide resolved
… HasValue/Value for every nullable
@@ -69,6 +74,7 @@ public TranslationContext(CosmosLinqSerializerOptions linqSerializerOptions, IDi | |||
{ | |||
this.linqSerializerOptions = linqSerializerOptions; | |||
this.parameters = parameters; | |||
this.memberNames = new MemberNames(linqSerializerOptions); | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@khdang any objections to consolidating these constructors into a single constructor? The first parameterless constructor is currently only called by the second constructor that takes the serializer options and the class is internal so there are no public API concerns. This will ensure that there is no path where memberNames
is not set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went ahead and did this in the latest round of changes. Please let me know if you have any issues.
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
Pull Request Template
Description
Uses the configured serializer settings when checking for the special cases of
Nullable<T>.Value
andNullable<T>.HasValue
This allows the use of .HasValue when using camelCase serialization, as it currently translates to
["propName"]["hasValue"]
in the resultant query instead of the intendedIS_DEFINED("propName")
Type of change
Closing issues
Closes #2916