-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
base: main
Are you sure you want to change the base?
Conversation
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 What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
Looks reasonable, but maybe two things which have to be considered:
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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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. |
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.
This is the first time I've seen this after more than 10 years writing Java. I do know about |
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?