Skip to content

Fix null property name handling logic in SystemTextJsonValidationMetadataProvider #47775

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
captainsafia merged 6 commits into from
Apr 24, 2023
Merged

Fix null property name handling logic in SystemTextJsonValidationMetadataProvider #47775

captainsafia merged 6 commits into from
Apr 24, 2023

Conversation

@eiriktsarpalis
Copy link
Member

@eiriktsarpalis eiriktsarpalis commented Apr 19, 2023 •
edited
Loading

Fixing what appears to be a negation bug causing null names to be passed to the underlying JsonNamingPolicy.

cf. dotnet/runtime#85002 (comment)

Fix #47835

@eiriktsarpalis
Fix https://github.com/dotnet/runtime/issues/84830
761ce21 
Fixing what appears to be a negation bug causing null names to be passed to the underlying `JsonNamingPolicy`.
@eiriktsarpalis eiriktsarpalis requested a review from a team as a code owner April 19, 2023 13:28
@ghost ghost added old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels Apr 19, 2023
@eiriktsarpalis eiriktsarpalis changed the title Fix https://github.com/dotnet/runtime/issues/84830 Fix a negation bug in SystemTextJsonValidationMetadataProvider Apr 19, 2023
@eiriktsarpalis eiriktsarpalis mentioned this pull request Apr 19, 2023
Merged
eerhardt
eerhardt reviewed Apr 19, 2023
View reviewed changes
src/Mvc/Mvc.Core/src/ModelBinding/Metadata/SystemTextJsonValidationMetadataProvider.cs Outdated
var propertyName = ReadPropertyNameFrom(context.Attributes);

if (string.IsNullOrEmpty(propertyName))
if (!string.IsNullOrEmpty(propertyName))
Copy link
Member

@eerhardt eerhardt Apr 19, 2023

Choose a reason for hiding this comment

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

I believe the existing code is correct. "If propertyName isn't set, read it from the context.Key.Name".

However, the bug here appears to be the null suppression on line 60:

_jsonNamingPolicy.ConvertName(context.Key.Name!)

If context.Key.Name can also be null, then I think we have a problem.

Copy link
Member Author

@eiriktsarpalis eiriktsarpalis Apr 19, 2023

Choose a reason for hiding this comment

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

Good call, I misinterpreted the code as running the naming policy against propertyName. In that case I'll close the PR and transfer https://github.com/dotnet/runtime/issues/84830 to aspnetcore so that you folks can make the appropriate fix.

Copy link
Member Author

@eiriktsarpalis eiriktsarpalis Apr 19, 2023

Choose a reason for hiding this comment

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

I ended up updating the PR so that it includes a null check for context.Key.Name. That should emulate the semantics of JsonNamingPolicy.CamelCase for other naming policies.

@eiriktsarpalis eiriktsarpalis changed the title Fix a negation bug in SystemTextJsonValidationMetadataProvider Fix null property name handling logic in SystemTextJsonValidationMetadataProvider Apr 19, 2023
captainsafia
captainsafia reviewed Apr 19, 2023
View reviewed changes
Copy link
Member

@captainsafia captainsafia left a comment

Choose a reason for hiding this comment

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

Can we add a test for this in https://github.com/dotnet/aspnetcore/blob/main/src/Mvc/Mvc.Core/test/ModelBinding/Metadata/SystemTextJsonValidationMetadataProviderTest.cs?

I think it should work if we use the following key in the test cases:

var key = ModelMetadataIdentity.ForType(typeof(SampleTestClass));

@captainsafia
Copy link
Member

captainsafia commented Apr 24, 2023

@eerhardt I added the test I recommended in #47775 (review). Can you review both changes?

Also, I observed something interesting which is that the bug only applies to scenarios that use the JsonSeperatorNamingPolicy. Things that use JsonCamelCasePolicy don't have the same bug because of this https://github.com/dotnet/runtime/blob/main/src/libraries/System.Text.Json/Common/JsonCamelCaseNamingPolicy.cs#L10-L13.

I was curious as to why this happens but I suppose it makes sense because you don't need to do any seperator-based concatenation for other policies?

eerhardt
eerhardt approved these changes Apr 24, 2023
View reviewed changes
Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

Thanks!

@eerhardt
Copy link
Member

eerhardt commented Apr 24, 2023

Also, I observed something interesting which is that the bug only applies to scenarios that use the JsonSeperatorNamingPolicy. Things that use JsonCamelCasePolicy don't have the same bug because of this https://github.com/dotnet/runtime/blob/main/src/libraries/System.Text.Json/Common/JsonCamelCaseNamingPolicy.cs#L10-L13.

I was curious as to why this happens but I suppose it makes sense because you don't need to do any seperator-based concatenation for other policies?

This is a (now) known inconsistency between the policies. See the conversation at dotnet/runtime#85002 (comment).

@captainsafia captainsafia enabled auto-merge (squash) April 24, 2023 18:57
@captainsafia captainsafia merged commit f1590bf into main Apr 24, 2023
@captainsafia captainsafia deleted the eiriktsarpalis-patch-1 branch April 24, 2023 19:53
@ghost ghost added this to the 8.0-preview4 milestone Apr 24, 2023
@danmoseley danmoseley removed the old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels label Jul 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@captainsafia captainsafia captainsafia left review comments

@eerhardt eerhardt eerhardt approved these changes

Assignees

No one assigned

Labels

area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates

Projects

None yet

Milestone

8.0-preview4

Development

Successfully merging this pull request may close these issues.

SystemTextJsonValidationMetadataProvider passes null values to the JsonNamingPolicy

5 participants

@eiriktsarpalis @captainsafia @eerhardt @danmoseley