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

JsonPropertyName inheritance perserve the parent member #51165

Open
Tracked by #73255
goyzhang opened this issue Apr 13, 2021 · 10 comments
Open
Tracked by #73255

JsonPropertyName inheritance perserve the parent member #51165

goyzhang opened this issue Apr 13, 2021 · 10 comments

Comments

@goyzhang
Copy link

goyzhang commented Apr 13, 2021

When JsonPropertyName is applied to a sub-class override member, the original name in the parent class is preserved in the output of JsonSerializer.Serialize, as a result, that's two JSON field. I don't think this is by design?

Description

abstract class Person
{
    public abstract string Name { get; set; }
}
class Student : Person
{
    [JsonPropertyName("StudentName")]
    public override string Name { get; set; }
}

var json = JsonSerializer.Serialize(new Student() { Name = "Alice" });
  • Expected:
    {"StudentName":"Alice"}
  • Actual:
    {"StudentName":"Alice","Name":"Alice"}

Configuration

  • Which version of .NET is the code running on?
    .NET 5.0.2
  • What OS and version, and what distro if applicable?
    Windows 10 2004
  • What is the architecture (x64, x86, ARM, ARM64)?
    X64
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Text.Json untriaged New issue has not been triaged by the area owner labels Apr 13, 2021
@ghost
Copy link

ghost commented Apr 13, 2021

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

Issue Details

When JsonPropertyName is applied to a sub-class override member, the original name in the parent class is preserved in the output of JsonSerializer.Serialize, as a result, that's two JSON field. I don't think this is by design?

Description

	abstract class Person
	{
		public abstract string Name { get; set; }
	}
	class Student : Person
	{
		[JsonPropertyName("StudentName")]
		public override string Name { get; set; }
	}

var json = JsonSerializer.Serialize(new Student() { Name = "Alice" });
  • Expected:
    {"StudentName":"Alice"}
  • Actual:
    {"StudentName":"Alice","Name":"Alice"}

Configuration

  • Which version of .NET is the code running on?
    .NET 5.0.2
  • What OS and version, and what distro if applicable?
    Windows 10 2004
  • What is the architecture (x64, x86, ARM, ARM64)?
    X64
Author: goyzhang
Assignees: -
Labels:

area-System.Text.Json, untriaged

Milestone: -

@eiriktsarpalis eiriktsarpalis added bug and removed untriaged New issue has not been triaged by the area owner labels Apr 13, 2021
@eiriktsarpalis eiriktsarpalis added this to the 6.0.0 milestone Apr 13, 2021
@eiriktsarpalis
Copy link
Member

That seems like a bug to me. FWIW I can reproduce the same behaviour when I create this combination:

public abstract class Person
{
    [JsonPropertyName("StudentName")]
    public virtual string Name { get; }
}
public class Student : Person
{
    public override string Name => "Bob";
}

but the issue goes away when I remove the JsonPropertyName attribute altogether. cc @layomia

@devsko
Copy link
Contributor

devsko commented Apr 24, 2021

@eiriktsarpalis I can have a look. Even if it's a bug - fixing this would be a breaking change.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Apr 29, 2021
@layomia
Copy link
Contributor

layomia commented Jul 23, 2021

Triage - this isn't a regression from .NET 5.0 and I think it's a bit too late in the 6.0 wave to introduce a breaking change like this. We should consider in .NET 7.0.

@layomia layomia modified the milestones: 6.0.0, 7.0.0 Jul 23, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 23, 2021
@eiriktsarpalis eiriktsarpalis added the Priority:1 Work that is critical for the release, but we could probably ship without label Oct 12, 2021
@simon10says
Copy link

Is there any workaround?

@rogerfar
Copy link

Another bug related to this issue.

	abstract class Person
	{
		[JsonPropertyName("StudentName")]
		public abstract string Name { get; set; }
	}
	class Student : Person
	{
		public override string Name { get; set; }
	}

var json = JsonSerializer.Serialize(new Student() { Name = "Alice" });

Results in 2 properties:

{
    Name: 'Alice',
    StudentName: 'Alice'
}

@krwq
Copy link
Member

krwq commented Jul 7, 2022

Unfortunately we won't have time to address this in 7.0 but contract customization can be used to workaround this. Moving to 8.0 in the meantime

@eiriktsarpalis eiriktsarpalis modified the milestones: 8.0.0, Future Sep 28, 2022
@kgbaum
Copy link

kgbaum commented Feb 28, 2023

For those who may be looking for a work around until this can be addressed. You can make use of a dummy property:

abstract class Person
{
	public abstract string Name { get; set; }
}
class Student : Person
{
	[JsonIgnore]
	public override string Name { get => base.Name; set => base.Name = value; }
	
	public string StudentName { get => base.Name; set => base.Name = value; }
}

@pinkfloydx33
Copy link

Just ran into this as well. I was hoping to avoid contract customization as it feels a bit heavy for this; might try the workaround posted above instead. Would be nice if it worked out of the box though

@eiriktsarpalis eiriktsarpalis removed the Priority:1 Work that is critical for the release, but we could probably ship without label Mar 13, 2023
@tanveerbadar
Copy link

tanveerbadar commented Feb 18, 2024

This is causing source generation to generate invalid code when it interacts with JsonIgnoreCondition.WhenWritingDefault or JsonIgnoreCondition.WhenWritingNull.

#98634

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

Successfully merging a pull request may close this issue.

10 participants