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

Add option to let nulls through custom deserializers #852

Closed
wants to merge 1 commit into from
Closed

Add option to let nulls through custom deserializers #852

wants to merge 1 commit into from

Conversation

joffrey-bion
Copy link

This option fixes the "bug" that custom deserializers don't get called for null values, but without breaking backwards compatibility.
The issue #171 was already marked as closed but the problem was never truly adressed.

Resolves:
#171

This option fixes the "bug" that custom deserializers don't get called for null values without breaking backwards compatibility.
The issue #171 was already marked as closed but the problem was never truly adressed.

Resolves:
#171
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@joffrey-bion
Copy link
Author

I signed the CLA!

@googlebot
Copy link

CLAs look good, thanks!

@JakeWharton
Copy link
Contributor

The linked issue has a workaround without needing any changes though. Why
is this better?

On Tue, May 10, 2016, 7:36 PM Joffrey Bion [email protected] wrote:

I signed the CLA!


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#852 (comment)

@joffrey-bion
Copy link
Author

joffrey-bion commented May 11, 2016

Because, as you said, this is a workaround, and it forces the users to write more code than necessary. It doesn't really solve the initial problem.
The problem is that the current behaviour is not what is expected by users. As stated in the first answer on the issue, the only reason preventing the fix was backwards compatibility, which is preserved in this PR.
I'm sure that other users like me and the reporter of the issue will stumble upon this problem while using custom deserializers and the serializeNulls() global setting.

@joffrey-bion
Copy link
Author

Also, I may be missing something, but is there any reason not to make this change? I don't find that very costly to provide such an option.

@swankjesse
Copy link
Collaborator

If you want null you should use a TypeAdapter instead of a Deserializer. That already does what you want.

I'm opposed to this change because it adds a global setting to configure local behavior. In particular I don't expect all of my deserializers want this behavior, and enabling it means they all must be made null safe.

@joffrey-bion
Copy link
Author

joffrey-bion commented May 11, 2016

I'm opposed to this change because it adds a global setting to configure local behavior.

I don't really love using a global setting either, but this was the only easy way I could think of to keep the backwards compatibility. And I don't find it totally unacceptable.

In particular I don't expect all of my deserializers want this behavior, and enabling it means they all must be made null safe.

This is only necessary if you activate the feature and if you have explicit null values. And I think it is worth considering it anyway. If you activate the feature, it means you are in the mindset of handling your nulls in your deserializers so I don't see the problem.

If you want null you should use a TypeAdapter instead of a Deserializer.

I don't think Deserializer is conceptually a wrong choice just because null values are involved.

Using a TypeAdapter, could you please help me find the equivalent of the following:

@Override
public Property<?> deserialize(JsonElement json, Type typeOfT, JsonDeserializationContext context) throws
        JsonParseException {
    if (json.isJsonNull()) {
        return new SimpleObjectProperty<>(null);
    }
    Type typeParam = ((ParameterizedType) typeOfT).getActualTypeArguments()[0];
    Object obj = context.deserialize(json, typeParam);
    return new SimpleObjectProperty<>(obj);
}

This is a custom deserializer for JavaFX Property<T>.

Maybe I'm not familiar enough with TypeAdapters, but it looks like using it would require me to know the whole object hierarchy inside my object, which I don't. How would you suggest replacing:

context.deserialize(json, typeParam);

@JakeWharton
Copy link
Contributor

JakeWharton commented May 11, 2016

public final class PropertyTypeAdapterFactory implements TypeAdapterFactory {
  @Override public <T> TypeAdapter<T> create(Gson gson, TypeToken<T> type) {
    if (type.getRawType() != Property.class) return null;
    Type param = ((ParameterizedType) type.getType()).getActualTypeArguments()[0];
    TypeAdapter<?> delegate = gson.getDelegateAdapter(this, TypeToken.get(param));
    return (TypeAdapter<T>) new PropertyTypeAdapter<>(delegate);
  }

  static class PropertyTypeAdapter<V> extends TypeAdapter<Property<V>> {
    private final TypeAdapter<V> delegate;

    PropertyTypeAdapter(TypeAdapter<V> delegate) {
      this.delegate = delegate;
    }

    @Override public void write(JsonWriter out, Property<V> value) throws IOException {
      delegate.write(out, value.getValue());
    }

    @Override public Property<V> read(JsonReader in) throws IOException {
      boolean isNull = in.peek() == JsonToken.NULL;
      return new SimpleObjectProperty<>(isNull ? null : delegate.read(in));
    }
  }
}

@joffrey-bion
Copy link
Author

joffrey-bion commented May 11, 2016

Thanks a lot Jake for the adaptation, so in fact the TypeAdapterFactory is where I can get information about the inner target type and find a delegate TypeAdapter. That was my issue.

For future readers, there is a small bug in the above code, because it's not consuming the NULL token:

    @Override public Property<V> read(JsonReader in) throws IOException {
        if (in.peek() == JsonToken.NULL) {
            in.nextNull();
            return new SimpleObjectProperty<>(null);
        } else {
            return new SimpleObjectProperty<>(delegate.read(in));
        }
    }

Now I understand the workaround better, but anyway, don't you think Serializers should be able to handle it as well?
I mean, just because a workaround exists doesn't make the actual behaviour of the serializers more obvious to the users of Gson. I still believe null is a valid use case for deserialization, especially when they are explicit like that. Maybe I just don't have the correct mindset.

@joffrey-bion
Copy link
Author

joffrey-bion commented May 11, 2016

Interestingly, in the rush of giving sample code to convert, I reproduced the very thing I'm fighting against: I didn't let the chance to the delegate serializer to handle null as it pleases. It should instead have been:

@Override
public Property<?> deserialize(JsonElement json, Type typeOfT, JsonDeserializationContext context) throws
        JsonParseException {
    Type typeParam = ((ParameterizedType) typeOfT).getActualTypeArguments()[0];
    Object obj = context.deserialize(json, typeParam);
    return new SimpleObjectProperty<>(obj);
}

@AlexeyGorovoy
Copy link

Tried to use a code snippet provided @JakeWharton (except that I replaced types with my own), but unfortunately the TypeAdapter.read() method never gets called for null values.

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

Successfully merging this pull request may close these issues.

6 participants