-
Notifications
You must be signed in to change notification settings - Fork 44
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 stack overflow when EnableLowerCamelCase is used (fixes #233) #234
Fix stack overflow when EnableLowerCamelCase is used (fixes #233) #234
Conversation
9f33d04
to
6686a5c
Compare
Two things I wasn't sure about:
|
DefaultExpansionsBuilder expands complex properties only, to prevent expanding navigation properties endlessly. But under EnableLowerCamelCase, it was failing to recognize navigation properties as such, due to a casing issue: GetNavigationProperties returned names as used in the EDM model (lowercase), and used it to filter a list of C# property names (uppercase). This fix works by looking up each navigation property's corresponding C# name. Another approach would be to use case-insensitive comparison, but this would not handle the scenario where EDM property names are customized.
In ExpressionBuilder tests, set the ODataQueryOptionParser to case-insensitive, since using EnableLowerCamelCase makes all of the model properties case-insensitive.
6686a5c
to
892c8ef
Compare
@dotnet-policy-service agree |
|
Thanks |
Thanks. One thing I forgot about: can you check the web tests? I'm not sure how to run those; they abort immediately when I attempt to launch them. |
To run the web tests.
|
All 69 tests in Web.Tests passed for master with my changes included. To get the tests to run, I did the following:
I tested using SQL Server 2022 CU17 (Linux, hosted in Docker). |
This fixes bug #233, which affects apps that use EnableLowerCamelCase. This regression was introduced in #229, due to the public EDM model using lowercase property names whereas the C# classes use capitalized names.
This fix works by looking up each navigation property's corresponding C# property name. This might not work if the EDM model was built by hand, meaning it omits annotations. If any app still has the stack overflow, we could try changing the ToHashSet calls to use
StringComparer.OrdinalIgnoreCase
as a fallback.This pull request also changes all of the tests to use
EnableLowerCamelCase
, though ideally we'd test both with and without this mode.