Firestore - automatic id generation #2466

dmitry-s · 2020-07-15T22:03:11Z

codecov · 2020-07-15T22:15:57Z

Codecov Report

Merging #2466 into master will decrease coverage by 7.20%.
The diff coverage is 90.00%.

@@             Coverage Diff              @@
##             master    #2466      +/-   ##
============================================
- Coverage     81.29%   74.09%   -7.21%     
+ Complexity     2343     2125     -218     
============================================
  Files           264      265       +1     
  Lines          7663     7678      +15     
  Branches        790      794       +4     
============================================
- Hits           6230     5689     -541     
- Misses         1097     1620     +523     
- Partials        336      369      +33

Flag	Coverage Δ	Complexity Δ
#integration	`?`	`?`
#unittests	`74.09% <90.00%> (+0.03%)`	`2125.00 <6.00> (+6.00)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ	Complexity Δ
...rk/cloud/gcp/data/firestore/FirestoreTemplate.java	`75.00% <85.71%> (-18.80%)`	`52.00 <3.00> (-12.00)`
...ringframework/cloud/gcp/data/firestore/AutoId.java	`100.00% <100.00%> (ø)`	`3.00 <3.00> (?)`
...gcp/secretmanager/SecretManagerPropertySource.java	`0.00% <0.00%> (-100.00%)`	`0.00% <0.00%> (-4.00%)`
...a/spanner/repository/query/SpannerQueryMethod.java	`0.00% <0.00%> (-100.00%)`	`0.00% <0.00%> (-6.00%)`
...retmanager/SecretManagerPropertySourceLocator.java	`0.00% <0.00%> (-100.00%)`	`0.00% <0.00%> (-2.00%)`
...figure/config/GcpConfigBootstrapConfiguration.java	`0.00% <0.00%> (-100.00%)`	`0.00% <0.00%> (-2.00%)`
...e/spanner/GcpSpannerEmulatorAutoConfiguration.java	`0.00% <0.00%> (-100.00%)`	`0.00% <0.00%> (-2.00%)`
...epository/config/SpannerRepositoriesRegistrar.java	`0.00% <0.00%> (-100.00%)`	`0.00% <0.00%> (-3.00%)`
...ository/config/DatastoreRepositoriesRegistrar.java	`0.00% <0.00%> (-100.00%)`	`0.00% <0.00%> (-3.00%)`
...restore/GcpFirestoreEmulatorAutoConfiguration.java	`0.00% <0.00%> (-84.85%)`	`0.00% <0.00%> (-4.00%)`
... and 56 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 853a949...6de9ca1. Read the comment docs.

dzou · 2020-07-15T23:00:37Z

spring-cloud-gcp-data-firestore/pom.xml

@@ -23,6 +23,7 @@
        <dependency>
            <groupId>com.google.cloud</groupId>
            <artifactId>google-cloud-firestore</artifactId>
+			<version>1.35.2-SNAPSHOT</version>


Fix indentation here; tab/space mismatch.

dzou · 2020-07-15T23:04:09Z

...-firestore/src/main/java/org/springframework/cloud/gcp/data/firestore/FirestoreTemplate.java

