Skip to content
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

Conversation

joeyadams-bec
Copy link
Contributor

@joeyadams-bec joeyadams-bec commented Jan 19, 2025

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.

@joeyadams-bec joeyadams-bec force-pushed the fix-stack-overflow-EnableLowerCamelCase branch from 9f33d04 to 6686a5c Compare January 19, 2025 23:34
@joeyadams-bec joeyadams-bec changed the title Fix stack overflow enable lower camel case (fixes #233) Fix stack overflow when EnableLowerCamelCase is used (fixes #233) Jan 19, 2025
@joeyadams-bec
Copy link
Contributor Author

Two things I wasn't sure about:

  1. Should we also use case-insensitive property comparison here? I chose to leave it out for now, and wait to see if someone else reports a stack overflow under the current fix.
  2. I changed all of the tests to use EnableLowerCamelCase, but I'm not sure if we want that or not. I suppose a better approach would be to have the test helpers run through multiple ways the OData EDM model can be configured (with/without custom namespace, lowercase or not, etc.), but that would take a lot of refactoring.

Joey Adams added 2 commits January 19, 2025 21:14
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.
@joeyadams-bec joeyadams-bec force-pushed the fix-stack-overflow-EnableLowerCamelCase branch from 6686a5c to 892c8ef Compare January 20, 2025 02:15
@joeyadams-bec
Copy link
Contributor Author

@dotnet-policy-service agree

@BlaiseD
Copy link
Member

BlaiseD commented Jan 20, 2025

Two things I wasn't sure about:

  1. Should we also use case-insensitive property comparison here? I chose to leave it out for now, and wait to see if someone else reports a stack overflow under the current fix.
  2. I changed all of the tests to use EnableLowerCamelCase, but I'm not sure if we want that or not. I suppose a better approach would be to have the test helpers run through multiple ways the OData EDM model can be configured (with/without custom namespace, lowercase or not, etc.), but that would take a lot of refactoring.
  1. Agree. 2. Usually create an override like we've done here and let them both run - not a big deal for this case.

@BlaiseD BlaiseD merged commit 66dba4d into AutoMapper:master Jan 20, 2025
3 checks passed
@BlaiseD
Copy link
Member

BlaiseD commented Jan 20, 2025

Thanks

@joeyadams-bec
Copy link
Contributor Author

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.

@BlaiseD
Copy link
Member

BlaiseD commented Jan 26, 2025

To run the web tests.

  • Open the configuration manager and check the Build checkboxes for the web tests and the sample web projects (3 total).
  • Start the web projects - they should start with the same ports listed in the inline data on the web tests.
  • Then run the web tests.

@joeyadams-bec
Copy link
Contributor Author

All 69 tests in Web.Tests passed for master with my changes included.

To get the tests to run, I did the following:

  • In Configuration Manager, set all projects to build (WebAPI.OData.EFCore, WebAPI.OData.EF6, and Web.Tests were unchecked initially).
  • Change the connection strings from Server=.\SQL2016;Database=YourDB to point to my SQL Server.
  • Run MigrationTool and SeedDatabase with dotnet run.
  • In Configure Startup Projects, choose "Multiple startup projects", and configure WebAPI.OData.EFCore and WebAPI.OData.EF6 to start.

I tested using SQL Server 2022 CU17 (Linux, hosted in Docker).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants