-
Notifications
You must be signed in to change notification settings - Fork 161
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
Potential fix for issue #616 #947
Conversation
@microsoft-github-policy-service agree |
@xuzhg did you have a chance to take a look at this PR ? Thanks |
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.
Add tests
Hi @KenitoInc , |
@KenitoInc , it was not as much a hassle as I thought :) . Hope this new commit fulfils your requirements |
test/Microsoft.AspNetCore.OData.Tests/Query/Query/DefaultSkipTokenHandlerTests.cs
Outdated
Show resolved
Hide resolved
…` is used in query
src/Microsoft.AspNetCore.OData/Query/Query/DefaultSkipTokenHandler.cs
Outdated
Show resolved
Hide resolved
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.
Thanks @orty for your contribution
You're welcome ! @KenitoInc, do you agree with the overall changes ? |
@orty Thanks for this PR |
Based on issue #616 which I also encountered, I would like to propose this fix.
As I am not completely aware of side-effects that using only
edmProperty.Name
could have, I relied on a safe way to rollback to the EDM property name instead of the CLR one if the latter is not found.https://github.com/OData/AspNetCoreOData/blob/10a246671b6312fd234151c47cc91787da5cb171/src/Microsoft.AspNetCore.OData/Query/Query/DefaultSkipTokenHandler.cs#LL136C1-L145C18
I tested this change with my current project, but I suspect that some unit tests might have to be "doubled" with a
builder.EnableLowerCamelCase()
scenario to check for regressions on property retrieval when using a lowercased convention.edit: Also tested along with the
[DataContract]
and[DataMember]
attributes which create aliases for properties.