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 strategies for unknown and missing fields #2358

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

Conversation

Marcono1234
Copy link
Collaborator

@Marcono1234 Marcono1234 commented Mar 26, 2023

Purpose

Fixes #188
Fixes #1005

Resolves partially #61
(Users could implement their own @Required annotation, and a MissingFieldValueStrategy which throws an exception when the field whose value is missing has a @Required annotation.)

Supersedes #1006
(though does not contain the built-in support for registering default values, but users should be able to implement this themselves relatively easily with a MissingFieldValueStrategy which looks up the default values)

Description

Adds the interfaces MissingFieldValueStrategy and UnknownFieldStrategy to allow handling missing and unknown fields. The default behavior remains the same.

The API for MissingFieldValueStrategy was inspired by #1006, the API for UnknownFieldStrategy by #6601. These interfaces are rather low level, but should hopefully allow implementing most strategies, such as throwing an exception or ignoring missing or unknown fields.

Important: MissingFieldValueStrategy makes the assumption that there is always a Field for a JSON object member. If we decide to take this path we might have a hard time supporting setter methods in the future (#232), in case we want that at some point. Should we instead of directly exposing Field use some wrapper type which in the future would support representing setter methods as well (maybe we could use the existing FieldAttributes for that)?

I have marked this as draft for now to gather some feedback, especially about API and implementation. Please feel free to try out the changes locally and let me know what functionality you are missing or if you encounter any issues.

Also note that a similar pull request was dismissed before, see #660 (comment). Though the comments there indicate that the API there would suffice the needs of the users.

I am a bit worried that the changes to ReflectiveTypeAdapterFactory might negatively affect performance. Though maybe that is unavoidable to some extent.

Checklist

  • New code follows the Google Java Style Guide
  • If necessary, new public API validates arguments, for example rejects null
  • New public API has Javadoc
    • Javadoc uses @since $next-version$
      ($next-version$ is a special placeholder which is automatically replaced during release)
  • If necessary, new unit tests have been added
    • Assertions in unit tests use Truth, see existing tests
    • No JUnit 3 features are used (such as extending class TestCase)
    • If this pull request fixes a bug, a new test was added for a situation which failed previously and is now fixed
  • mvn clean verify javadoc:jar passes without errors

Footnotes

  1. In case this pull request gets merged, you can mention them as co-authors though if you want, since their APIs are still quite similar.

*/
// TODO: Should this really expose `instance`? Only use case would be to derive value from other fields
// but besides that user should not directly manipulate `instance` but return new value instead
Object handleMissingField(TypeToken<?> declaringType, Object instance, Field field, TypeToken<?> resolvedFieldType);

Check notice

Code scanning / CodeQL

Useless parameter

The parameter 'instance' is never used.
*/
// TODO: Should this really expose `instance`? Only use case would be to derive value from other fields
// but besides that user should not directly manipulate `instance` but return new value instead
Object handleMissingField(TypeToken<?> declaringType, Object instance, Field field, TypeToken<?> resolvedFieldType);

Check notice

Code scanning / CodeQL

Useless parameter

The parameter 'resolvedFieldType' is never used.
* @param gson {@code Gson} instance which can be used to read the field value from {@code jsonReader}
* @throws IOException if reading or skipping the field value fails
*/
void handleUnknownField(TypeToken<?> declaringType, Object instance, String fieldName, JsonReader jsonReader, Gson gson) throws IOException;

Check notice

Code scanning / CodeQL

Useless parameter

The parameter 'instance' is never used.
* @param gson {@code Gson} instance which can be used to read the field value from {@code jsonReader}
* @throws IOException if reading or skipping the field value fails
*/
void handleUnknownField(TypeToken<?> declaringType, Object instance, String fieldName, JsonReader jsonReader, Gson gson) throws IOException;

Check notice

Code scanning / CodeQL

Useless parameter

The parameter 'gson' is never used.
*/
// TODO: Should this really expose `instance`? Only use case would be to derive value from other fields
// but besides that user should not directly manipulate `instance` but return new value instead
Object handleMissingField(TypeToken<?> declaringType, Object instance, Field field, TypeToken<?> resolvedFieldType);

Check notice

Code scanning / CodeQL

Useless parameter

The parameter 'declaringType' is never used.
@Marcono1234 Marcono1234 force-pushed the marcono1234/unknown-missing-field-strategies branch from 140eda7 to 083ea86 Compare March 26, 2023 23:28
Comment on lines +415 to +416
* is encountered in the JSON data. If a field which is excluded from deserialization
* appears in the JSON data it is considered unknown as well.
Copy link
Collaborator Author

@Marcono1234 Marcono1234 Mar 26, 2023

Choose a reason for hiding this comment

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

There might be use cases where a field excluded from deserialization should not be considered unknown, but should just be silently ignored.

With the current UnknownFieldStrategy API users might be able to determine themselves if a field is excluded, but not in a very reliable way since there are multiple ways in which a field can be excluded (e.g. modifiers, exclusion strategy, @Expose) and Gson does not expose direct ways to test this.

We could possibly add a boolean excluded parameter to UnknownFieldStrategy.handleUnknownField whose value is true if ReflectiveTypeAdapterFactory finds a BoundField but it has !field.deserialized. Though I am not sure if that is really worth it. What are your opinions?

Comment on lines +74 to +76
// TODO: Should this really expose `instance`? Only use case would be to derive value from other fields
// but besides that user should not directly manipulate `instance` but return new value instead
Object handleMissingField(TypeToken<?> declaringType, Object instance, Field field, TypeToken<?> resolvedFieldType);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not completely sure whether it is really necessary to expose instance.


/**
* A strategy defining how to handle missing field values during reflection-based deserialization.
*
Copy link
Collaborator Author

@Marcono1234 Marcono1234 Mar 8, 2024

Choose a reason for hiding this comment

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

Maybe this should have a warning that users should be careful when implementing security critical validation with this, and should add extensive tests. Because if they forget to call setMissingFieldValueStrategy or use a default Gson instance somewhere, then their custom strategy will not be used.


/**
* A strategy defining how to handle unknown fields during reflection-based deserialization.
*
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe this should have a warning that users should be careful when implementing security critical validation with this, and should add extensive tests. Because if they forget to call setUnknownFieldStrategy or use a default Gson instance somewhere, then their custom strategy will not be used.

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

Successfully merging this pull request may close these issues.

Default Value for Missing fields in Json Support handlers for unknown properties, useful for error handling
1 participant