Feat: Record serialization/deserialization #1706

milaGGL · 2024-06-13T17:42:24Z

Fixes #1152
Contribution by @eranl: #1223
Internal link for details: b/363272745

…cloud-firestore module by adding a test-jdk17 directory and a java17 profile, and eliminate google-cloud-firestore-jdk17 module

…cord

generated-files-bot · 2024-08-28T18:10:24Z

Warning: This pull request is touching the following templated files:

.github/workflows/ci.yaml

This reverts commit 646ff81.

This reverts commit 3ea41cd.

tom-andersen

Looks good.

My comments are mostly about internal location of code. I feel code is being split across many files, and this has caused some duplication, decrease in encapsulation and code that jumps between files more. I think this could be simplified.

If this can be cleaned up a little now, that would be good. But I don't consider this a blocker to merging. We can followup on this in future PRs if you like.

tom-andersen · 2024-09-11T16:54:03Z

google-cloud-firestore/src/main/java/com/google/cloud/firestore/CustomClassMapper.java

  // Helper class to convert from maps to custom objects (Beans), and vice versa.
-  private static class BeanMapper<T> {
-    private final Class<T> clazz;
+  private static class PojoBeanMapper<T> extends BeanMapper<T> {


Since BeanMapper and RecordMapper exist in separate files, I think we should also move the PojoBeanMapper from being an inner class to also being a top level class in it's own file as well.

I think previously, the serialization/deserialization logic was contained in CustomClassMapper, but now is spread across many files. This is the Java way of doing things, with separate files for separate classes. But we should maybe consider having a folder to contain all these classes, maybe named serialization.

I would like to see some refactoring to:

Better encapsulate logic. Many methods have had their private attribute removed, which I take as indication to logic being placed in the wrong place. The BeanMapper class could be a better place for common logic, since methods can be protected here.

Remove duplication. For example:

if (serverTimestamps.contains(property) && propertyValue == null) { // Replace null ServerTimestamp-annotated fields with the sentinel. serializedValue = FieldValue.serverTimestamp(); } else { serializedValue = CustomClassMapper.serialize(propertyValue, path.child(property)); }

Could be pushed up into parent BeanMapper somehow.

Some more logic from CustomClassMapper should likely be moved over to DeserializeContext. For example, since ErrorPath lives in DeserializeContext, the serializeError and deserializeError should maybe move over there as well now, perhaps even as methods on the ErrorPath class.

move PojoBeanMapper to separate class

move serialization related files into encoding folder (keeping consistent with database)
-- Note: this will make CustomClassMapper public to be accessible by other files in firestore. Marked "internalApi" to avoid public use.

"Many methods have had their private attribute removed" -> so far, custom class mapper only have serialize and deserializeToType methods changed from private to default, this is acceptable.

move duplictated code to BeanMapper

move serializeError and deserializeError to DeserializeContext

The BeanMapper class could be a better place for common logic, since methods can be protected here.

I agree. Having mapper subclasses call an external utility class is less modular.

tom-andersen · 2024-09-11T19:45:03Z

...ud-firestore/src/test-jdk17/java/com/google/cloud/firestore/record/LocalFirestoreHelper.java

Can we remove the record folder, and have this in the parent folder instead?

done. Also removed duplicated code from the file

...ud-firestore/src/test-jdk17/java/com/google/cloud/firestore/RecordDocumentReferenceTest.java

eranl · 2024-09-11T22:08:26Z

Looks good.

My comments are mostly about internal location of code. I feel code is being split across many files, and this has caused some duplication, decrease in encapsulation and code that jumps between files more. I think this could be simplified.

If this can be cleaned up a little now, that would be good. But I don't consider this a blocker to merging. We can followup on this in future PRs if you like.

@tom-andersen, @milaGGL, let me know if you'd like me to take a stab at these comments, either pre- or post-merge.

milaGGL · 2024-09-11T22:27:16Z

Looks good.
My comments are mostly about internal location of code. I feel code is being split across many files, and this has caused some duplication, decrease in encapsulation and code that jumps between files more. I think this could be simplified.
If this can be cleaned up a little now, that would be good. But I don't consider this a blocker to merging. We can followup on this in future PRs if you like.

@tom-andersen, @milaGGL, let me know if you'd like me to take a stab at these comments, either pre- or post-merge.

hi @eranl, I agree with Tom's comments and prefer to address them pre-merge. I will work on the comments tomorrow, but you are very much welcome to help out if you would like to.

eranl · 2024-09-12T13:07:17Z

hi @eranl, I agree with Tom's comments and prefer to address them pre-merge. I will work on the comments tomorrow, but you are very much welcome to help out if you would like to.

Sure. Let me know how I can help.

eranl · 2024-09-12T22:22:15Z

google-cloud-firestore/src/main/java/com/google/cloud/firestore/encoding/CustomClassMapper.java

@@ -38,7 +43,7 @@
 import java.util.concurrent.ConcurrentMap;

 /** Helper class to convert to/from custom POJO classes and plain Java types. */
-class CustomClassMapper {
+public class CustomClassMapper {


Is it a good idea to make this class and the methods below public, exposing them to library users?

I am also hesitating over this.

added "@internalapi" to avoid public use.

eranl · 2024-09-13T13:47:53Z

google-cloud-firestore/src/main/java/com/google/cloud/firestore/encoding/BeanMapper.java

@@ -93,18 +90,6 @@ abstract T deserialize(
      Map<TypeVariable<Class<T>>, Type> types,
      DeserializeContext context);

-  void ensureValidDocumentIdType(String fieldDescription, String operation, Type type) {


Why move these methods from base class to a utility class?

I am moving out all util methods that are not specific to the base class or used in both child classes.

and this is renamed to validateDocumentIdType to match validateServerTimestampType method.

milaGGL · 2024-09-13T16:19:12Z

Have done some code re-format. Please review @tom-andersen, @eranl.

eranl · 2024-09-13T22:49:17Z

google-cloud-firestore/src/main/java/com/google/cloud/firestore/encoding/RecordMapper.java

+    if (constructor == null) {
+      throw context.errorPath.deserializeError(
+          "Class "
+              + getClazz().getName()
+              + " does not define a no-argument constructor. If you are using ProGuard, make "
+              + "sure these constructors are not stripped");
+    }


This check is redundant, since this class can't be instantiated with a null constructor. Also, it's not a no-argument constructor, but the record's canonical one.

That's correct, the constructor should not be null.

eranl · 2024-09-13T23:18:36Z

...ud-firestore/src/test-jdk17/java/com/google/cloud/firestore/RecordDocumentReferenceTest.java

+
+    documentReference.set(ALL_SUPPORTED_TYPES_OBJECT).get();
+
+    CommitRequest expectedCommit = commit(set(ALL_SUPPORTED_TYPES_PROTO));


Since these files are only compile-able with JDK16+ anyway, why not use var?

Explicitly writing out the types is more readable and maintainable. Clarity is preferred over simplicity.

eranl · 2024-09-16T17:04:07Z

Looks good. Thanks for pushing this through.

This reverts commit f5613b4.

eranl and others added 8 commits March 4, 2023 02:34

feat: Add support for java record serialization

28d2e80

Add support for java record serialization - use field annotations

ae46486

Add support for java record serialization - update doc

b50fc25

Add support for java record serialization - fix formatting

d050e24

Merge branch 'googleapis:main' into java-record

de25226

Add support for java record serialization - move test code to google-…

53aabf5

…cloud-firestore module by adding a test-jdk17 directory and a java17 profile, and eliminate google-cloud-firestore-jdk17 module

Merge branch 'googleapis:main' into java-record

846917b

Merge remote-tracking branch 'userRepo/java-record' into mila/test-re…

7725ce9

…cord

product-auto-label bot added size: xl Pull request size is extra large. api: firestore Issues related to the googleapis/java-firestore API. labels Jun 13, 2024

milaGGL changed the title ~~Mila/test record~~ Feat: Record serialization/deserialization Jun 13, 2024

milaGGL added 4 commits August 28, 2024 11:19

Merge branch 'main' into mila/test-record

f7ff7f3

check if CI running the new tests

3fc7a23

check if new pom setup leads to units(8) failure

b6197d5

try ignoring the ToStringTest

b155bee

milaGGL added 14 commits August 28, 2024 15:17

remove var

0bea023

import missing types

91e1282

set javadoc source to java 17

c6fd913

update maven-javadoc-plugin at the root pom.xml

a85a466

Update pom.xml

9e46f9c

remove < release > in pom.xml

5a70439

Update pom.xml

56e3031

remove the build.sh changes

b5a465b

Update pom.xml

646ff81

Revert "Update pom.xml"

9e99d2c

This reverts commit 646ff81.

remove the <maven-compiler-plugin>

3ea41cd

Revert "remove the <maven-compiler-plugin>"

2c3f144

This reverts commit 3ea41cd.

Update build.sh

ec8d30c

Update pom.xml

316df89

format

2f082c4

milaGGL marked this pull request as ready for review September 11, 2024 16:17

milaGGL requested review from a team as code owners September 11, 2024 16:17

tom-andersen self-assigned this Sep 11, 2024

tom-andersen approved these changes Sep 11, 2024

View reviewed changes

milaGGL added 3 commits September 12, 2024 11:30

rename & relocate record test helper file

b51506a

move PojoBeanMapper out, move errors to

d08cd6b

relocate serialization/deserialization files

0af8a9e

eranl reviewed Sep 12, 2024

View reviewed changes

mark @internalapi, move shared code to Util file,

5c94936

eranl reviewed Sep 13, 2024

View reviewed changes

milaGGL added 2 commits September 13, 2024 11:44

mark methods protected, reduce code duplication

48885ce

add class comment for EncodingUtil

ef324e2

milaGGL requested a review from eranl September 13, 2024 16:19

remove EncodingUtil to make methods as private as possible

28465af

eranl reviewed Sep 13, 2024

View reviewed changes

remove redundant constructor null check

382d0aa

milaGGL merged commit f5613b4 into main Sep 16, 2024
24 checks passed

milaGGL deleted the mila/test-record branch September 16, 2024 17:06

release-please bot mentioned this pull request Sep 16, 2024

chore(main): release 3.26.2 #1824

Merged

milaGGL added a commit that referenced this pull request Sep 18, 2024

Revert "Feat: Record serialization/deserialization (#1706)"

148430b

This reverts commit f5613b4.

milaGGL added a commit that referenced this pull request Sep 18, 2024

Revert "Feat: Record serialization/deserialization (#1706)" (#1833)

61073e3

This reverts commit f5613b4.

milaGGL restored the mila/test-record branch September 20, 2024 16:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: Record serialization/deserialization #1706

Feat: Record serialization/deserialization #1706

milaGGL commented Jun 13, 2024 •

edited

Loading

generated-files-bot bot commented Aug 28, 2024 •

edited

Loading

tom-andersen left a comment

tom-andersen Sep 11, 2024

milaGGL Sep 12, 2024 •

edited

Loading

eranl Sep 13, 2024

milaGGL Sep 13, 2024

tom-andersen Sep 11, 2024

milaGGL Sep 12, 2024

eranl commented Sep 11, 2024

milaGGL commented Sep 11, 2024

eranl commented Sep 12, 2024

eranl Sep 12, 2024

milaGGL Sep 12, 2024

milaGGL Sep 12, 2024

eranl Sep 13, 2024

milaGGL Sep 13, 2024

milaGGL Sep 13, 2024

milaGGL commented Sep 13, 2024

eranl Sep 13, 2024

milaGGL Sep 16, 2024

eranl Sep 13, 2024

milaGGL Sep 16, 2024

eranl commented Sep 16, 2024


		documentReference.set(ALL_SUPPORTED_TYPES_OBJECT).get();

		CommitRequest expectedCommit = commit(set(ALL_SUPPORTED_TYPES_PROTO));

Feat: Record serialization/deserialization #1706

Feat: Record serialization/deserialization #1706

Conversation

milaGGL commented Jun 13, 2024 • edited Loading

generated-files-bot bot commented Aug 28, 2024 • edited Loading

tom-andersen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

milaGGL Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eranl commented Sep 11, 2024

milaGGL commented Sep 11, 2024

eranl commented Sep 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

milaGGL commented Sep 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eranl commented Sep 16, 2024

milaGGL commented Jun 13, 2024 •

edited

Loading

generated-files-bot bot commented Aug 28, 2024 •

edited

Loading

milaGGL Sep 12, 2024 •

edited

Loading