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

Serialize type discriminator when using attributes too #138

Open
MYDIH opened this issue Sep 29, 2021 · 6 comments
Open

Serialize type discriminator when using attributes too #138

MYDIH opened this issue Sep 29, 2021 · 6 comments

Comments

@MYDIH
Copy link

MYDIH commented Sep 29, 2021

Hi,

I'm using polymorphic deserialization in the context of generated data classes. The fact that everything is configured through Attributes is really helping in this case because it means the models are self sufficient, they don't need a companion class that allow well configured deserialization. I don't want to have to set the type by hand or anything, but I still need to serialize deserializable JSON though (with the discriminator property serialized) ...

I forked this projet and found that adding the JsonSubtypesByDiscriminatorValueConverter like so achieves what I want :

[JsonConverter(typeof(JsonSubtypesByDiscriminatorValueConverter), typeof(Animal), "type")]

(I basically changed the visibility of the constructor in the fork)

I didn't know at the time the reason for this limitation, but from my experience (2 years of production code), I never experienced any problems with it.

Now I'm years later, and I need to use the fallback type attribute (what a relief that this thing exists). The Attribute isn't working with the above method, though creating a new constructor will achieve what I want.

Describe the solution you'd like

Is there a reason for this class to be strictly internal even though you can get a neat feature using it directly ?
Would you consider exposing it correctly (I mean in a "compliant with the API vision" way) ?

With the correct directions, I could file a PR

Describe alternatives you've considered

I would keep my fork ...

@manuc66
Copy link
Owner

manuc66 commented Sep 29, 2021

Hi @MYDIH

I try to put all that is not "documented api" to internal so that I can have less constraint linked to public api breaking changes.

Could you explain why you're not using:

[JsonConverter(typeof(JsonSubtypes), "type")]
[JsonSubtypes.FallBackSubType(typeof(UnknownExpression))]
public class Animal
{

}

@MYDIH
Copy link
Author

MYDIH commented Sep 30, 2021

Hi and thank you for your time,

Sorry that it wasn't clear enough,

Actually I want to serialize polymorphic JSON with a "type" attribute containing the type. I don't want to have to specify this type in the code, it should be set automatically when a class is of a given type based on the mappings I use with KnownSubType. I would prefer that this field isn't taking part of the class even though it wouldn't be blocking if it wasn't possible. My other requirement is that I want to be able to achieve that using Attributes of my classes/fields for the reason I mentioned before.

So far, when I'm using the documented method (the one you describe), the type isn't serialized along with the class. If I want to serialize it, it seems I have two options from my understanding, use the method where I need to generate a JsonSerializerSettings (which is a no go because I need to use the Attributes approach), and declaring the field type in my class (I figured that one searching the issues).

I tried adding the type field to my classes, but it is not automatically filled with the proper type when serializing. It seems I need to specify it manually. Tell me if I'm wrong of course.

What I did in the fork is creating a new constructor for the class JsonSubtypesByDiscriminatorValueConverter that sets the _serializeDiscriminatorProperty to true. I can confirm that it's working like charm 😄

I hope it makes everything clear enough ...

@An2An96
Copy link

An2An96 commented Nov 26, 2021

+1, this functional is very necessary

1 similar comment
@my6521
Copy link

my6521 commented Jan 12, 2022

+1, this functional is very necessary

@esbenbach
Copy link

@manuc66 Any chance a PR will be approved if we change the ctor to become public like MYDIH suggests?
I have found myself needing this several times as well.

@HaptelmanovArtem
Copy link

+1

manuc66 added a commit that referenced this issue Nov 27, 2023
manuc66 added a commit that referenced this issue Apr 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants