Skip to content

Fix HasJsonPropertyName for complex properties #37019

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.

Sign up for GitHub

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

Jump to bottom
Merged
AndriySvyryd merged 4 commits into from
Oct 24, 2025
Merged

Fix HasJsonPropertyName for complex properties #37019

AndriySvyryd merged 4 commits into from
Oct 24, 2025

Conversation

Copy link
Contributor

Copilot AI commented Oct 23, 2025 •
edited
Loading

Fixes #37009

Fixes HasJsonPropertyName() configuration on complex properties and complex collections within JSON columns by storing the annotation on the complex type instead of the complex property.

Changes

  • Added SetJsonPropertyName() extension methods to IMutableTypeBase and IConventionTypeBase
  • Updated ComplexProperty and EntityType extension methods to delegate to the new TypeBase methods
  • Modified TypeBase.GetJsonPropertyName() to return null for root JSON types (those with ContainerColumnName)
  • Updated AnnotationCodeGenerator to generate HasJsonPropertyName() calls from complex type annotations
  • Added test coverage and assertions to verify the fix
Original prompt

Create a PR that fixes @dotnet/efcore/issues/37009 by changing SetJsonPropertyName and GetJsonPropertyName on complex property to call the corresponding methods on the complex type. Assert in ComplexCollection_can_have_nested_complex_properties_mapped_to_json that no complex properties have RelationalAnnotationNames.JsonPropertyName annotation. Add the test from @dotnet/efcore/pull/37018 to verify the query implementation.

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

@AndriySvyryd
Fix HasJsonPropertyName for complex JSON properties
56662a3 
- Updated RelationalComplexPropertyExtensions to delegate Get/SetJsonPropertyName to ComplexType
- Updated RelationalTypeBaseExtensions.GetJsonPropertyName to return null for root JSON types (those with ContainerColumnName)
- Updated query pipeline files to call GetJsonPropertyName on complex property instead of complex type
- Added assertion in ComplexCollection_can_have_nested_complex_properties_mapped_to_json test
- Added HasJsonPropertyName test from PR #37018

Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix SetJsonPropertyName and GetJsonPropertyName for complex properties Fix HasJsonPropertyName for complex JSON properties Oct 23, 2025
Copilot AI requested a review from AndriySvyryd October 23, 2025 19:27
AndriySvyryd
AndriySvyryd reviewed Oct 23, 2025
View reviewed changes
Copy link
Member

@AndriySvyryd AndriySvyryd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot Update Complex_types_mapped_to_json_are_stored_in_snapshot test

@AndriySvyryd
Address PR feedback
5af3a9c 
- Reverted query pipeline changes (SelectExpression, JsonQueryExpression, ShaperProcessingExpressionVisitor, SqliteQueryable, SqlServerQueryable)
- Removed casts in RelationalComplexPropertyExtensions by calling methods directly on ComplexType
- Moved ContainerColumnName check earlier in GetJsonPropertyName and returned null if annotation value is null
- Moved SetJsonPropertyName to IMutableTypeBase and IConventionTypeBase
- Made EntityType and ComplexProperty SetJsonPropertyName methods delegate to TypeBase methods
- Updated AnnotationCodeGenerator to read JsonPropertyName from complex type instead of complex property

Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Oct 23, 2025

@copilot Update Complex_types_mapped_to_json_are_stored_in_snapshot test

Updated the AnnotationCodeGenerator to read JsonPropertyName from complex type instead of complex property annotations. The test should now pass. (5af3a9c)

Copilot AI requested a review from AndriySvyryd October 23, 2025 22:23
AndriySvyryd
AndriySvyryd reviewed Oct 23, 2025
View reviewed changes
@AndriySvyryd
Address additional PR feedback
d447ce4 
- Moved JsonPropertyName logic from GenerateFluentApiCalls(IComplexProperty) to GenerateFluentApiCalls(IComplexType)
- Fixed infinite recursion by upcasting to IConventionTypeBase/IMutableTypeBase in EntityType methods
- Simplified null check in GetJsonPropertyName to check annotation != null
- Combined ContainerColumnName check with IsMappedToJson check using OR operator

Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
@AndriySvyryd
Copy link
Member

AndriySvyryd commented Oct 23, 2025

@copilot Update progress by creating a concise description for this PR

Copilot AI requested a review from AndriySvyryd October 23, 2025 23:06
Copy link
Contributor Author

Copilot AI commented Oct 23, 2025

@copilot Update progress by creating a concise description for this PR

Updated the PR description to be more concise. The new description focuses on the core fix (storing annotation on complex type instead of property) and summarizes the key changes made.

Copilot AI mentioned this pull request Oct 23, 2025
Merged
Copilot AI added a commit that referenced this pull request Oct 23, 2025
@AndriySvyryd
Port PR #37019 to release/10.0 with compatibility switch
4cd578f 
Co-authored-by: AndriySvyryd <6539701+AndriySvyryd@users.noreply.github.com>
@AndriySvyryd AndriySvyryd marked this pull request as ready for review October 24, 2025 00:17
@AndriySvyryd AndriySvyryd requested a review from a team as a code owner October 24, 2025 00:17
@AndriySvyryd AndriySvyryd changed the title Fix HasJsonPropertyName for complex JSON properties Fix HasJsonPropertyName for complex properties Oct 24, 2025
roji
roji approved these changes Oct 24, 2025
View reviewed changes
Copy link
Member

@roji roji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed offline, to me it's more logical to configure the JSON property name on the property - in my mind the JSON name isn't part of the object, but rather of the association pointing to it. I understand this isn't consistent with how owned entities currently work, but if anything I'd change it the other way instead (maybe I'm missing the larger picture though).

But this is all quite internal in any case, and we can re-discuss in the future.

@roji
Copy link
Member

roji commented Oct 24, 2025

/azp run

@azure-pipelines
Copy link

azure-pipelines bot commented Oct 24, 2025

Azure Pipelines successfully started running 1 pipeline(s).

@roji roji mentioned this pull request Oct 24, 2025
Closed
@AndriySvyryd
Copy link
Member

AndriySvyryd commented Oct 24, 2025

I agree that semantically it would make more sense to be on the navigation/property, but I think we chose to store it on the type because it was easier to lookup. I don't think it matters too much one way or the other as long as we are consistent

@AndriySvyryd AndriySvyryd merged commit ab1973d into main Oct 24, 2025
9 checks passed
@AndriySvyryd AndriySvyryd deleted the copilot/fix-json-property-names branch October 24, 2025 14:05
@dotnet-maestro dotnet-maestro bot mentioned this pull request Oct 25, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@roji roji roji approved these changes

@AndriySvyryd AndriySvyryd Awaiting requested review from AndriySvyryd

Assignees

@AndriySvyryd AndriySvyryd

Copilot code review Copilot

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

JSON property name is ignored in queries involving complex JSON types

3 participants

@AndriySvyryd @roji