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

Optional data type #2022

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

Conversation

nadiawoodninja
Copy link

Resolves the enhancement request to add Support for java.util.Optional. issue 1102

@google-cla google-cla bot added the cla: yes label Nov 18, 2021
Copy link
Member

@eamonnmcmanus eamonnmcmanus left a comment

Choose a reason for hiding this comment

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

Thanks!

This PR does two things:

  1. Update the build to Java 8. I think we are indeed ready to do that, but it should be in a PR of its own. Do you want to make that?
  2. Add support for serializing Optional. I'm not 100% convinced that this is useful. I note that the Optional class is not serializable with Java serialization (does not implement java.io.Serializable) for example. Do you have a need for this?

Also, the PR includes generated javadoc files, which it should not.

import java.util.Map;
import java.util.StringTokenizer;
import java.util.UUID;
import java.util.*;
Copy link
Member

Choose a reason for hiding this comment

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

Google Java style doesn't allow wildcard imports.

@Marcono1234
Copy link
Collaborator

I'm not 100% convinced that this is useful. I note that the Optional class is not serializable with Java serialization (does not implement java.io.Serializable) for example. Do you have a need for this?

If I may jump in here; the number of 'thumbs up' on #1102 and the number of upvotes on the corresponding StackOverflow question suggest that there is some need for this.

hanwen pushed a commit to GerritCodeReview/gerrit that referenced this pull request May 20, 2022
In the current version GSON has no dedicated implementation for Optional
(more context given in [1]) type and uses reflection in order to
(de)serialize it. The problem is that newer versions of JDK (for
instance 17) no longer allow for JDK internal classes reflection access.
One option to solve it is to pass arg to VM ('--add-opens
java.base/java.util=ALL-UNNAMED') but that gets flaky as more tests are
written and option keeps getting propagated to more packages and what
is more it is not a solution for long term.

This change solves it by adding a general purpose Optional
(de)serializer that gets registered prior to the specific ones and
doesn't rely on reflection.

The (de)serialization of this adapter is exactly the same as the
reflective (de)serialization.

[1] google/gson#2022

Release-Notes: Introduce non-reflection based Optional Type Adapter
Bug: Issue 15504
Change-Id: I64b553f2a8c14ce1c83fbac8edfeeb3e6aa2c2cf
hanwen pushed a commit to GerritCodeReview/gerrit that referenced this pull request May 23, 2022
In the current version GSON has no dedicated implementation for Optional
(more context given in [1]) type and uses reflection in order to
(de)serialize it. The problem is that newer versions of JDK (for
instance 17) no longer allow for JDK internal classes reflection access.
One option to solve it is to pass arg to VM ('--add-opens
java.base/java.util=ALL-UNNAMED') but that gets flaky as more tests are
written and option keeps getting propagated to more packages and what
is more it is not a solution for long term.

This change solves it by adding a general purpose Optional
(de)serializer that gets registered prior to the specific ones and
doesn't rely on reflection.

The (de)serialization of this adapter is exactly the same as the
reflective (de)serialization.

[1] google/gson#2022

Release-Notes: Introduce non-reflection based Optional Type Adapter
Bug: Issue 15504
Change-Id: I64b553f2a8c14ce1c83fbac8edfeeb3e6aa2c2cf
(cherry picked from commit f0b875b)
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.

3 participants