+		return Flux.from(entities).flatMap(this::create);
+	}
+
+	private <T> Mono<T> create(T entity) {


Would be nice to add a javadoc for this one describing purpose of method something like "Creates a single (non-batched) document in Firestore; ID generation is only available for non-batched created documents."

dzou · 2020-07-15T23:08:04Z

...-firestore/src/main/java/org/springframework/cloud/gcp/data/firestore/FirestoreTemplate.java

-			return ObservableReactiveUtil.streamingBidirectionalCall(
-					this::openWriteStream, inputs, this::buildWriteRequest);
+			return  Flux.from(instances)
+							.groupBy(t -> getIdValue(t) == null).flatMap(groupedFlux ->


Minor nit, would prefer if you can format this section so each operator gets its own line like this:

return Flux.from(instances) .groupBy(t -> getIdValue(t) == null) .flatMap(groupedFlux -> groupedFlux.key() ? create(groupedFlux) : upsert(groupedFlux));

Clever usage of Group By!

dzou · 2020-07-15T23:14:28Z

...-firestore/src/main/java/org/springframework/cloud/gcp/data/firestore/FirestoreTemplate.java

 	}

 	public FirestoreClassMapper getClassMapper() {
 		return this.classMapper;
 	}
+
+	private static class ResourceName {


I see; I would avoid introducing a new class here if possible -

I think if you include it, you will have to write a lot more docs to describe what it means for future readers of this code - i.e. it would at least require a class-level javadoc, and also - generated is too general (i.e. generated what?); you may have to rename to needsGeneratedId or add comment; similarly name is too general as well - i.e. (name of what?)

It's not worth the burden of adding this class in my opinion if it is avoidable.

dzou · 2020-07-15T23:22:48Z

...-firestore/src/main/java/org/springframework/cloud/gcp/data/firestore/FirestoreTemplate.java

-		return Write.newBuilder()
-				.setUpdate(document)
-				.build();
+		ResourceName resourceName = buildResourceName(entity);


I would avoid introducing the ResourceName class to avoid coupling the resource name with whether the id was generated or not. Also mentioned below; I think it's not worth the documentation burden.

Instead, maybe refactor so it's like:

Object idVal = getOrGenerateEntityId(entity); String resourceName = buildResourceName(Class<?> entityType, Object idVal);

meltsufin

Looks good. Just some minor polish suggestions.

meltsufin · 2020-07-17T03:35:29Z

...-firestore/src/main/java/org/springframework/cloud/gcp/data/firestore/FirestoreTemplate.java

+		String resourceName = buildResourceName(entity);
+		Document document = getClassMapper().entityToDocument(entity, resourceName);
+		Builder builder = Write.newBuilder().setUpdate(document);
+		if (needsAutoId) {


I don't really see the point of having this needsAutoId local variable, since it's only used once.
I would just put getIdValue(entity) == null directly in the if.

meltsufin · 2020-07-17T03:37:13Z

...-firestore/src/main/java/org/springframework/cloud/gcp/data/firestore/FirestoreTemplate.java


+			//TODO: replace with Internal.autoId() when it is available


Indicate the package of Internal? Otherwise, it's ambiguous.

meltsufin · 2020-07-17T03:39:04Z

...-firestore/src/main/java/org/springframework/cloud/gcp/data/firestore/FirestoreTemplate.java

 	}

 	private <T> String buildResourceName(T entity) {
 		FirestorePersistentEntity<?> persistentEntity =
 				this.mappingContext.getPersistentEntity(entity.getClass());
 		FirestorePersistentProperty idProperty = persistentEntity.getIdPropertyOrFail();
 		Object idVal = persistentEntity.getPropertyAccessor(entity).getProperty(idProperty);
+		if (idVal == null) {
+			if (idProperty.getType() != String.class) {
+				throw new FirestoreDataException("Automatic ID generation only supported for String type");


Maybe also say that id was null and what its type was to make the exception more clear.

meltsufin · 2020-07-17T03:40:58Z

...-firestore/src/main/java/org/springframework/cloud/gcp/data/firestore/FirestoreTemplate.java

 	}

 	public FirestoreClassMapper getClassMapper() {
 		return this.classMapper;
 	}
+
+	/** Creates a pseudo-random 20-character ID that can be used for Firestore documents. */


Can you indicate where it was copied from?

dzou · 2020-07-17T18:26:18Z

...n/java/org/springframework/cloud/gcp/data/firestore/mapping/FirestoreDefaultClassMapper.java

-				.putAllFields(valuesMap)
-				.setName(documentResourceName).build();
+		Builder builder = Document.newBuilder().putAllFields(valuesMap);
+		if (documentResourceName != null) {


Why is it now possible for documentResourceName to be null?

dzou · 2020-07-17T18:31:49Z

...-firestore/src/main/java/org/springframework/cloud/gcp/data/firestore/FirestoreTemplate.java

+
+	//Creates a pseudo-random 20-character ID that can be used for Firestore documents.
+	//Copied from com.google.cloud.firestore.FirestoreImpl
+	private static String autoId() {


Could you move the Id generating helper method autoId() to a package-private class? maybe something like FirestoreIdGenerator.

This way you can move all the code and fields in there related to ID generation; which allows you to remember what to delete once com.google.cloud.firestore.Internal.autoId() becomes available. Otherwise you will have to comb through the code a little bit more in the future to do this removal.

dzou

Looks pretty good; just some minor nits.

dzou · 2020-07-17T19:21:37Z

...ud-gcp-data-firestore/src/main/java/org/springframework/cloud/gcp/data/firestore/AutoId.java

+import java.security.SecureRandom;
+import java.util.Random;
+
+//Copied from com.google.cloud.firestore.FirestoreImpl


For new classes, add a top level Javadoc :) i.e. with the author tag and brief description etc.

That's why I didn't want to introduce a new class :)

Lol sorry for the inconvenience. At least it will be deleted shortly though! (Once they expose auto id).

dzou · 2020-07-17T19:22:56Z

...n/java/org/springframework/cloud/gcp/data/firestore/mapping/FirestoreDefaultClassMapper.java

-		return Document.newBuilder()
-				.putAllFields(valuesMap)
-				.setName(documentResourceName).build();
+		Builder builder = Document.newBuilder().putAllFields(valuesMap);


Might as well just revert this file then if there are no changes.

elefeint

Looks good from my side.

sonarcloud · 2020-07-20T18:17:21Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
1 Code Smell

92.9% Coverage
0.0% Duplication

The version of Java (1.8.0_151) you have used to run this analysis is deprecated and we will stop accepting it from October 2020. Please update to at least Java 11.
Read more here

addressed

automatic id generation

ff44857

dmitry-s requested review from dzou, meltsufin and elefeint July 15, 2020 22:03

dzou reviewed Jul 15, 2020

View reviewed changes

dmitry-s changed the title ~~automatic id generation~~ Firestore - automatic id generation Jul 16, 2020

dmitry-s added 3 commits July 16, 2020 17:37

switch to write with precondition

f38a658

remove createDocument

0423283

remove ResourceName class

fdee3a3

dmitry-s marked this pull request as ready for review July 16, 2020 22:10

dmitry-s requested a review from dzou July 16, 2020 22:10

improve readability

1869d36

meltsufin previously requested changes Jul 17, 2020

View reviewed changes

dmitry-s added 2 commits July 17, 2020 10:00

PR comments

cd32aee

PR comments

947d6f9

dzou reviewed Jul 17, 2020

View reviewed changes

PR comments

b35e69b

dmitry-s requested review from dzou and meltsufin July 17, 2020 19:03

dzou reviewed Jul 17, 2020

View reviewed changes

elefeint approved these changes Jul 20, 2020

View reviewed changes

dmitry-s added 2 commits July 20, 2020 13:52

PR comments

b86fc0a

PR comments

6de9ca1

dmitry-s requested review from dzou and elefeint July 20, 2020 19:37

dzou approved these changes Jul 20, 2020

View reviewed changes

dmitry-s merged commit e6def07 into master Jul 20, 2020

dmitry-s deleted the auto-id-generation branch July 20, 2020 20:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Firestore - automatic id generation #2466

Firestore - automatic id generation #2466

dmitry-s commented Jul 15, 2020

codecov bot commented Jul 15, 2020 •

edited

Loading

dzou Jul 15, 2020

dzou Jul 15, 2020

dzou Jul 15, 2020 •

edited

Loading

dzou Jul 15, 2020

dzou Jul 15, 2020

meltsufin left a comment

meltsufin Jul 17, 2020

meltsufin Jul 17, 2020

meltsufin Jul 17, 2020

meltsufin Jul 17, 2020

dzou Jul 17, 2020

dzou Jul 17, 2020

dzou left a comment

dzou Jul 17, 2020

dmitry-s Jul 17, 2020

dzou Jul 20, 2020

dzou Jul 17, 2020

elefeint left a comment

sonarcloud bot commented Jul 20, 2020

Firestore - automatic id generation #2466

Firestore - automatic id generation #2466

Conversation

dmitry-s commented Jul 15, 2020

codecov bot commented Jul 15, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dzou Jul 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meltsufin left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dzou left a 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

elefeint left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Jul 20, 2020

codecov bot commented Jul 15, 2020 •

edited

Loading

dzou Jul 15, 2020 •

edited

Loading