-
Notifications
You must be signed in to change notification settings - Fork 357
Resolve issues with Deserialization of Untyped Types #3283
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
base: dev-9.x
Are you sure you want to change the base?
Conversation
/AzurePipelines run |
No pipelines are associated with this pull request. |
/AzurePipelines run |
No pipelines are associated with this pull request. |
/AzurePipelines run |
No pipelines are associated with this pull request. |
/AzurePipelines run |
No pipelines are associated with this pull request. |
payloadTypeReference, | ||
this.MessageReaderSettings.PrimitiveTypeResolver, | ||
this.MessageReaderSettings.ReadUntypedAsString, | ||
!this.MessageReaderSettings.ThrowIfTypeConflictsWithMetadata); |
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.
If it's for 9.x, do we need to keep back compatible?
...sts/Common/Microsoft.OData.E2E.TestCommon/Common/Server/UntypedTypes/UntypedTypesEdmModel.cs
Outdated
Show resolved
Hide resolved
@WanjohiSammy I’d like to highlight two specific comments from the thread on the issue you have referenced that are particularly relevant to this PR:
Regarding backward compatibility, we typically manage such changes using the To align with this approach, I suggest introducing a new enum member, e.g., Since Finally, please refer to how compatibility flags are evaluated and toggled across the library to ensure consistency with existing patterns. |
bool readUntypedAsString, | ||
bool generateTypeIfMissing) | ||
bool generateTypeIfMissing, | ||
bool readUntypedNumericAsDecimal = true) |
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.
Given that this method is internal, and we have updated the call points to pass the relevant value, do we need to set a default parameter value?
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.
Resolved
Just to confirm, if I create a new instance of |
Yes, this is correct, the new behavior will be applied. I have added tests: https://github.com/OData/odata.net/pull/3283/files#diff-503ffc0454f3d70fe4153d243d53eb35d79aaddfc1d9dcd09e742f4aa88d9317 |
ffc1bdd
to
991925b
Compare
991925b
to
2f26a40
Compare
2f26a40
to
a28eb6e
Compare
… ODataMessageReader
…odata.net into fix/resolve-untype-values
/AzurePipelines run |
No pipelines are associated with this pull request. |
Issues
This pull request fixes
Description
The change ensures that untyped values of various primitive types are correctly deserialized and materialized, supporting a broader range of types.
The previous implementation only handled
bool
,string
, and defaulted all other types todecimal
. This was insufficient for untyped properties that could be of unresolved types such asint, long, double, and collections
.This change introduces a new
boolean
flagPreserveUntypedNumericAsDecimal
to enhance control over how untyped numeric values are handled during JSON parsing.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.
Repository notes
Team members can start a CI build by adding a comment with the text
/AzurePipelines run
to a PR. A bot may respond indicating that there is no pipeline associated with the pull request. This can be ignored if the build is triggered.Team members should not trigger a build this way for pull requests coming from forked repositories. They should instead trigger the build manually by setting the "branch" to
refs/pull/{prId}/merge
where{prId}
is the ID of the PR.