-
Notifications
You must be signed in to change notification settings - Fork 352
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
Fix bug in LoadProperty for complex and complex collection #2487
Fix bug in LoadProperty for complex and complex collection #2487
Conversation
@@ -99,12 +102,11 @@ protected override bool ReadImplementation() | |||
this.MaterializeEntryPlan, | |||
this.responseInfo.IsContinuation); | |||
} | |||
else | |||
else if (property.EdmProperty.Type.TypeKind() == EdmTypeKind.Entity) |
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.
In which block do complex properties and/or complex collection properties get handled?
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.
@habbes This block
else if (property.IsPrimitiveOrEnumOrComplexCollection) |
cfd2a2f
to
2597166
Compare
@@ -81,6 +81,7 @@ | |||
<Compile Include="Tests\Materialization\CollectionValueMaterializationPolicyTests.cs" /> | |||
<Compile Include="Tests\Materialization\EntryValueMaterializationPolicyUnitTests.cs" /> | |||
<Compile Include="Tests\Materialization\CamelCasedTypeMaterializationTests.cs" /> | |||
<Compile Include="Tests\LoadPropertyTests.cs" /> |
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.
shall we put the new test file under "Materialization" subfolder?
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.
@xuzhg Done
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
Issues
This pull request fixes #2471.
Description
We rely on
ODataLoadNavigationPropertyMaterializer
to materialize load property result for complex and complex collection property. Complex and complex collection properties are structural properties, the reliance onODataLoadNavigationPropertyMaterializer
is by design - we useODataReaderEntityMaterializer
as well to materialize complex and complex collection properties, since just like entities, they're returned asODataResource
.The bug was in how we apply the materialized values to the respective properties on the client type instance.
Due to a missing condition check, the complex collections were being treated as if they are a single entity value.
I fixed the bug by applying the missing check. By doing so, this block in
LoadPropertyResult
class does the job of applying the complex collection to the property - after first clearing the existing items.Checklist (Uncheck if it is not completed)
Additional work necessary
If documentation update is needed, please add "Docs Needed" label to the issue and provide details about the required document change in the issue.