Skip to content

Conversation

@devsko
Copy link
Contributor

@devsko devsko commented Apr 29, 2021

Fixes #51165

Implicitly ignore properties that are overridden in a derived class but have a differing name set by JsonPropertyName.
This meets the current behavior of overridden properties with the same name or no JsonPropertyName at all.
Also fixes an issue with new virtual properties that were previously handled as override.

This seems to be a breaking change of the scenario described in #51165.

@ghost
Copy link

ghost commented Apr 29, 2021

Tagging subscribers to this area: @eiriktsarpalis, @layomia
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #51165

Implicitly ignore properties that are overridden in a derived class but have a differing name set by JsonPropertyName.
This meets the current behavior of overridden properties with the same name or no JsonPropertyName at all.

Author: devsko
Assignees: -
Labels:

area-System.Text.Json

Milestone: -

@ericstj ericstj added the breaking-change Issue or PR that represents a breaking API or functional change over a previous release. label May 24, 2021
@ghost ghost added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label May 24, 2021
@ericstj
Copy link
Member

ericstj commented May 24, 2021

It's not clear to me if there was a conclusion on expected behavior here. We should get that before opening the PR. IIRC we had app-compat related issues to this in 5.0 (#32107, #36936, #37720) and I'd want to make sure whatever changes we make here don't introduce new breaking changes.

@layomia layomia self-assigned this Jun 1, 2021
@ericstj ericstj requested review from layomia and removed request for layomia June 21, 2021 20:47
@layomia
Copy link
Contributor

layomia commented Jun 23, 2021

This seems to be a breaking change of the scenario described in #51165.

A breaking change here for correctness in this edge case seems fine. The proposed changes in this PR seem safe and reasonable, but would need to come after this other PR that unifies the property visibility semantics between the source-gen and non-source gen programming models: #54526.

@ericstj
Copy link
Member

ericstj commented Jul 19, 2021

@devsko could you rebase this, @layomia do you have any feedback for @devsko that he could address during that process?

@terrajobst terrajobst added the community-contribution Indicates that the PR has been added by a community member label Jul 19, 2021
@layomia
Copy link
Contributor

layomia commented Jul 23, 2021

@devsko, thanks for the PR! Unfortunately, per the tracking issue #51165 (comment), I do think it's a bit late in the release cycle to introduce this breaking change. We'll ping this PR when we circle back in .NET 7.

@layomia layomia closed this Jul 23, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Aug 22, 2021
@ericstj ericstj added this to the 6.0.0 milestone Sep 30, 2021
@ericstj ericstj removed breaking-change Issue or PR that represents a breaking API or functional change over a previous release. needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet labels Sep 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Text.Json community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

JsonPropertyName inheritance perserve the parent member

4 participants