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

Custom static factory-based deserializer? #4004

Open
garretwilson opened this issue Jun 26, 2023 · 15 comments
Open

Custom static factory-based deserializer? #4004

garretwilson opened this issue Jun 26, 2023 · 15 comments
Labels
to-evaluate Issue that has been received but not yet evaluated

Comments

@garretwilson
Copy link

As I noted on Stack Overflow, I'm using Java 17 and I have a Bar type I want serialized to JSON using its toString() method. I'm serializing using a Jackson ObjectMapper (created via a JsonMapper.Builder). I found out from my other Stack Overflow question that I can specify a ToStringSerializer as a serializer in a Module, as the answer indicated. (I had done this years ago, but forgotten how.)

module.addSerializer(Bar.class, new ToStringSerializer());
//technically ToStringSerializer.instance would be better; I used instantiation for

But now how about deserializing Bar using a static factory method? Looking at the code from FromStringDeserializer.Std._deserialize(…), I see that known types are simply checked using a case and then the static factory methods are explicitly invoked, e.g. Charset.forName(value) or URI.create(value). This indicates to me that there may not be an existing deserializer for general serializer types—otherwise we could just use new StaticFactoryDeserializer("forName") and new StaticFactoryDeserializer("create"), etc. to do the same thing.

Of course I can write one of these myself, but I'm curious whether this exists already. Basically I'd like to do this:

module.addDeserializer(Bar.class, new StaticFactoryDeserializer("createNewBarInstance"));
…

StaticFactoryDeserializer would also allow a Function, so I could use it like this:

module.addDeserializer(Bar.class, new StaticFactoryDeserializer(Bar::createNewBarInstance));
…

