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 a @Required annotation for fields #1900

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

grishka
Copy link

@grishka grishka commented May 30, 2021

This is a long-requested feature (#61).

I'm not sure if this is the best way to do this, especially the Set<String> part to keep track of required fields that were found in the input, but I haven't come up with anything better. I'm also not sure if this annotation should affect serialization — maybe it should also throw an exception when attempting to serialize a required field that is null?

@google-cla
Copy link

google-cla bot commented May 30, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). 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 (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added the cla: no label May 30, 2021
@grishka
Copy link
Author

grishka commented May 30, 2021

@googlebot I signed it!

@google-cla google-cla bot added cla: yes and removed cla: no labels May 30, 2021
@Marcono1234
Copy link
Collaborator

Marcono1234 commented May 30, 2021

Looks reasonable, but maybe two things which have to be considered:

  1. In some use cases an explicit JSON null is considered a value, so maybe the annotation should have an allowNull (or similar) annotation element with default value false to support that use case.
  2. Instead of tracking which field names have been deserialized you should probably track the Field (needs some refactoring). Otherwise this won't work for @SerializedName with alternate, e.g.:
    class Test {
        @SerializedName(value = "a", alternate = "b")
        String s;
    }
    For the exception listing missing required fields it might be reasonable to report the 'main' name, i.e. either the field name or @SerializedName#value, otherwise the exception message might be confusing.

Note that I am not a member of this project, so these are only suggestions.

/**
* This annotation causes a {@link com.google.gson.JsonParseException} to be thrown
* during deserialization if this field is not present in the input JSON, is null,
* or is not of a type assignable to this field.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or is not of a type assignable to this field.

That is the default behavior for fields anyways, isn't it? I don't think this is worth mentioning here then, otherwise it might cause confusion whether this does / does not apply to optional fields.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, if you try to parse something that has an object where you expect a string, the string will be null. The idea behind this annotation is to guarantee that a required field will never, under any circumstances, be null.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, if you try to parse something that has an object where you expect a string, the string will be null.

Are you sure? The following throws an exception:

public class GsonParsingTest {
    static class Test {
        String s;
    }
    
    public static void main(String[] args) {
        new Gson().fromJson("{\"s\":{}}", Test.class);
    }
}

Maybe there are custom type adapters which return null for non-null JSON in case of type mismatch, but we cannot generally assume that. Not every type adapter which returns null must have encountered wrong JSON data.

@lyubomyr-shaydariv
Copy link
Contributor

lyubomyr-shaydariv commented May 31, 2021

@Required sounds like a twin of JSR-303 @NotNull and does not seem to make it extensible. I would not create a doubling annotation, but rather would think of implementing a generic method to apply any kind of post-processing at least at DTO class level (probably by extending ReflectiveTypeAdapterFactory that makes a lot of things hard to accompilsh being not extensible). Validation is only a particular case of such a generic thing. This all seems to be pretty achievable using post-processing mechanisms like RuntimeTypeAdapterFactory from the Gson Extras demo:

  • it's possible to do anything with a deserialized object, including validation (entire JSON document must be consumed though);
  • it's possible to incorporate any strategy (including JSR-303 validation as a Jackson-like module that can be built on top of type adapter factories) right into type adapter factories, so that any ReflectiveTypeAdapterFactory.Adapter-derived type adapter could invoke the strategy and quick-validate the just-deserialized object (subobjects revalidated every time a super object is completed to be serialized, so it may be time expensive -- but it sounds like it can be optimized greatly).

Any other thoughts on making Gson deserialization-only, but extensible on user demand only, tool?


UPD1 (just not to spawn another comment): JSR-303 annotations are runtime-visible and have nothing to do to static code analysis and Android/JetBrains at all.

@grishka
Copy link
Author

grishka commented May 31, 2021

but rather would think of implementing a generic method to apply any kind of post-processing

The problem with the post-processing approach is that it, in case of a field of a primitive type, it won't let you tell the difference between the lack of that field in the json and its presence with its default value (0/false). Yes, boxed types can kinda awkwardly solve this by allowing those fields to be null. But you won't want to use boxed types in all your objects just so you could validate them in a post-processing step. You really want to do this in a place where you have access to the JSON.

JSR-303 @NotNull

This is the first time I've seen this after more than 10 years writing Java. I do know about @NotNull/@Nullable commonly used in many places these days, but these usually either come from a JetBrains library for Java SE or an AndroidX library for Android. And — I just looked — their retention is "class", so not visible at runtime.

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.

None yet

3 participants