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

System.Text.Json.Serialization.JsonSerializer should respect DataMemberAttribute.Name #30009

Closed
abelykh0 opened this issue Jun 24, 2019 · 10 comments
Labels
area-System.Text.Json enhancement Product code improvement that does NOT require public API changes/additions json-functionality-doc Missing JSON specific functionality that needs documenting
Milestone

Comments

@abelykh0
Copy link

The following code outputs {"MyDataMember":"dataMember"}, should be {"RENAMED":"dataMember"}

using System;
using System.Diagnostics;
using System.Runtime.Serialization;
using System.Text.Json.Serialization;

namespace JsonSerializerIssue
{
    [DataContract]
    internal class MyData
    {
        [DataMember(Name = "RENAMED")]
        public string MyDataMember { get; set; }
    }

    class Program
    {
        static void Main(string[] args)
        {
            MyData myData = new MyData() { MyDataMember = "dataMember" };
            Debug.WriteLine(JsonSerializer.ToString(myData));
        }
    }
}
@ahsonkhan
Copy link
Member

Is there a particular reason why the new JsonPropertyName attribute won't work for your scenario?

@abelykh0
Copy link
Author

We have tens of thousand lines of code and would like to replace Newtonsoft.Json with the new functionality.

@jjrdk
Copy link

jjrdk commented Sep 28, 2019

+1 for sticking with an existing API.

@hmqgg
Copy link

hmqgg commented Sep 29, 2019

Is there a particular reason why the new JsonPropertyName attribute won't work for your scenario?

For example, if we build a Blazor app, and ofc we'd like to make JSON models shared between our server and client to keep them consistent, but on the client side (i.e. Blazor project), we can't use System.Text.Json due to the runtime restrictions so we can't use JsonPropertyName.

What's more, many existing projects now use DataContract/DataMember attributes because it's common and most of JSON libs respect it.

I knew that now it's life after WCF, and we'd better not use these attributes introduced by WCF since it's nearly EoS. But we need attributes that could describe JSON models across so many JSON libs and runtime.

@kucint
Copy link

kucint commented Oct 15, 2019

there is currently no replacement for [DataMember(EmitDefaultValue)] in System.Text.Json.

@jjrdk
Copy link

jjrdk commented Oct 15, 2019

@ahsonkhan Can you explain why it is necessary to create a new attribute only for json serialization? What if I also need to serialize to other formats? Should I now decorate my type with additional attributes?

Why not use the existing common attribute?

@stephanharmse
Copy link

@jjrdk, I agree. It seems logical to abstract the serialization decorations to allow for the serializing and deserializing in multiple formats (based on request Accept headers for example).

@layomia
Copy link
Contributor

layomia commented Nov 23, 2019

Closing as duplicate - this falls under the ask to support types from System.Runtime.Serialization: https://github.com/dotnet/corefx/issues/38758.

@Tiedye
Copy link

Tiedye commented Jul 20, 2020

Should this be reopened as the conclusion of #29975 was to make individual issues for individual features, which this is.

yallie added a commit to yallie/JsonServices that referenced this issue Aug 14, 2020
…ssues:

1. Lacks support for DataContract/DataMember attributes:
dotnet/runtime#29975
dotnet/runtime#30009
2. Deserializes the primitive values as JsonElement instances.
@steveharter
Copy link
Member

Early-on during design of STJ in 3.0, it was decided not to support pre-existing attributes mainly because they would only be partially supported and it would be hit-and-miss meaning STJ would only support some of those in the first release and additional support added in future releases. This would cause endless confusion over what is and what is not supported. STJ can't just throw NotSupportedException for the unsupported cases since that would mean the attribute usage would need to be removed from the corresponding types (not feasible unless the types are owned), or have a way to turn off the exception.

Also since STJ would need to explicitly look for those attributes, it would cause some slowdown during warm-up.

Consider just the DataMemberAttribute properties:

  • EmitDefaultValue: in 3.0, we couldn't support since we didn't have that feature. Now in 5.0 it is.
  • IsNameSetExplicitly: not supported yet.
  • IsRequired: not supported yet.
  • Name: supported through STJ.PropertyNameAttribute.
  • Order: not supported yet.

Then there's also a few other attributes including CollectionDataContractAttribute, OnDeserializedAttribute that would also contribute to hit-and-miss. STJ also didn't consider other areas including System.IConvertible.

So we went with a new explicit model that is intuitive (if it's not in the STJ namespace then it's not supported) that is high-performance with the thought we can add support for pre-existing attributes through an opt-in mode\flag of some sort or perhaps a pluggable metadata provider. These have not been implemented yet.

Moving-forward, having a pluggable metadata provider along with an opt-in System.Runtime.Serialization "compat" implementation of that provider makes sense to me. Not sure if that has enough priority for 6.0, but it could be considered along with the feature to expose metadata and more flexible converters.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Text.Json enhancement Product code improvement that does NOT require public API changes/additions json-functionality-doc Missing JSON specific functionality that needs documenting
Projects
None yet
Development

No branches or pull requests

10 participants