(Probably the deserializer would create its own function instance internally when created with the method name, but that's an implementation detail.)

Does such a deserializer exist, or is one planned? If I wrote one, would it be something you'd be interested in including in Jackson?

@garretwilson garretwilson added the to-evaluate Issue that has been received but not yet evaluated label Jun 26, 2023
@garretwilson
Copy link
Author

Looks like this is especially important, because the default implementation will find a private constructor accepting a String using AnnotatedConstructor from StdValueInstantiator.createFromString(DeserializationContext ctxt, String value):

    public Object createFromString(DeserializationContext ctxt, String value) throws IOException
    {
        if (_fromStringCreator != null) {
            try {
                return _fromStringCreator.call1(value);
            } catch (Exception t) {
                …

A user may not realize that this internal constructor is being called, because it seemingly "just works". Unfortunately for my particular MyValueType, there will be tens of thousands of instances of the exact same value being represented. (This is typical of value classes.) So instead of calling new MyValueType("foo") I need to call MyValueType.from("foo").

Because there is no easy way to register value types with ObjectMapper, it may be that many users are having huge unnecessary memory bloat with their custom value classes, because the code works and they didn't know they needed to register a deserializer for the value type factory method (and wouldn't readily know how anyway, and wouldn't have time to research it over days as I am).

I think such a facility definitely needs to be added and called out in the documentation.

@yawkat
Copy link
Member

yawkat commented Jun 29, 2023

you can mark the factory method as a JsonCreator (with a mixin if necessary)

@garretwilson
Copy link
Author

you can mark the factory method as a JsonCreator (with a mixin if necessary)

Thanks, but the whole point of this exercise was to configure the mapper without modifying the class itself.

@yawkat
Copy link
Member

yawkat commented Jun 29, 2023

then use a mixin?

@garretwilson
Copy link
Author

then use a mixin?

Why should I have to create a completely new class?

Why can't there be an existing class I can simply instantiate, telling it the name of my factory method, or providing it with a method reference?

I know there are workarounds. This ticket was opened so that I wouldn't have to use a workaround.

@yawkat
Copy link
Member

yawkat commented Jun 29, 2023

if you dont want to add a mixin for some reason (e.g. because the name of the method is dynamic), you can instead add a value instantiator.

@garretwilson
Copy link
Author

if you dont want to add a mixin for some reason …

I don't want to add a mixin because I don't want to write an extra class to go along with every value class I use, and I prefer not to litter the source code of the value class with serialization-specific annotations.

I don't know why this request is difficult to understand. I can do this to serialize a class to a string:

module.addSerializer(Bar.class, new ToStringSerializer());
…

I want to do the analogous thing to register a deserializer for my value class, indicating the factory method to use.

Do you agree that ToStringSerializer is useful, or do you think it should be removed and that we should be forced to write a mixin class for every Foo and Bar value class we create? If you agree ToStringSerializer is useful, I don't understand why you don't think the analogous deserializer helper is appropriate.

I'm even offering to write this class. I just want to make sure Tatu or someone will accept it so I don't waste my time doing it as part of the existing Jackson project.

@pjfanning
Copy link
Member

@garretwilson Tatu is on vacation. Can you wait a few weeks?

@garretwilson
Copy link
Author

garretwilson commented Jun 29, 2023

Can you wait a few weeks?

Oh, sure, no problem at all! Believe me I have a thousand things ahead of this anyway on my list of things to do.

For the moment I have this in my source code:

//TODO create utility class; see https://github.com/FasterXML/jackson-databind/issues/4004
.addDeserializer(Bar.class, new JsonDeserializer<Bar>() {
  @Override
  public Bar deserialize(JsonParser parser, DeserializationContext context) throws IOException, JacksonException {
    return Bar.of(parser.getValueAsString());
  }
})

No rush on this—just wanted to file this while I was thinking about it.

@yihtserns
Copy link
Contributor

But now how about deserializing Bar using a static factory method?

I believe you can declare a static method named either valueOf or fromString, and Jackson will automagically use that for deserialization:

public class Bar {
  ...
   public static Bar valueOf(String stringVal) {
       ... 
   } 
} 

// or... 

public class Bar {
  ...
   public static Bar fromString(String stringVal) {
       ... 
   } 
} 

@garretwilson
Copy link
Author

garretwilson commented Jun 30, 2023

I believe you can declare a static method named either valueOf or fromString, and Jackson will automagically use that for deserialization:

Thanks, but the static factory method of my value class has a different name.

And since you brought this up … 😁

I realize that at the time Jackson was written, valueOf() and fromString() were the convention for value class factory methods (e.g. Integer.valueOf(int)). Value classes at the time were relatively rare (remember AWT Insets 😱 and even Date?) and not well understood in the community as such; see for example the note on Value-based classes in the JDK 8 which reflects a growing awareness.

However nowadays value classes/objects are better understood (see Fowler's ValueObject for an overview) and many newer languages include facilities for value classes built-in. The naming convention is evolving merely to use of(…) when referring to to components (e.g. Set.of(…)) or parse(CharSequence) when converting from canonical lexical form (e.g. Instant.parse(CharSequence)).

In essence Jackson, in recognizing valueOf() and fromString() as special, is stuck on an older convention which was still evolving. (I'm not criticizing Jackson in this regard, unless it would be to suggest revisiting these conventions or making them pluggable, which I guess is what this ticket is to some extent.) I certainly wouldn't want to design my classes around those rules of yesteryear.

P.S. I was just looking over the lesson I wrote on value objects around 10 years ago, and interestingly I find this sentence:

Modern factory methods commonly use the name of(…) rather than valueOf(…).

@yihtserns
Copy link
Contributor

yihtserns commented Jun 30, 2023

Sure, everybody has got their own preferences. Just letting you know such facility exists.

Of course I can write one of these myself, but I'm curious whether this exists already. Basically I'd like to do this...
I'm even offering to write this class. I just want to make sure Tatu or someone will accept it so I don't waste my time doing it as part of the existing Jackson project.

Now we know that thing you preferred doesn't exist in Jackson out-of-the-box. So whether the Jackson core maintainers want to accept it into the codebase or not, you'd still have to create it anyway to satisfy your personal preference, right? No reason to wait time to get cracking good luck. 👍

@cowtowncoder
Copy link
Member

cowtowncoder commented Jul 7, 2023

Ok I am back and... I think it makes sense to offer easier means for adding basic closure-based deserializer(s), maybe just (or at first) for things that accept JSON Strings, like more advanced FromStringDeserializer.
It would be given supported type (like Locale.class) and then value-accessor Function. Or something along those lines.
And as suggested we could/should add convenience method(s) in SimpleModule if that's helpful.

... that is, yes, exactly like you said.

Implementation should probably also support CoercionConfig to allow binding from other JSON scalar types, like numbers -- I think FromStringDeserializer already does that I guess.

We could perhaps extend FromStringDeserializer as FunctionalFromStringDeserializer, implement _deserialize(...) via Function evaluation.

@yawkat
Copy link
Member

yawkat commented Jul 10, 2023

Maybe we should make ValueInstantiator easier to implement (with a lambda?) and create a way to add it without a Module similar to (de)serializers.

@cowtowncoder
Copy link
Member

While ValueInstantiator is a possibility, addition of methods in Module is just syntactic sugar and could be left out & just offer convenience deserializer(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
to-evaluate Issue that has been received but not yet evaluated
Projects
None yet
Development

No branches or pull requests

5 participants