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

Prevent runtime prop metadata retrieval when [JsonIgnore] is used #60024

Merged
merged 4 commits into from
Oct 12, 2021

Conversation

layomia
Copy link
Contributor

@layomia layomia commented Oct 5, 2021

Fixes #59936, a 6.0 regression. Contributes to #59364.

@ghost
Copy link

ghost commented Oct 5, 2021

Tagging subscribers to this area: @dotnet/area-system-text-json
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #59936, a 6.0 regression. Contributes to #59364.

Author: layomia
Assignees: -
Labels:

area-System.Text.Json

Milestone: 6.0.0

@layomia layomia requested a review from eiriktsarpalis October 6, 2021 20:23
@layomia layomia force-pushed the JsonIgnore branch 2 times, most recently from c43b151 to ffb733f Compare October 6, 2021 20:39
@layomia layomia force-pushed the JsonIgnore branch 5 times, most recently from d901f29 to 6112780 Compare October 11, 2021 17:26
@layomia layomia force-pushed the JsonIgnore branch 2 times, most recently from 08945c3 to ae2f69c Compare October 11, 2021 20:28
Copy link
Member

@steveharter steveharter left a comment

Choose a reason for hiding this comment

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

LGTM assuming green CI

MemberInfo memberInfo,
Type memberType,
bool isVirtual,
JsonSerializerOptions options)
{
JsonPropertyInfo jsonPropertyInfo = converter.CreateJsonPropertyInfo();
JsonPropertyInfo jsonPropertyInfo = new JsonPropertyInfo<sbyte>();
Copy link
Member

Choose a reason for hiding this comment

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

Could you also add a comment here explaining why sbyte is being used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will be sure to do so in a follow up to avoid CI reset. It will be the same as the comment on CreateIgnoredParameterPlaceholder below.

@layomia
Copy link
Contributor Author

layomia commented Oct 12, 2021

Test failures unrelated.

@layomia layomia merged commit 784687f into dotnet:main Oct 12, 2021
@layomia
Copy link
Contributor Author

layomia commented Oct 12, 2021

/backport to release/6.0

@github-actions
Copy link
Contributor

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1333901172

@ghost ghost locked as resolved and limited conversation to collaborators Nov 11, 2021
@layomia layomia deleted the JsonIgnore branch November 18, 2021 20:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[System.Text.Json] Regression: [JsonIgnore] Attribute not Applied to Ref Returns
3 participants