Fix assignment of CollectionExpandedProperty.TotalCount when using EF #1693
Conversation
|
|
KanishManuja-MS
added
the
Ready for review
| Assert.Contains("\"[email protected]\":9", payload); // Categories | ||
| } | ||
|
|
||
| private Task<HttpResponseMessage> GetResponse(string uri, string acceptHeader, bool handleNullPropagation = true) |
There was a problem hiding this comment.
bool handleNullPropagation = true [](start = 87, length = 33)
Please add variations to the test for handle null propagation equal to both true and false (i.e., turn this method into [Theory] with [InlineData] for handleNullPropagation = true and handleNullPropagation=false). Better yet, make all of the tests that assume handleNullPropagation=false into [Theory] that run with handleNullPropagation equal to both true and false.
There was a problem hiding this comment.
removed the redundant argument - HandleNullPropagationOption is set on controller action level anyway
do not quite understand the point: SelectExpand_WithOneLevelNestedDollarCount_HandleNullPropagationFalse_Works is already a variation on test SelectExpand_WithOneLevelNestedDollarCount_Works which tests the same thing (count field for a nested collection) but with the default HandleNullPropagationOption value of True, which depends on the namespace of query provider (class HandleNullPropagationOptionHelper, method GetDefaultHandleNullPropagationOption)
There was a problem hiding this comment.
Right. My preference would be to have unit test that explicitly tests both the null propagation true case as well as the null propagation false case. I realize that the existing test on ln 38 doesn't specify null propagation, which picks up the default, which for the current query provider happens to be null propagation true, but that's not explicit in the test so a future change to that test, such as using a different query provider, could inadvertently invalidate the intention that we have coverage for both null propagation false and null propagation true.
It's not a matter of making sure that this fix works and doesn't break existing tests (which the PR currently validates), but making sure we continue to have coverage and don't inadvertantly invalidate assumptions as the tests and code evolve.
One way to address this would be to specifically set HandleNullPropagation to OData.Query.HandleNullPropagationOption.True in the MsCustomersController, so that we are explicit that we are testing a handle null propagation value of true, rather than relying on a query provider-specific default value.
In reply to: 300698007 [](ancestors = 300698007)
| @@ -118,7 +118,7 @@ private static Expression CreateNamedPropertyCreationExpression(NamedPropertyExp | |||
|
|
|||
| if (property.CountOption != null && property.CountOption.Value) | |||
| { | |||
| memberBindings.Add(Expression.Bind(namedPropertyType.GetProperty("TotalCount"), property.TotalCount)); | |||
| memberBindings.Add(Expression.Bind(namedPropertyType.GetProperty("TotalCount"), ExpressionHelpers.ToNullable(property.TotalCount))); | |||
There was a problem hiding this comment.
ExpressionHelpers.ToNullable [](start = 100, length = 28)
Should this be dependent upon whether or not null propagation handling is true or false? i.e., should we only call ExpressionHelpers.ToNullable if null propagation handling is true? #Closed
There was a problem hiding this comment.
it has been a while and had almost forgotten about this
from what i can recall, the property that is being set is of type long? (class CollectionExpandedProperty? ), no matter what the settings are, and for assignment to work correctly, the assigned expression should be converted to nullable anyway
i don't remember if the expression type was correct in HandleNullPropagationOption.True case, but ExpressionHelpers.ToNullable does not do anything if the expression is of nullable type already
i don't see a straightforward way of getting that setting value inside of the PropertyContainer class anyway so imho this should stay as is #Resolved
|
@gunars-skangals - thanks for the pull request to fix this issue! See my comments inline. Should we only call ExpressionHelpers.ToNullable if null propagation handling is true? If you can follow up, I'd love to get this fix integrated into our next WebAPI OData build. Thanks! #Closed |
Issues
This pull request fixes issue #1565
Description
As the expression contained in NamedPropertyExpression.TotalCount can be either Int64 or Nullable depending on ODataQuerySettings.HandleNullPropagation, cast result of expression to the nullable type before assigning, otherwise the property is set to some random number.
Checklist (Uncheck if it is not completed)