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

feat(java): add Optional<T> serialization/deserialization #985

Closed
wants to merge 1 commit into from

Conversation

jpJuni0r
Copy link

@jpJuni0r jpJuni0r commented Jul 6, 2022

This PR adds support for Optional<T> serialization/deserialization for the firestore.

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #984 ☕️

@jpJuni0r jpJuni0r requested a review from a team as a code owner July 6, 2022 07:59
@google-cla
Copy link

google-cla bot commented Jul 6, 2022

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).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@product-auto-label product-auto-label bot added the api: firestore Issues related to the googleapis/java-firestore API. label Jul 6, 2022
Copy link

@ywmei-brt1 ywmei-brt1 left a comment

Choose a reason for hiding this comment

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

Thank you very much for your PR, I have left some questions for you in the PR, and looking forward to hearing back from you.


@Test
public void deserializeOptional() {
OptionalStringBean bean = deserialize("{'foo': 'bar'}", OptionalStringBean.class);

Choose a reason for hiding this comment

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

What is the plan for deserializing a null value back to an Optional field?
Currently, both of the following 3 cases are serializing foo as null in firestore:

  1. OptionalStringBean.foo = Optional.empty() ;
  2. OptionalStringBean.foo = Optional.ofNullable(null) ;
  3. OptionalStringBean.foo = null ;

How do you plan to deserialize them back in each case? For example, which of the following should pass, and which should throw?

  @Test
  public void deserializeEmptyOptional() {
    OptionalStringBean bean = deserialize("{'foo': 'null'}", OptionalStringBean.class);
    assertEquals(Optional.empty(), bean.foo);
  }

  @Test
  public void deserializeNullableOptional() {
    OptionalStringBean bean = deserialize("{'foo': 'null'}", OptionalStringBean.class);
    assertEquals(Optional.ofNullable(null), bean.foo);
  }

  @Test
  public void deserializeNullOptional() {
    OptionalStringBean bean = deserialize("{'foo': 'null'}", OptionalStringBean.class);
    assertEquals(null, bean.foo);
  }
  ``

Copy link
Author

Choose a reason for hiding this comment

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

Hi and thanks a lot for taking a look at the code. You raise an interesting point which I overlooked at the first Iteration!

When receiving null in our JSON, I think it best makes sense to set the property to Optional.empty() and not to null. Also, there's no distinction between the first and the second case you described, as Optional.empty() and Optional.ofNullable(null) are equivalent.

I've adjusted my implementation and added a test case for the described behaviour.

Choose a reason for hiding this comment

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

Thank you very much for your contribution. We are discussing the best way to handle some edge cases right now.
Just want to check with you that you have any use case for serializing something like Optional<Optional<String>>?
Or you think we should just not support the nested Optional object's serialization:

  private static class OptionalInnerdBean{
    public Optional<String> bar;
  }
  private static class OptionalNestBean{
    public Optional<OptionalInnerdBean> foo;
  }

  @Test
  public void deserializeNestedOptional() {
    OptionalNestBean outer = new OptionalNestBean();
    OptionalInnerdBean inner = new OptionalInnerdBean();
    inner.bar = Optional.of("foo-bar");
    outer.foo = Optional.of(inner);
    
    assertExceptionContains(
            "throw some error message",
            () -> serialize(outer));
  }

Copy link
Author

Choose a reason for hiding this comment

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

I guess the best way to serialize null to Optional<Optional<String>> would be Optional.of(Optional.empty()). Would like to have any other modifications or other edge cases handled? Then I'll adjust the code accordingly and also add a test (incl. implementation) for the case you just described.

Choose a reason for hiding this comment

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

Hi, thank you very much for the update. Apologize for getting back to you late.

We had some meetings within the team, and decided we are going to support serialization of Optional<T> in a different way. There are a few reasons why we hesitate about direct supporting serialization of Optional<T>:

  1. We need to keep our Java server SDK aligned with Android SDK (but this reflection requires android API level 29, which is too high for now );
  2. Different customers might want to handle the serialization of Optional in different approach, we are leaning to provide a mechanism to customers so that they can build their own custom serializers in the future to handle these cases.

Copy link
Author

Choose a reason for hiding this comment

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

Okay, thanks a lot for the feedback! I guess I can close this PR now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: firestore Issues related to the googleapis/java-firestore API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow Optional<T> serialization/deserialization
2 participants