From 371e0888659505b62be8ee58561f0d6fc215bb4e Mon Sep 17 00:00:00 2001 From: Hui Wu Date: Tue, 21 May 2019 16:42:43 -0400 Subject: [PATCH 01/15] add more instructions --- firebase-firestore/README.md | 70 +++++++++++++++++++++++++++++++++++- 1 file changed, 69 insertions(+), 1 deletion(-) diff --git a/firebase-firestore/README.md b/firebase-firestore/README.md index 3260973abc0..2e3183b91a1 100644 --- a/firebase-firestore/README.md +++ b/firebase-firestore/README.md @@ -10,12 +10,80 @@ responsive apps that work regardless of network latency or Internet connectivity. Cloud Firestore also offers seamless integration with other Firebase and Google Cloud Platform products, including Cloud Functions. +## Building + All Gradle commands should be run from the source root (which is one level up from this folder). See the README.md in the source root for instructions on publishing/testing Cloud Firestore. -## Testing +To build Cloud Firestore, from the source root run: +```bash +./gradlew :firebase-firestore:assembleRelease +``` + +## Unit Testing + +To run unit tests for Cloud Firestore, from the source root run: +```bash +./gradlew :firebase-firestore:check +``` + +## Integration Testing + +Running integration tests requires a Firebase project because they would try +to connect to the Firestore backends. + +See [here](../README.md#project-setup) for how to setup a project. + +Once you setup the project, download `google-services.json` and place it in +the source root. + +### Run on Local Emulator + +Make sure you have created a Firestore instance for your project. + +Then simply run: +```bash +./gradlew :firebase-firestore:connectedCheck +``` + +### Run on Firebase Test Lab + +You can also test on Firebase Test Lab, which allow you to run the integration +tests on devices hosted in Google data center. + +See [here](../README.md#running-integration-tests-on-firebase-test-lab) for +instructions of how to setup Firebase Test Lab for your project. + +Run: +```bash +./gradlew :firebase-firestore:deviceCheck +``` + +## Code Formatting + +Run below to format Java code: +```bash +./gradlew :firebase-firestore:googleJavaFormat +``` + +See [here](../README.md#code-formatting) if you want to be able to format code +from within Android Studio. + +## Build Local Jar of Firestore SDK + +Run: +```bash +./gradlew -PprojectsToPublish="firebase-firestore" firebasePublish +``` + +This will build an `m2repository.zip` under `build/`, which contains a locally +built Firestore SDK. + +This way you can pick up updates/fixes before official releases. + +## Misc After importing the project into Android Studio and building successfully for the first time, Android Studio will delete the run configuration xml files in `./idea/runConfigurations`. Undo these changes with the command: From c659bae53e1f806e6ed97cf0cdb32bf317c24202 Mon Sep 17 00:00:00 2001 From: Hui Wu Date: Tue, 21 May 2019 16:50:35 -0400 Subject: [PATCH 02/15] more document --- firebase-firestore/README.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/firebase-firestore/README.md b/firebase-firestore/README.md index 2e3183b91a1..da71cbe947f 100644 --- a/firebase-firestore/README.md +++ b/firebase-firestore/README.md @@ -38,9 +38,10 @@ See [here](../README.md#project-setup) for how to setup a project. Once you setup the project, download `google-services.json` and place it in the source root. -### Run on Local Emulator +Make sure you have created a Firestore instance for your project, before +you proceed. -Make sure you have created a Firestore instance for your project. +### Run on Local Emulator Then simply run: ```bash @@ -77,7 +78,7 @@ Run: ./gradlew -PprojectsToPublish="firebase-firestore" firebasePublish ``` -This will build an `m2repository.zip` under `build/`, which contains a locally +This will build a `m2repository.zip` under `build/`, which contains a locally built Firestore SDK. This way you can pick up updates/fixes before official releases. From 2923e35b86091c314fea2477af9bcdf9bd5a4288 Mon Sep 17 00:00:00 2001 From: Hui Wu Date: Wed, 22 May 2019 11:49:11 -0400 Subject: [PATCH 03/15] Add DocumentId annotation, and mark neccessary interface/call site updates. --- .../firestore/CollectionReference.java | 3 +- .../google/firebase/firestore/DocumentId.java | 43 +++++++++++++++++++ .../firebase/firestore/DocumentReference.java | 13 ++++++ .../firebase/firestore/Transaction.java | 1 + .../google/firebase/firestore/WriteBatch.java | 1 + .../firestore/util/CustomClassMapper.java | 10 +++++ 6 files changed, 70 insertions(+), 1 deletion(-) create mode 100644 firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentId.java diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/CollectionReference.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/CollectionReference.java index c8364f7a76f..f9aaa4c7bd0 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/CollectionReference.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/CollectionReference.java @@ -125,7 +125,8 @@ public DocumentReference document(@NonNull String documentPath) { public Task add(@NonNull Object data) { checkNotNull(data, "Provided data must not be null."); final DocumentReference ref = document(); - return ref.set(data) + // TODO: Assert fields annotated with DocumentId in `data` are set to null. + return ref.setForAddition(data) .continueWith( Executors.DIRECT_EXECUTOR, task -> { diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentId.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentId.java new file mode 100644 index 00000000000..8a60087da01 --- /dev/null +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentId.java @@ -0,0 +1,43 @@ +// Copyright 2019 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.firebase.firestore; + +import com.google.firebase.annotations.PublicApi; +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +/** + * Annotation used to mark an object field to be the ID of a firestore document. + * + *

This annotation is recognized by {@link com.google.firebase.firestore.util.CustomClassMapper}. + * + *

During conversions from documents to java objects, fields with this annotation will be + * populated with the document ID being converted. + * + *

When objects with the annotation is used to create new documents, its field value must be null + * to guarantee uniqueness, otherwise a runtime exception will be thrown. + * + *

When objects with the annotation is used to update documents, its field value must match the + * target documents, otherwise a runtime exception will be thrown. + * + *

This annotation can only be applied to fields of String or {@link DocumentReference}, + * otherwise a runtime exception will be thrown. + */ +@PublicApi +@Retention(RetentionPolicy.RUNTIME) +@Target({ElementType.FIELD}) +public @interface DocumentId {} diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentReference.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentReference.java index 1d54149a68a..a39104a094a 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentReference.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentReference.java @@ -154,6 +154,13 @@ public Task set(@NonNull Object data) { return set(data, SetOptions.OVERWRITE); } + // Handles specific logic for document addition before entering comment `set` logic. + /* package */ Task setForAddition(@NonNull Object data) { + checkNotNull(data, "Provided data must not be null."); + // TODO: Assert fields annotated with DocumentId in `data` are set to null. + return setImpl(data, SetOptions.OVERWRITE); + } + /** * Writes to the document referred to by this DocumentReference. If the document does not yet * exist, it will be created. If you pass {@link SetOptions}, the provided data can be merged into @@ -169,6 +176,12 @@ public Task set(@NonNull Object data) { public Task set(@NonNull Object data, @NonNull SetOptions options) { checkNotNull(data, "Provided data must not be null."); checkNotNull(options, "Provided options must not be null."); + // TODO: Assert fields annotated with DocumentId in `data` match `documentRef`. + return setImpl(data, options); + } + + @NonNull + private Task setImpl(@NonNull Object data, @NonNull SetOptions options) { ParsedSetData parsed = options.isMerge() ? firestore.getDataConverter().parseMergeData(data, options.getFieldMask()) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/Transaction.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/Transaction.java index 8951d3c4b08..cf800e3430f 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/Transaction.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/Transaction.java @@ -88,6 +88,7 @@ public Transaction set( firestore.validateReference(documentRef); checkNotNull(data, "Provided data must not be null."); checkNotNull(options, "Provided options must not be null."); + // TODO: Assert fields annotated with DocumentId in `data` match `documentRef`. ParsedSetData parsed = options.isMerge() ? firestore.getDataConverter().parseMergeData(data, options.getFieldMask()) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/WriteBatch.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/WriteBatch.java index 21e368ef8fb..8d26c0ab819 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/WriteBatch.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/WriteBatch.java @@ -87,6 +87,7 @@ public WriteBatch set( firestore.validateReference(documentRef); checkNotNull(data, "Provided data must not be null."); checkNotNull(options, "Provided options must not be null."); + // TODO: Assert fields annotated with DocumentId in `data` match `documentRef`. verifyNotCommitted(); ParsedSetData parsed = options.isMerge() diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/util/CustomClassMapper.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/util/CustomClassMapper.java index a8abd7e3635..1cde1271097 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/util/CustomClassMapper.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/util/CustomClassMapper.java @@ -48,6 +48,7 @@ import java.util.Map; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; +import javax.annotation.Nullable; /** Helper class to convert to/from custom POJO classes and plain Java types. */ public class CustomClassMapper { @@ -97,6 +98,15 @@ public static T convertToCustomClass(Object object, Class clazz) { return deserializeToClass(object, clazz, ErrorPath.EMPTY); } + /** + * If there are fields annotated with {@link com.google.firebase.firestore.DocumentId}, make sure + * they are set to expected value by throwing runtime exception when it fails to comply. + * + * @param object Java object that might have DocumentId annotated fields. + * @param expected Expected value of the DocumentId annotated fields. + */ + public static void assertAnnotatedDocumentIdEqual(Object object, @Nullable Object expected) {} + private static Object serialize(T o) { return serialize(o, ErrorPath.EMPTY); } From ec067eda9ae6830ce270e3198e88c2e45afd6b1c Mon Sep 17 00:00:00 2001 From: Hui Wu Date: Wed, 22 May 2019 11:56:15 -0400 Subject: [PATCH 04/15] undo readme change --- firebase-firestore/README.md | 71 +----------------------------------- 1 file changed, 1 insertion(+), 70 deletions(-) diff --git a/firebase-firestore/README.md b/firebase-firestore/README.md index da71cbe947f..3260973abc0 100644 --- a/firebase-firestore/README.md +++ b/firebase-firestore/README.md @@ -10,81 +10,12 @@ responsive apps that work regardless of network latency or Internet connectivity. Cloud Firestore also offers seamless integration with other Firebase and Google Cloud Platform products, including Cloud Functions. -## Building - All Gradle commands should be run from the source root (which is one level up from this folder). See the README.md in the source root for instructions on publishing/testing Cloud Firestore. -To build Cloud Firestore, from the source root run: -```bash -./gradlew :firebase-firestore:assembleRelease -``` - -## Unit Testing - -To run unit tests for Cloud Firestore, from the source root run: -```bash -./gradlew :firebase-firestore:check -``` - -## Integration Testing - -Running integration tests requires a Firebase project because they would try -to connect to the Firestore backends. - -See [here](../README.md#project-setup) for how to setup a project. - -Once you setup the project, download `google-services.json` and place it in -the source root. - -Make sure you have created a Firestore instance for your project, before -you proceed. - -### Run on Local Emulator - -Then simply run: -```bash -./gradlew :firebase-firestore:connectedCheck -``` - -### Run on Firebase Test Lab - -You can also test on Firebase Test Lab, which allow you to run the integration -tests on devices hosted in Google data center. - -See [here](../README.md#running-integration-tests-on-firebase-test-lab) for -instructions of how to setup Firebase Test Lab for your project. - -Run: -```bash -./gradlew :firebase-firestore:deviceCheck -``` - -## Code Formatting - -Run below to format Java code: -```bash -./gradlew :firebase-firestore:googleJavaFormat -``` - -See [here](../README.md#code-formatting) if you want to be able to format code -from within Android Studio. - -## Build Local Jar of Firestore SDK - -Run: -```bash -./gradlew -PprojectsToPublish="firebase-firestore" firebasePublish -``` - -This will build a `m2repository.zip` under `build/`, which contains a locally -built Firestore SDK. - -This way you can pick up updates/fixes before official releases. - +## Testing -## Misc After importing the project into Android Studio and building successfully for the first time, Android Studio will delete the run configuration xml files in `./idea/runConfigurations`. Undo these changes with the command: From 1533c5e8bfa4479e5fc978ec0bbf808d7066a0e0 Mon Sep 17 00:00:00 2001 From: Hui Wu Date: Wed, 22 May 2019 16:28:46 -0400 Subject: [PATCH 05/15] making it non-static --- .../firestore/CollectionReference.java | 3 +- .../google/firebase/firestore/DocumentId.java | 17 ++-- .../firebase/firestore/DocumentReference.java | 17 +--- .../firebase/firestore/DocumentSnapshot.java | 10 ++- .../firebase/firestore/Transaction.java | 7 +- .../firebase/firestore/UserDataConverter.java | 35 +++++--- .../google/firebase/firestore/WriteBatch.java | 7 +- .../firestore/util/CustomClassMapper.java | 43 +++++----- .../firebase/firestore/util/MapperTest.java | 79 +++++++++++-------- 9 files changed, 123 insertions(+), 95 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/CollectionReference.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/CollectionReference.java index f9aaa4c7bd0..c8364f7a76f 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/CollectionReference.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/CollectionReference.java @@ -125,8 +125,7 @@ public DocumentReference document(@NonNull String documentPath) { public Task add(@NonNull Object data) { checkNotNull(data, "Provided data must not be null."); final DocumentReference ref = document(); - // TODO: Assert fields annotated with DocumentId in `data` are set to null. - return ref.setForAddition(data) + return ref.set(data) .continueWith( Executors.DIRECT_EXECUTOR, task -> { diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentId.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentId.java index 8a60087da01..6d84fd9d3db 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentId.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentId.java @@ -21,21 +21,14 @@ import java.lang.annotation.Target; /** - * Annotation used to mark an object field to be the ID of a firestore document. - * - *

This annotation is recognized by {@link com.google.firebase.firestore.util.CustomClassMapper}. - * - *

During conversions from documents to java objects, fields with this annotation will be - * populated with the document ID being converted. - * - *

When objects with the annotation is used to create new documents, its field value must be null - * to guarantee uniqueness, otherwise a runtime exception will be thrown. - * - *

When objects with the annotation is used to update documents, its field value must match the - * target documents, otherwise a runtime exception will be thrown. + * Annotation used to mark a POJO field to be automatically populated with the document's ID when + * the POJO is created from a Firestore document (e.g. via {@link DocumentSnapshot#toObject}). * *

This annotation can only be applied to fields of String or {@link DocumentReference}, * otherwise a runtime exception will be thrown. + * + *

When writing a POJO to Firestore, the @DocumentId-annotated field must either be null or match + * the document ID of the document being written to, else a runtime exception will be thrown. */ @PublicApi @Retention(RetentionPolicy.RUNTIME) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentReference.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentReference.java index a39104a094a..f52b3c536ce 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentReference.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentReference.java @@ -154,13 +154,6 @@ public Task set(@NonNull Object data) { return set(data, SetOptions.OVERWRITE); } - // Handles specific logic for document addition before entering comment `set` logic. - /* package */ Task setForAddition(@NonNull Object data) { - checkNotNull(data, "Provided data must not be null."); - // TODO: Assert fields annotated with DocumentId in `data` are set to null. - return setImpl(data, SetOptions.OVERWRITE); - } - /** * Writes to the document referred to by this DocumentReference. If the document does not yet * exist, it will be created. If you pass {@link SetOptions}, the provided data can be merged into @@ -176,16 +169,10 @@ public Task set(@NonNull Object data) { public Task set(@NonNull Object data, @NonNull SetOptions options) { checkNotNull(data, "Provided data must not be null."); checkNotNull(options, "Provided options must not be null."); - // TODO: Assert fields annotated with DocumentId in `data` match `documentRef`. - return setImpl(data, options); - } - - @NonNull - private Task setImpl(@NonNull Object data, @NonNull SetOptions options) { ParsedSetData parsed = options.isMerge() - ? firestore.getDataConverter().parseMergeData(data, options.getFieldMask()) - : firestore.getDataConverter().parseSetData(data); + ? firestore.getDataConverter().parseMergeData(data, options.getFieldMask(), getId()) + : firestore.getDataConverter().parseSetData(data, getId()); return firestore .getClient() .write(parsed.toMutationList(key, Precondition.NONE)) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentSnapshot.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentSnapshot.java index b5ec5fab564..f709428c4a9 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentSnapshot.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentSnapshot.java @@ -200,7 +200,10 @@ public T toObject( checkNotNull( serverTimestampBehavior, "Provided serverTimestampBehavior value must not be null."); Map data = getData(serverTimestampBehavior); - return data == null ? null : CustomClassMapper.convertToCustomClass(data, valueType); + return data == null + ? null + : CustomClassMapper.withDocumentId(key.getPath().getLastSegment()) + .convertToCustomClass(data, valueType); } /** @@ -353,7 +356,10 @@ public T get( @NonNull Class valueType, @NonNull ServerTimestampBehavior serverTimestampBehavior) { Object data = get(fieldPath, serverTimestampBehavior); - return data == null ? null : CustomClassMapper.convertToCustomClass(data, valueType); + return data == null + ? null + : CustomClassMapper.withDocumentId(key.getPath().getLastSegment()) + .convertToCustomClass(data, valueType); } /** diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/Transaction.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/Transaction.java index cf800e3430f..4189e2d934f 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/Transaction.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/Transaction.java @@ -88,11 +88,12 @@ public Transaction set( firestore.validateReference(documentRef); checkNotNull(data, "Provided data must not be null."); checkNotNull(options, "Provided options must not be null."); - // TODO: Assert fields annotated with DocumentId in `data` match `documentRef`. ParsedSetData parsed = options.isMerge() - ? firestore.getDataConverter().parseMergeData(data, options.getFieldMask()) - : firestore.getDataConverter().parseSetData(data); + ? firestore + .getDataConverter() + .parseMergeData(data, options.getFieldMask(), documentRef.getId()) + : firestore.getDataConverter().parseSetData(data, documentRef.getId()); transaction.set(documentRef.getKey(), parsed); return this; } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/UserDataConverter.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/UserDataConverter.java index 15726d97d3a..6ad6d9dc8ba 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/UserDataConverter.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/UserDataConverter.java @@ -73,17 +73,33 @@ public UserDataConverter(DatabaseId databaseId) { this.databaseId = databaseId; } - /** Parse document data from a non-merge set() call. */ - public ParsedSetData parseSetData(Object input) { + /** + * Parse document data from a non-merge set() call. + * + * @param input A POJO object representing document data. + * @param expectedId If `input` has any fields annotated with {@link DocumentId}, they should + * either be null or match this parameter. + */ + public ParsedSetData parseSetData(Object input, String expectedId) { ParseAccumulator accumulator = new ParseAccumulator(UserData.Source.Set); - ObjectValue updateData = convertAndParseDocumentData(input, accumulator.rootContext()); + ObjectValue updateData = + convertAndParseDocumentData(input, accumulator.rootContext(), expectedId); return accumulator.toSetData(updateData); } - /** Parse document data from a set() call with SetOptions.merge() set. */ - public ParsedSetData parseMergeData(Object input, @Nullable FieldMask fieldMask) { + /** + * Parse document data from a set() call with SetOptions.merge() set. + * + * @param input A POJO object representing document data. + * @param fieldMask A {@link FieldMask} object representing the fields to be merged. + * @param expectedId If `input` has any fields annotated with {@link DocumentId}, they should + * either be null or match this parameter. + */ + public ParsedSetData parseMergeData( + Object input, @Nullable FieldMask fieldMask, String expectedId) { ParseAccumulator accumulator = new ParseAccumulator(UserData.Source.MergeSet); - ObjectValue updateData = convertAndParseDocumentData(input, accumulator.rootContext()); + ObjectValue updateData = + convertAndParseDocumentData(input, accumulator.rootContext(), expectedId); if (fieldMask != null) { // Verify that all elements specified in the field mask are part of the parsed context. @@ -198,7 +214,7 @@ public FieldValue parseQueryValue(Object input) { /** Converts a POJO to native types and then parses it into model types. */ private FieldValue convertAndParseFieldData(Object input, ParseContext context) { - Object converted = CustomClassMapper.convertToPlainJavaTypes(input); + Object converted = CustomClassMapper.withDocumentId(null).convertToPlainJavaTypes(input); return parseData(converted, context); } @@ -207,7 +223,8 @@ private FieldValue convertAndParseFieldData(Object input, ParseContext context) * conform to document data (i.e. it must parse into an ObjectValue model type) and will throw an * appropriate error otherwise. */ - private ObjectValue convertAndParseDocumentData(Object input, ParseContext context) { + private ObjectValue convertAndParseDocumentData( + Object input, ParseContext context, String expectedId) { String badDocReason = "Invalid data. Data must be a Map or a suitable POJO object, but it was "; @@ -217,7 +234,7 @@ private ObjectValue convertAndParseDocumentData(Object input, ParseContext conte throw new IllegalArgumentException(badDocReason + "an array"); } - Object converted = CustomClassMapper.convertToPlainJavaTypes(input); + Object converted = CustomClassMapper.withDocumentId(expectedId).convertToPlainJavaTypes(input); FieldValue value = parseData(converted, context); if (!(value instanceof ObjectValue)) { diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/WriteBatch.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/WriteBatch.java index 8d26c0ab819..5813406ec59 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/WriteBatch.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/WriteBatch.java @@ -87,12 +87,13 @@ public WriteBatch set( firestore.validateReference(documentRef); checkNotNull(data, "Provided data must not be null."); checkNotNull(options, "Provided options must not be null."); - // TODO: Assert fields annotated with DocumentId in `data` match `documentRef`. verifyNotCommitted(); ParsedSetData parsed = options.isMerge() - ? firestore.getDataConverter().parseMergeData(data, options.getFieldMask()) - : firestore.getDataConverter().parseSetData(data); + ? firestore + .getDataConverter() + .parseMergeData(data, options.getFieldMask(), documentRef.getId()) + : firestore.getDataConverter().parseSetData(data, documentRef.getId()); mutations.addAll(parsed.toMutationList(documentRef.getKey(), Precondition.NONE)); return this; } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/util/CustomClassMapper.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/util/CustomClassMapper.java index 1cde1271097..7707ae9ac94 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/util/CustomClassMapper.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/util/CustomClassMapper.java @@ -48,7 +48,6 @@ import java.util.Map; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; -import javax.annotation.Nullable; /** Helper class to convert to/from custom POJO classes and plain Java types. */ public class CustomClassMapper { @@ -67,6 +66,23 @@ private static void hardAssert(boolean assertion, String message) { } } + /** + * Returns a mapper with a given document ID. The document ID will be used to populate the {@link + * com.google.firebase.firestore.DocumentId} annotated fields of the POJO of the custom class; + * when converting a POJO to plain Java type, the @DocumentId-annotated fields should be either + * null or match the given document ID. + */ + // TODO: actually implement documentId population/validation logic during (de)serialization. + public static CustomClassMapper withDocumentId(String documentId) { + return new CustomClassMapper(documentId); + } + + private CustomClassMapper(String documentId) { + this.documentId = documentId; + } + + private String documentId = null; + /** * Converts a Java representation of JSON data to standard library Java data types: Map, Array, * String, Double, Integer and Boolean. POJOs are converted to Java Maps. @@ -74,11 +90,11 @@ private static void hardAssert(boolean assertion, String message) { * @param object The representation of the JSON data * @return JSON representation containing only standard library Java types */ - public static Object convertToPlainJavaTypes(Object object) { + public Object convertToPlainJavaTypes(Object object) { return serialize(object); } - public static Map convertToPlainJavaTypes(Map update) { + public Map convertToPlainJavaTypes(Map update) { Object converted = serialize(update); hardAssert(converted instanceof Map); @SuppressWarnings("unchecked") @@ -94,25 +110,16 @@ public static Map convertToPlainJavaTypes(Map update) * @param clazz The class of the object to convert to * @return The POJO object. */ - public static T convertToCustomClass(Object object, Class clazz) { + public T convertToCustomClass(Object object, Class clazz) { return deserializeToClass(object, clazz, ErrorPath.EMPTY); } - /** - * If there are fields annotated with {@link com.google.firebase.firestore.DocumentId}, make sure - * they are set to expected value by throwing runtime exception when it fails to comply. - * - * @param object Java object that might have DocumentId annotated fields. - * @param expected Expected value of the DocumentId annotated fields. - */ - public static void assertAnnotatedDocumentIdEqual(Object object, @Nullable Object expected) {} - - private static Object serialize(T o) { + private Object serialize(T o) { return serialize(o, ErrorPath.EMPTY); } @SuppressWarnings("unchecked") - private static Object serialize(T o, ErrorPath path) { + private Object serialize(T o, ErrorPath path) { if (path.getLength() > MAX_DEPTH) { throw serializeError( path, @@ -182,7 +189,7 @@ private static Object serialize(T o, ErrorPath path) { } else { Class clazz = (Class) o.getClass(); BeanMapper mapper = loadOrCreateBeanMapperForClass(clazz); - return mapper.serialize(o, path); + return mapper.serialize(o, path, this); } } @@ -763,7 +770,7 @@ private Type resolveType(Type type, Map>, Type> types) { } } - Map serialize(T object, ErrorPath path) { + Map serialize(T object, ErrorPath path, CustomClassMapper classMapper) { if (!clazz.isAssignableFrom(object.getClass())) { throw new IllegalArgumentException( "Can't serialize object of class " @@ -795,7 +802,7 @@ Map serialize(T object, ErrorPath path) { // Replace null ServerTimestamp-annotated fields with the sentinel. serializedValue = FieldValue.serverTimestamp(); } else { - serializedValue = CustomClassMapper.serialize(propertyValue, path.child(property)); + serializedValue = classMapper.serialize(propertyValue, path.child(property)); } result.put(property, serializedValue); } diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/util/MapperTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/util/MapperTest.java index ce62718a4ca..297bcfc846f 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/util/MapperTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/util/MapperTest.java @@ -899,11 +899,11 @@ public void setValue(String value) { private static T deserialize(String jsonString, Class clazz) { Map json = fromSingleQuotedString(jsonString); - return CustomClassMapper.convertToCustomClass(json, clazz); + return CustomClassMapper.withDocumentId(null).convertToCustomClass(json, clazz); } private static Object serialize(Object object) { - return CustomClassMapper.convertToPlainJavaTypes(object); + return CustomClassMapper.withDocumentId(null).convertToPlainJavaTypes(object); } private static void assertJson(String expected, Object actual) { @@ -1340,7 +1340,8 @@ public void beansCanContainUpperBoundedMaps() { Date date = new Date(1491847082123L); Map source = map("values", map("foo", date)); UpperBoundedMapBean bean = - CustomClassMapper.convertToCustomClass(source, UpperBoundedMapBean.class); + CustomClassMapper.withDocumentId(null) + .convertToCustomClass(source, UpperBoundedMapBean.class); Map expected = map("foo", date); assertEquals(expected, bean.values); } @@ -1350,7 +1351,8 @@ public void beansCanContainMultiBoundedMaps() { Date date = new Date(1491847082123L); Map source = map("map", map("values", map("foo", date))); MultiBoundedMapHolderBean bean = - CustomClassMapper.convertToCustomClass(source, MultiBoundedMapHolderBean.class); + CustomClassMapper.withDocumentId(null) + .convertToCustomClass(source, MultiBoundedMapHolderBean.class); Map expected = map("foo", date); assertEquals(expected, bean.map.values); @@ -1367,7 +1369,8 @@ public void beansCanContainUnboundedMaps() { public void beansCanContainUnboundedTypeVariableMaps() { Map source = map("map", map("values", map("foo", "bar"))); UnboundedTypeVariableMapHolderBean bean = - CustomClassMapper.convertToCustomClass(source, UnboundedTypeVariableMapHolderBean.class); + CustomClassMapper.withDocumentId(null) + .convertToCustomClass(source, UnboundedTypeVariableMapHolderBean.class); Map expected = map("foo", "bar"); assertEquals(expected, bean.map.values); @@ -1823,24 +1826,36 @@ public void objectAcceptsAnyObject() { @Test public void objectClassCanBePassedInAtTopLevel() { - assertEquals("foo", CustomClassMapper.convertToCustomClass("foo", Object.class)); - assertEquals(1, CustomClassMapper.convertToCustomClass(1, Object.class)); - assertEquals(1L, CustomClassMapper.convertToCustomClass(1L, Object.class)); - assertEquals(true, CustomClassMapper.convertToCustomClass(true, Object.class)); - assertEquals(1.1, CustomClassMapper.convertToCustomClass(1.1, Object.class)); + assertEquals( + "foo", CustomClassMapper.withDocumentId(null).convertToCustomClass("foo", Object.class)); + assertEquals(1, CustomClassMapper.withDocumentId(null).convertToCustomClass(1, Object.class)); + assertEquals(1L, CustomClassMapper.withDocumentId(null).convertToCustomClass(1L, Object.class)); + assertEquals( + true, CustomClassMapper.withDocumentId(null).convertToCustomClass(true, Object.class)); + assertEquals( + 1.1, CustomClassMapper.withDocumentId(null).convertToCustomClass(1.1, Object.class)); List fooList = Collections.singletonList("foo"); - assertEquals(fooList, CustomClassMapper.convertToCustomClass(fooList, Object.class)); + assertEquals( + fooList, + CustomClassMapper.withDocumentId(null).convertToCustomClass(fooList, Object.class)); Map fooMap = Collections.singletonMap("foo", "bar"); - assertEquals(fooMap, CustomClassMapper.convertToCustomClass(fooMap, Object.class)); + assertEquals( + fooMap, CustomClassMapper.withDocumentId(null).convertToCustomClass(fooMap, Object.class)); } @Test public void primitiveClassesCanBePassedInTopLevel() { - assertEquals("foo", CustomClassMapper.convertToCustomClass("foo", String.class)); - assertEquals((Integer) 1, CustomClassMapper.convertToCustomClass(1, Integer.class)); - assertEquals((Long) 1L, CustomClassMapper.convertToCustomClass(1L, Long.class)); - assertEquals(true, CustomClassMapper.convertToCustomClass(true, Boolean.class)); - assertEquals((Double) 1.1, CustomClassMapper.convertToCustomClass(1.1, Double.class)); + assertEquals( + "foo", CustomClassMapper.withDocumentId(null).convertToCustomClass("foo", String.class)); + assertEquals( + (Integer) 1, CustomClassMapper.withDocumentId(null).convertToCustomClass(1, Integer.class)); + assertEquals( + (Long) 1L, CustomClassMapper.withDocumentId(null).convertToCustomClass(1L, Long.class)); + assertEquals( + true, CustomClassMapper.withDocumentId(null).convertToCustomClass(true, Boolean.class)); + assertEquals( + (Double) 1.1, + CustomClassMapper.withDocumentId(null).convertToCustomClass(1.1, Double.class)); } @Test @@ -1848,7 +1863,9 @@ public void passingInListTopLevelThrows() { assertExceptionContains( "Class java.util.List has generic type parameters, please use GenericTypeIndicator " + "instead", - () -> CustomClassMapper.convertToCustomClass(Collections.singletonList("foo"), List.class)); + () -> + CustomClassMapper.withDocumentId(null) + .convertToCustomClass(Collections.singletonList("foo"), List.class)); } @Test @@ -1857,29 +1874,29 @@ public void passingInMapTopLevelThrows() { "Class java.util.Map has generic type parameters, please use GenericTypeIndicator " + "instead", () -> - CustomClassMapper.convertToCustomClass( - Collections.singletonMap("foo", "bar"), Map.class)); + CustomClassMapper.withDocumentId(null) + .convertToCustomClass(Collections.singletonMap("foo", "bar"), Map.class)); } @Test public void passingInCharacterTopLevelThrows() { assertExceptionContains( "Deserializing values to Character is not supported", - () -> CustomClassMapper.convertToCustomClass('1', Character.class)); + () -> CustomClassMapper.withDocumentId(null).convertToCustomClass('1', Character.class)); } @Test public void passingInShortTopLevelThrows() { assertExceptionContains( "Deserializing values to Short is not supported", - () -> CustomClassMapper.convertToCustomClass(1, Short.class)); + () -> CustomClassMapper.withDocumentId(null).convertToCustomClass(1, Short.class)); } @Test public void passingInByteTopLevelThrows() { assertExceptionContains( "Deserializing values to Byte is not supported", - () -> CustomClassMapper.convertToCustomClass(1, Byte.class)); + () -> CustomClassMapper.withDocumentId(null).convertToCustomClass(1, Byte.class)); } @Test @@ -1916,13 +1933,13 @@ public void collectionsCantBeDeserialized() { @Test public void allowNullEverywhere() { - assertNull(CustomClassMapper.convertToCustomClass(null, Integer.class)); - assertNull(CustomClassMapper.convertToCustomClass(null, String.class)); - assertNull(CustomClassMapper.convertToCustomClass(null, Double.class)); - assertNull(CustomClassMapper.convertToCustomClass(null, Long.class)); - assertNull(CustomClassMapper.convertToCustomClass(null, Boolean.class)); - assertNull(CustomClassMapper.convertToCustomClass(null, StringBean.class)); - assertNull(CustomClassMapper.convertToCustomClass(null, Object.class)); + assertNull(CustomClassMapper.withDocumentId(null).convertToCustomClass(null, Integer.class)); + assertNull(CustomClassMapper.withDocumentId(null).convertToCustomClass(null, String.class)); + assertNull(CustomClassMapper.withDocumentId(null).convertToCustomClass(null, Double.class)); + assertNull(CustomClassMapper.withDocumentId(null).convertToCustomClass(null, Long.class)); + assertNull(CustomClassMapper.withDocumentId(null).convertToCustomClass(null, Boolean.class)); + assertNull(CustomClassMapper.withDocumentId(null).convertToCustomClass(null, StringBean.class)); + assertNull(CustomClassMapper.withDocumentId(null).convertToCustomClass(null, Object.class)); } @Test @@ -2233,7 +2250,7 @@ public void deserializationFailureIncludesPath() { Object serialized = Collections.singletonMap("value", (short) 1); try { - CustomClassMapper.convertToCustomClass(serialized, ShortBean.class); + CustomClassMapper.withDocumentId(null).convertToCustomClass(serialized, ShortBean.class); fail("should have thrown"); } catch (RuntimeException e) { assertEquals( From c7d95083481bd772f389f3375a4720dcc9819fd0 Mon Sep 17 00:00:00 2001 From: Hui Wu Date: Thu, 23 May 2019 10:26:00 -0400 Subject: [PATCH 06/15] Make it back to static --- .../firebase/firestore/DocumentReference.java | 4 +- .../firebase/firestore/DocumentSnapshot.java | 6 +- .../firebase/firestore/Transaction.java | 6 +- .../firebase/firestore/UserDataConverter.java | 22 ++--- .../google/firebase/firestore/WriteBatch.java | 6 +- .../firestore/util/CustomClassMapper.java | 51 ++++++------ .../firebase/firestore/util/MapperTest.java | 80 ++++++++----------- 7 files changed, 74 insertions(+), 101 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentReference.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentReference.java index f52b3c536ce..1d54149a68a 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentReference.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentReference.java @@ -171,8 +171,8 @@ public Task set(@NonNull Object data, @NonNull SetOptions options) { checkNotNull(options, "Provided options must not be null."); ParsedSetData parsed = options.isMerge() - ? firestore.getDataConverter().parseMergeData(data, options.getFieldMask(), getId()) - : firestore.getDataConverter().parseSetData(data, getId()); + ? firestore.getDataConverter().parseMergeData(data, options.getFieldMask()) + : firestore.getDataConverter().parseSetData(data); return firestore .getClient() .write(parsed.toMutationList(key, Precondition.NONE)) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentSnapshot.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentSnapshot.java index f709428c4a9..b5da4e82b4f 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentSnapshot.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentSnapshot.java @@ -202,8 +202,7 @@ public T toObject( Map data = getData(serverTimestampBehavior); return data == null ? null - : CustomClassMapper.withDocumentId(key.getPath().getLastSegment()) - .convertToCustomClass(data, valueType); + : CustomClassMapper.convertToCustomClass(data, valueType, key.getPath().getLastSegment()); } /** @@ -358,8 +357,7 @@ public T get( Object data = get(fieldPath, serverTimestampBehavior); return data == null ? null - : CustomClassMapper.withDocumentId(key.getPath().getLastSegment()) - .convertToCustomClass(data, valueType); + : CustomClassMapper.convertToCustomClass(data, valueType, key.getPath().getFirstSegment()); } /** diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/Transaction.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/Transaction.java index 4189e2d934f..8951d3c4b08 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/Transaction.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/Transaction.java @@ -90,10 +90,8 @@ public Transaction set( checkNotNull(options, "Provided options must not be null."); ParsedSetData parsed = options.isMerge() - ? firestore - .getDataConverter() - .parseMergeData(data, options.getFieldMask(), documentRef.getId()) - : firestore.getDataConverter().parseSetData(data, documentRef.getId()); + ? firestore.getDataConverter().parseMergeData(data, options.getFieldMask()) + : firestore.getDataConverter().parseSetData(data); transaction.set(documentRef.getKey(), parsed); return this; } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/UserDataConverter.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/UserDataConverter.java index 6ad6d9dc8ba..47875885282 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/UserDataConverter.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/UserDataConverter.java @@ -77,13 +77,10 @@ public UserDataConverter(DatabaseId databaseId) { * Parse document data from a non-merge set() call. * * @param input A POJO object representing document data. - * @param expectedId If `input` has any fields annotated with {@link DocumentId}, they should - * either be null or match this parameter. */ - public ParsedSetData parseSetData(Object input, String expectedId) { + public ParsedSetData parseSetData(Object input) { ParseAccumulator accumulator = new ParseAccumulator(UserData.Source.Set); - ObjectValue updateData = - convertAndParseDocumentData(input, accumulator.rootContext(), expectedId); + ObjectValue updateData = convertAndParseDocumentData(input, accumulator.rootContext()); return accumulator.toSetData(updateData); } @@ -92,14 +89,10 @@ public ParsedSetData parseSetData(Object input, String expectedId) { * * @param input A POJO object representing document data. * @param fieldMask A {@link FieldMask} object representing the fields to be merged. - * @param expectedId If `input` has any fields annotated with {@link DocumentId}, they should - * either be null or match this parameter. */ - public ParsedSetData parseMergeData( - Object input, @Nullable FieldMask fieldMask, String expectedId) { + public ParsedSetData parseMergeData(Object input, @Nullable FieldMask fieldMask) { ParseAccumulator accumulator = new ParseAccumulator(UserData.Source.MergeSet); - ObjectValue updateData = - convertAndParseDocumentData(input, accumulator.rootContext(), expectedId); + ObjectValue updateData = convertAndParseDocumentData(input, accumulator.rootContext()); if (fieldMask != null) { // Verify that all elements specified in the field mask are part of the parsed context. @@ -214,7 +207,7 @@ public FieldValue parseQueryValue(Object input) { /** Converts a POJO to native types and then parses it into model types. */ private FieldValue convertAndParseFieldData(Object input, ParseContext context) { - Object converted = CustomClassMapper.withDocumentId(null).convertToPlainJavaTypes(input); + Object converted = CustomClassMapper.convertToPlainJavaTypes(input); return parseData(converted, context); } @@ -223,8 +216,7 @@ private FieldValue convertAndParseFieldData(Object input, ParseContext context) * conform to document data (i.e. it must parse into an ObjectValue model type) and will throw an * appropriate error otherwise. */ - private ObjectValue convertAndParseDocumentData( - Object input, ParseContext context, String expectedId) { + private ObjectValue convertAndParseDocumentData(Object input, ParseContext context) { String badDocReason = "Invalid data. Data must be a Map or a suitable POJO object, but it was "; @@ -234,7 +226,7 @@ private ObjectValue convertAndParseDocumentData( throw new IllegalArgumentException(badDocReason + "an array"); } - Object converted = CustomClassMapper.withDocumentId(expectedId).convertToPlainJavaTypes(input); + Object converted = CustomClassMapper.convertToPlainJavaTypes(input); FieldValue value = parseData(converted, context); if (!(value instanceof ObjectValue)) { diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/WriteBatch.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/WriteBatch.java index 5813406ec59..21e368ef8fb 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/WriteBatch.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/WriteBatch.java @@ -90,10 +90,8 @@ public WriteBatch set( verifyNotCommitted(); ParsedSetData parsed = options.isMerge() - ? firestore - .getDataConverter() - .parseMergeData(data, options.getFieldMask(), documentRef.getId()) - : firestore.getDataConverter().parseSetData(data, documentRef.getId()); + ? firestore.getDataConverter().parseMergeData(data, options.getFieldMask()) + : firestore.getDataConverter().parseSetData(data); mutations.addAll(parsed.toMutationList(documentRef.getKey(), Precondition.NONE)); return this; } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/util/CustomClassMapper.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/util/CustomClassMapper.java index 7707ae9ac94..daf29f1f138 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/util/CustomClassMapper.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/util/CustomClassMapper.java @@ -66,23 +66,6 @@ private static void hardAssert(boolean assertion, String message) { } } - /** - * Returns a mapper with a given document ID. The document ID will be used to populate the {@link - * com.google.firebase.firestore.DocumentId} annotated fields of the POJO of the custom class; - * when converting a POJO to plain Java type, the @DocumentId-annotated fields should be either - * null or match the given document ID. - */ - // TODO: actually implement documentId population/validation logic during (de)serialization. - public static CustomClassMapper withDocumentId(String documentId) { - return new CustomClassMapper(documentId); - } - - private CustomClassMapper(String documentId) { - this.documentId = documentId; - } - - private String documentId = null; - /** * Converts a Java representation of JSON data to standard library Java data types: Map, Array, * String, Double, Integer and Boolean. POJOs are converted to Java Maps. @@ -90,11 +73,11 @@ private CustomClassMapper(String documentId) { * @param object The representation of the JSON data * @return JSON representation containing only standard library Java types */ - public Object convertToPlainJavaTypes(Object object) { + public static Object convertToPlainJavaTypes(Object object) { return serialize(object); } - public Map convertToPlainJavaTypes(Map update) { + public static Map convertToPlainJavaTypes(Map update) { Object converted = serialize(update); hardAssert(converted instanceof Map); @SuppressWarnings("unchecked") @@ -108,18 +91,21 @@ public Map convertToPlainJavaTypes(Map update) { * * @param object The representation of the JSON data * @param clazz The class of the object to convert to + * @param documentId The value to set to {@link com.google.firebase.firestore.DocumentId} + * annotated fields in the custom class. * @return The POJO object. */ - public T convertToCustomClass(Object object, Class clazz) { + public static T convertToCustomClass(Object object, Class clazz, String documentId) { + // TODO: Use DeserializeContext to encapsulate ErrorPath and documentId. return deserializeToClass(object, clazz, ErrorPath.EMPTY); } - private Object serialize(T o) { + private static Object serialize(T o) { return serialize(o, ErrorPath.EMPTY); } @SuppressWarnings("unchecked") - private Object serialize(T o, ErrorPath path) { + private static Object serialize(T o, ErrorPath path) { if (path.getLength() > MAX_DEPTH) { throw serializeError( path, @@ -189,7 +175,7 @@ private Object serialize(T o, ErrorPath path) { } else { Class clazz = (Class) o.getClass(); BeanMapper mapper = loadOrCreateBeanMapperForClass(clazz); - return mapper.serialize(o, path, this); + return mapper.serialize(o, path); } } @@ -770,7 +756,8 @@ private Type resolveType(Type type, Map>, Type> types) { } } - Map serialize(T object, ErrorPath path, CustomClassMapper classMapper) { + Map serialize(T object, ErrorPath path) { + // TODO: Add logic to skip @DocumentId annotated fields in serialization. if (!clazz.isAssignableFrom(object.getClass())) { throw new IllegalArgumentException( "Can't serialize object of class " @@ -802,7 +789,7 @@ Map serialize(T object, ErrorPath path, CustomClassMapper classM // Replace null ServerTimestamp-annotated fields with the sentinel. serializedValue = FieldValue.serverTimestamp(); } else { - serializedValue = classMapper.serialize(propertyValue, path.child(property)); + serializedValue = CustomClassMapper.serialize(propertyValue, path.child(property)); } result.put(property, serializedValue); } @@ -1028,4 +1015,18 @@ public String toString() { } } } + + static class DeserializeContext { + final ErrorPath errorPath; + final String documentId; + + DeserializeContext(ErrorPath path, String docId) { + errorPath = path; + documentId = docId; + } + + DeserializeContext newInstanceWithErrorPath(ErrorPath newPath) { + return new DeserializeContext(newPath, documentId); + } + } } diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/util/MapperTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/util/MapperTest.java index 297bcfc846f..b577fd123f5 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/util/MapperTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/util/MapperTest.java @@ -899,11 +899,11 @@ public void setValue(String value) { private static T deserialize(String jsonString, Class clazz) { Map json = fromSingleQuotedString(jsonString); - return CustomClassMapper.withDocumentId(null).convertToCustomClass(json, clazz); + return CustomClassMapper.convertToCustomClass(json, clazz, null); } private static Object serialize(Object object) { - return CustomClassMapper.withDocumentId(null).convertToPlainJavaTypes(object); + return CustomClassMapper.convertToPlainJavaTypes(object); } private static void assertJson(String expected, Object actual) { @@ -1340,8 +1340,7 @@ public void beansCanContainUpperBoundedMaps() { Date date = new Date(1491847082123L); Map source = map("values", map("foo", date)); UpperBoundedMapBean bean = - CustomClassMapper.withDocumentId(null) - .convertToCustomClass(source, UpperBoundedMapBean.class); + CustomClassMapper.convertToCustomClass(source, UpperBoundedMapBean.class, null); Map expected = map("foo", date); assertEquals(expected, bean.values); } @@ -1351,8 +1350,7 @@ public void beansCanContainMultiBoundedMaps() { Date date = new Date(1491847082123L); Map source = map("map", map("values", map("foo", date))); MultiBoundedMapHolderBean bean = - CustomClassMapper.withDocumentId(null) - .convertToCustomClass(source, MultiBoundedMapHolderBean.class); + CustomClassMapper.convertToCustomClass(source, MultiBoundedMapHolderBean.class, null); Map expected = map("foo", date); assertEquals(expected, bean.map.values); @@ -1369,8 +1367,8 @@ public void beansCanContainUnboundedMaps() { public void beansCanContainUnboundedTypeVariableMaps() { Map source = map("map", map("values", map("foo", "bar"))); UnboundedTypeVariableMapHolderBean bean = - CustomClassMapper.withDocumentId(null) - .convertToCustomClass(source, UnboundedTypeVariableMapHolderBean.class); + CustomClassMapper.convertToCustomClass( + source, UnboundedTypeVariableMapHolderBean.class, null); Map expected = map("foo", "bar"); assertEquals(expected, bean.map.values); @@ -1826,36 +1824,24 @@ public void objectAcceptsAnyObject() { @Test public void objectClassCanBePassedInAtTopLevel() { - assertEquals( - "foo", CustomClassMapper.withDocumentId(null).convertToCustomClass("foo", Object.class)); - assertEquals(1, CustomClassMapper.withDocumentId(null).convertToCustomClass(1, Object.class)); - assertEquals(1L, CustomClassMapper.withDocumentId(null).convertToCustomClass(1L, Object.class)); - assertEquals( - true, CustomClassMapper.withDocumentId(null).convertToCustomClass(true, Object.class)); - assertEquals( - 1.1, CustomClassMapper.withDocumentId(null).convertToCustomClass(1.1, Object.class)); + assertEquals("foo", CustomClassMapper.convertToCustomClass("foo", Object.class, null)); + assertEquals(1, CustomClassMapper.convertToCustomClass(1, Object.class, null)); + assertEquals(1L, CustomClassMapper.convertToCustomClass(1L, Object.class, null)); + assertEquals(true, CustomClassMapper.convertToCustomClass(true, Object.class, null)); + assertEquals(1.1, CustomClassMapper.convertToCustomClass(1.1, Object.class, null)); List fooList = Collections.singletonList("foo"); - assertEquals( - fooList, - CustomClassMapper.withDocumentId(null).convertToCustomClass(fooList, Object.class)); + assertEquals(fooList, CustomClassMapper.convertToCustomClass(fooList, Object.class, null)); Map fooMap = Collections.singletonMap("foo", "bar"); - assertEquals( - fooMap, CustomClassMapper.withDocumentId(null).convertToCustomClass(fooMap, Object.class)); + assertEquals(fooMap, CustomClassMapper.convertToCustomClass(fooMap, Object.class, null)); } @Test public void primitiveClassesCanBePassedInTopLevel() { - assertEquals( - "foo", CustomClassMapper.withDocumentId(null).convertToCustomClass("foo", String.class)); - assertEquals( - (Integer) 1, CustomClassMapper.withDocumentId(null).convertToCustomClass(1, Integer.class)); - assertEquals( - (Long) 1L, CustomClassMapper.withDocumentId(null).convertToCustomClass(1L, Long.class)); - assertEquals( - true, CustomClassMapper.withDocumentId(null).convertToCustomClass(true, Boolean.class)); - assertEquals( - (Double) 1.1, - CustomClassMapper.withDocumentId(null).convertToCustomClass(1.1, Double.class)); + assertEquals("foo", CustomClassMapper.convertToCustomClass("foo", String.class, null)); + assertEquals((Integer) 1, CustomClassMapper.convertToCustomClass(1, Integer.class, null)); + assertEquals((Long) 1L, CustomClassMapper.convertToCustomClass(1L, Long.class, null)); + assertEquals(true, CustomClassMapper.convertToCustomClass(true, Boolean.class, null)); + assertEquals((Double) 1.1, CustomClassMapper.convertToCustomClass(1.1, Double.class, null)); } @Test @@ -1864,8 +1850,8 @@ public void passingInListTopLevelThrows() { "Class java.util.List has generic type parameters, please use GenericTypeIndicator " + "instead", () -> - CustomClassMapper.withDocumentId(null) - .convertToCustomClass(Collections.singletonList("foo"), List.class)); + CustomClassMapper.convertToCustomClass( + Collections.singletonList("foo"), List.class, null)); } @Test @@ -1874,29 +1860,29 @@ public void passingInMapTopLevelThrows() { "Class java.util.Map has generic type parameters, please use GenericTypeIndicator " + "instead", () -> - CustomClassMapper.withDocumentId(null) - .convertToCustomClass(Collections.singletonMap("foo", "bar"), Map.class)); + CustomClassMapper.convertToCustomClass( + Collections.singletonMap("foo", "bar"), Map.class, null)); } @Test public void passingInCharacterTopLevelThrows() { assertExceptionContains( "Deserializing values to Character is not supported", - () -> CustomClassMapper.withDocumentId(null).convertToCustomClass('1', Character.class)); + () -> CustomClassMapper.convertToCustomClass('1', Character.class, null)); } @Test public void passingInShortTopLevelThrows() { assertExceptionContains( "Deserializing values to Short is not supported", - () -> CustomClassMapper.withDocumentId(null).convertToCustomClass(1, Short.class)); + () -> CustomClassMapper.convertToCustomClass(1, Short.class, null)); } @Test public void passingInByteTopLevelThrows() { assertExceptionContains( "Deserializing values to Byte is not supported", - () -> CustomClassMapper.withDocumentId(null).convertToCustomClass(1, Byte.class)); + () -> CustomClassMapper.convertToCustomClass(1, Byte.class, null)); } @Test @@ -1933,13 +1919,13 @@ public void collectionsCantBeDeserialized() { @Test public void allowNullEverywhere() { - assertNull(CustomClassMapper.withDocumentId(null).convertToCustomClass(null, Integer.class)); - assertNull(CustomClassMapper.withDocumentId(null).convertToCustomClass(null, String.class)); - assertNull(CustomClassMapper.withDocumentId(null).convertToCustomClass(null, Double.class)); - assertNull(CustomClassMapper.withDocumentId(null).convertToCustomClass(null, Long.class)); - assertNull(CustomClassMapper.withDocumentId(null).convertToCustomClass(null, Boolean.class)); - assertNull(CustomClassMapper.withDocumentId(null).convertToCustomClass(null, StringBean.class)); - assertNull(CustomClassMapper.withDocumentId(null).convertToCustomClass(null, Object.class)); + assertNull(CustomClassMapper.convertToCustomClass(null, Integer.class, null)); + assertNull(CustomClassMapper.convertToCustomClass(null, String.class, null)); + assertNull(CustomClassMapper.convertToCustomClass(null, Double.class, null)); + assertNull(CustomClassMapper.convertToCustomClass(null, Long.class, null)); + assertNull(CustomClassMapper.convertToCustomClass(null, Boolean.class, null)); + assertNull(CustomClassMapper.convertToCustomClass(null, StringBean.class, null)); + assertNull(CustomClassMapper.convertToCustomClass(null, Object.class, null)); } @Test @@ -2250,7 +2236,7 @@ public void deserializationFailureIncludesPath() { Object serialized = Collections.singletonMap("value", (short) 1); try { - CustomClassMapper.withDocumentId(null).convertToCustomClass(serialized, ShortBean.class); + CustomClassMapper.convertToCustomClass(serialized, ShortBean.class, null); fail("should have thrown"); } catch (RuntimeException e) { assertEquals( From cb89423490aa90776a3f52664281cd5a5aedaea0 Mon Sep 17 00:00:00 2001 From: Hui Wu Date: Thu, 23 May 2019 12:11:03 -0400 Subject: [PATCH 07/15] address comments --- .../firebase/firestore/DocumentSnapshot.java | 4 +- .../firestore/util/CustomClassMapper.java | 8 +- .../firestore/{ => util}/DocumentId.java | 8 +- .../firebase/firestore/util/MapperTest.java | 74 ++++++++++--------- 4 files changed, 49 insertions(+), 45 deletions(-) rename firebase-firestore/src/main/java/com/google/firebase/firestore/{ => util}/DocumentId.java (83%) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentSnapshot.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentSnapshot.java index b5da4e82b4f..abb4a7ab8cd 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentSnapshot.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentSnapshot.java @@ -202,7 +202,7 @@ public T toObject( Map data = getData(serverTimestampBehavior); return data == null ? null - : CustomClassMapper.convertToCustomClass(data, valueType, key.getPath().getLastSegment()); + : CustomClassMapper.convertToCustomClass(data, valueType, getReference()); } /** @@ -357,7 +357,7 @@ public T get( Object data = get(fieldPath, serverTimestampBehavior); return data == null ? null - : CustomClassMapper.convertToCustomClass(data, valueType, key.getPath().getFirstSegment()); + : CustomClassMapper.convertToCustomClass(data, valueType, getReference()); } /** diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/util/CustomClassMapper.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/util/CustomClassMapper.java index daf29f1f138..8c7250e54f9 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/util/CustomClassMapper.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/util/CustomClassMapper.java @@ -91,12 +91,12 @@ public static Map convertToPlainJavaTypes(Map update) * * @param object The representation of the JSON data * @param clazz The class of the object to convert to - * @param documentId The value to set to {@link com.google.firebase.firestore.DocumentId} - * annotated fields in the custom class. + * @param docRef The value to set to {@link DocumentId} annotated fields in the custom class. * @return The POJO object. */ - public static T convertToCustomClass(Object object, Class clazz, String documentId) { - // TODO: Use DeserializeContext to encapsulate ErrorPath and documentId. + public static T convertToCustomClass( + Object object, Class clazz, DocumentReference docRef) { + // TODO: Use DeserializeContext to encapsulate ErrorPath and docRef. return deserializeToClass(object, clazz, ErrorPath.EMPTY); } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentId.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/util/DocumentId.java similarity index 83% rename from firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentId.java rename to firebase-firestore/src/main/java/com/google/firebase/firestore/util/DocumentId.java index 6d84fd9d3db..73d670bbbab 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentId.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/util/DocumentId.java @@ -12,9 +12,11 @@ // See the License for the specific language governing permissions and // limitations under the License. -package com.google.firebase.firestore; +package com.google.firebase.firestore.util; import com.google.firebase.annotations.PublicApi; +import com.google.firebase.firestore.DocumentReference; +import com.google.firebase.firestore.DocumentSnapshot; import java.lang.annotation.ElementType; import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; @@ -27,8 +29,8 @@ *

This annotation can only be applied to fields of String or {@link DocumentReference}, * otherwise a runtime exception will be thrown. * - *

When writing a POJO to Firestore, the @DocumentId-annotated field must either be null or match - * the document ID of the document being written to, else a runtime exception will be thrown. + *

When writing a POJO to Firestore, the @DocumentId-annotated field will be ignored, to allow + * writing to any documents that are not the origin of the POJO. */ @PublicApi @Retention(RetentionPolicy.RUNTIME) diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/util/MapperTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/util/MapperTest.java index b577fd123f5..c9ca1d59f01 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/util/MapperTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/util/MapperTest.java @@ -21,6 +21,7 @@ import static org.junit.Assert.assertNull; import static org.junit.Assert.fail; +import com.google.firebase.firestore.DocumentReference; import com.google.firebase.firestore.Exclude; import com.google.firebase.firestore.PropertyName; import com.google.firebase.firestore.ThrowOnExtraProperties; @@ -919,6 +920,15 @@ private static void assertExceptionContains(String partialMessage, Runnable run) } } + private static T convertToCustomClass( + Object object, Class clazz, DocumentReference docRef) { + return CustomClassMapper.convertToCustomClass(object, clazz, docRef); + } + + private static T convertToCustomClass(Object object, Class clazz) { + return CustomClassMapper.convertToCustomClass(object, clazz, null); + } + @Test public void primitiveDeserializeString() { StringBean bean = deserialize("{'value': 'foo'}", StringBean.class); @@ -1339,8 +1349,7 @@ public void beansCanContainMaps() { public void beansCanContainUpperBoundedMaps() { Date date = new Date(1491847082123L); Map source = map("values", map("foo", date)); - UpperBoundedMapBean bean = - CustomClassMapper.convertToCustomClass(source, UpperBoundedMapBean.class, null); + UpperBoundedMapBean bean = convertToCustomClass(source, UpperBoundedMapBean.class); Map expected = map("foo", date); assertEquals(expected, bean.values); } @@ -1349,8 +1358,7 @@ public void beansCanContainUpperBoundedMaps() { public void beansCanContainMultiBoundedMaps() { Date date = new Date(1491847082123L); Map source = map("map", map("values", map("foo", date))); - MultiBoundedMapHolderBean bean = - CustomClassMapper.convertToCustomClass(source, MultiBoundedMapHolderBean.class, null); + MultiBoundedMapHolderBean bean = convertToCustomClass(source, MultiBoundedMapHolderBean.class); Map expected = map("foo", date); assertEquals(expected, bean.map.values); @@ -1367,8 +1375,7 @@ public void beansCanContainUnboundedMaps() { public void beansCanContainUnboundedTypeVariableMaps() { Map source = map("map", map("values", map("foo", "bar"))); UnboundedTypeVariableMapHolderBean bean = - CustomClassMapper.convertToCustomClass( - source, UnboundedTypeVariableMapHolderBean.class, null); + convertToCustomClass(source, UnboundedTypeVariableMapHolderBean.class); Map expected = map("foo", "bar"); assertEquals(expected, bean.map.values); @@ -1824,24 +1831,24 @@ public void objectAcceptsAnyObject() { @Test public void objectClassCanBePassedInAtTopLevel() { - assertEquals("foo", CustomClassMapper.convertToCustomClass("foo", Object.class, null)); - assertEquals(1, CustomClassMapper.convertToCustomClass(1, Object.class, null)); - assertEquals(1L, CustomClassMapper.convertToCustomClass(1L, Object.class, null)); - assertEquals(true, CustomClassMapper.convertToCustomClass(true, Object.class, null)); - assertEquals(1.1, CustomClassMapper.convertToCustomClass(1.1, Object.class, null)); + assertEquals("foo", convertToCustomClass("foo", Object.class)); + assertEquals(1, convertToCustomClass(1, Object.class)); + assertEquals(1L, convertToCustomClass(1L, Object.class)); + assertEquals(true, convertToCustomClass(true, Object.class)); + assertEquals(1.1, convertToCustomClass(1.1, Object.class)); List fooList = Collections.singletonList("foo"); - assertEquals(fooList, CustomClassMapper.convertToCustomClass(fooList, Object.class, null)); + assertEquals(fooList, convertToCustomClass(fooList, Object.class)); Map fooMap = Collections.singletonMap("foo", "bar"); - assertEquals(fooMap, CustomClassMapper.convertToCustomClass(fooMap, Object.class, null)); + assertEquals(fooMap, convertToCustomClass(fooMap, Object.class)); } @Test public void primitiveClassesCanBePassedInTopLevel() { - assertEquals("foo", CustomClassMapper.convertToCustomClass("foo", String.class, null)); - assertEquals((Integer) 1, CustomClassMapper.convertToCustomClass(1, Integer.class, null)); - assertEquals((Long) 1L, CustomClassMapper.convertToCustomClass(1L, Long.class, null)); - assertEquals(true, CustomClassMapper.convertToCustomClass(true, Boolean.class, null)); - assertEquals((Double) 1.1, CustomClassMapper.convertToCustomClass(1.1, Double.class, null)); + assertEquals("foo", convertToCustomClass("foo", String.class)); + assertEquals((Integer) 1, convertToCustomClass(1, Integer.class)); + assertEquals((Long) 1L, convertToCustomClass(1L, Long.class)); + assertEquals(true, convertToCustomClass(true, Boolean.class)); + assertEquals((Double) 1.1, convertToCustomClass(1.1, Double.class)); } @Test @@ -1849,9 +1856,7 @@ public void passingInListTopLevelThrows() { assertExceptionContains( "Class java.util.List has generic type parameters, please use GenericTypeIndicator " + "instead", - () -> - CustomClassMapper.convertToCustomClass( - Collections.singletonList("foo"), List.class, null)); + () -> convertToCustomClass(Collections.singletonList("foo"), List.class)); } @Test @@ -1859,30 +1864,27 @@ public void passingInMapTopLevelThrows() { assertExceptionContains( "Class java.util.Map has generic type parameters, please use GenericTypeIndicator " + "instead", - () -> - CustomClassMapper.convertToCustomClass( - Collections.singletonMap("foo", "bar"), Map.class, null)); + () -> convertToCustomClass(Collections.singletonMap("foo", "bar"), Map.class)); } @Test public void passingInCharacterTopLevelThrows() { assertExceptionContains( "Deserializing values to Character is not supported", - () -> CustomClassMapper.convertToCustomClass('1', Character.class, null)); + () -> convertToCustomClass('1', Character.class)); } @Test public void passingInShortTopLevelThrows() { assertExceptionContains( "Deserializing values to Short is not supported", - () -> CustomClassMapper.convertToCustomClass(1, Short.class, null)); + () -> convertToCustomClass(1, Short.class)); } @Test public void passingInByteTopLevelThrows() { assertExceptionContains( - "Deserializing values to Byte is not supported", - () -> CustomClassMapper.convertToCustomClass(1, Byte.class, null)); + "Deserializing values to Byte is not supported", () -> convertToCustomClass(1, Byte.class)); } @Test @@ -1919,13 +1921,13 @@ public void collectionsCantBeDeserialized() { @Test public void allowNullEverywhere() { - assertNull(CustomClassMapper.convertToCustomClass(null, Integer.class, null)); - assertNull(CustomClassMapper.convertToCustomClass(null, String.class, null)); - assertNull(CustomClassMapper.convertToCustomClass(null, Double.class, null)); - assertNull(CustomClassMapper.convertToCustomClass(null, Long.class, null)); - assertNull(CustomClassMapper.convertToCustomClass(null, Boolean.class, null)); - assertNull(CustomClassMapper.convertToCustomClass(null, StringBean.class, null)); - assertNull(CustomClassMapper.convertToCustomClass(null, Object.class, null)); + assertNull(convertToCustomClass(null, Integer.class)); + assertNull(convertToCustomClass(null, String.class)); + assertNull(convertToCustomClass(null, Double.class)); + assertNull(convertToCustomClass(null, Long.class)); + assertNull(convertToCustomClass(null, Boolean.class)); + assertNull(convertToCustomClass(null, StringBean.class)); + assertNull(convertToCustomClass(null, Object.class)); } @Test @@ -2236,7 +2238,7 @@ public void deserializationFailureIncludesPath() { Object serialized = Collections.singletonMap("value", (short) 1); try { - CustomClassMapper.convertToCustomClass(serialized, ShortBean.class, null); + convertToCustomClass(serialized, ShortBean.class); fail("should have thrown"); } catch (RuntimeException e) { assertEquals( From 2d5ec684307c2d5533e96c1544d869c0d36edf5c Mon Sep 17 00:00:00 2001 From: Hui Wu Date: Thu, 23 May 2019 13:43:18 -0400 Subject: [PATCH 08/15] addressing more nits. --- .../firebase/firestore/UserDataConverter.java | 4 ++-- .../firestore/util/CustomClassMapper.java | 18 ++---------------- .../firebase/firestore/util/DocumentId.java | 2 +- 3 files changed, 5 insertions(+), 19 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/UserDataConverter.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/UserDataConverter.java index 47875885282..642796c07d5 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/UserDataConverter.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/UserDataConverter.java @@ -76,7 +76,7 @@ public UserDataConverter(DatabaseId databaseId) { /** * Parse document data from a non-merge set() call. * - * @param input A POJO object representing document data. + * @param input A map or POJO object representing document data. */ public ParsedSetData parseSetData(Object input) { ParseAccumulator accumulator = new ParseAccumulator(UserData.Source.Set); @@ -87,7 +87,7 @@ public ParsedSetData parseSetData(Object input) { /** * Parse document data from a set() call with SetOptions.merge() set. * - * @param input A POJO object representing document data. + * @param input A map or POJO object representing document data. * @param fieldMask A {@link FieldMask} object representing the fields to be merged. */ public ParsedSetData parseMergeData(Object input, @Nullable FieldMask fieldMask) { diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/util/CustomClassMapper.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/util/CustomClassMapper.java index 8c7250e54f9..df78fa838ca 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/util/CustomClassMapper.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/util/CustomClassMapper.java @@ -96,7 +96,7 @@ public static Map convertToPlainJavaTypes(Map update) */ public static T convertToCustomClass( Object object, Class clazz, DocumentReference docRef) { - // TODO: Use DeserializeContext to encapsulate ErrorPath and docRef. + // TODO(wuandy): Use DeserializeContext to encapsulate ErrorPath and docRef. return deserializeToClass(object, clazz, ErrorPath.EMPTY); } @@ -757,7 +757,7 @@ private Type resolveType(Type type, Map>, Type> types) { } Map serialize(T object, ErrorPath path) { - // TODO: Add logic to skip @DocumentId annotated fields in serialization. + // TODO(wuandy): Add logic to skip @DocumentId annotated fields in serialization. if (!clazz.isAssignableFrom(object.getClass())) { throw new IllegalArgumentException( "Can't serialize object of class " @@ -1015,18 +1015,4 @@ public String toString() { } } } - - static class DeserializeContext { - final ErrorPath errorPath; - final String documentId; - - DeserializeContext(ErrorPath path, String docId) { - errorPath = path; - documentId = docId; - } - - DeserializeContext newInstanceWithErrorPath(ErrorPath newPath) { - return new DeserializeContext(newPath, documentId); - } - } } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/util/DocumentId.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/util/DocumentId.java index 73d670bbbab..9ac8097bd0b 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/util/DocumentId.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/util/DocumentId.java @@ -35,4 +35,4 @@ @PublicApi @Retention(RetentionPolicy.RUNTIME) @Target({ElementType.FIELD}) -public @interface DocumentId {} +@interface DocumentId {} From 90b2e259db8873fd362a1c377da79008deea60c8 Mon Sep 17 00:00:00 2001 From: Hui Wu Date: Fri, 24 May 2019 16:22:23 -0400 Subject: [PATCH 09/15] initial checkin, tests not complete --- .../firestore/util/CustomClassMapper.java | 383 ++++++++++++------ .../firebase/firestore/util/DocumentId.java | 16 +- .../firebase/firestore/util/MapperTest.java | 140 +++++++ 3 files changed, 418 insertions(+), 121 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/util/CustomClassMapper.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/util/CustomClassMapper.java index df78fa838ca..a75d55c0716 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/util/CustomClassMapper.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/util/CustomClassMapper.java @@ -96,8 +96,7 @@ public static Map convertToPlainJavaTypes(Map update) */ public static T convertToCustomClass( Object object, Class clazz, DocumentReference docRef) { - // TODO(wuandy): Use DeserializeContext to encapsulate ErrorPath and docRef. - return deserializeToClass(object, clazz, ErrorPath.EMPTY); + return deserializeToClass(object, clazz, new DeserializeContext(ErrorPath.EMPTY, docRef)); } private static Object serialize(T o) { @@ -180,17 +179,18 @@ private static Object serialize(T o, ErrorPath path) { } @SuppressWarnings({"unchecked", "TypeParameterUnusedInFormals"}) - private static T deserializeToType(Object o, Type type, ErrorPath path) { + private static T deserializeToType(Object o, Type type, DeserializeContext context) { if (o == null) { return null; } else if (type instanceof ParameterizedType) { - return deserializeToParameterizedType(o, (ParameterizedType) type, path); + return deserializeToParameterizedType(o, (ParameterizedType) type, context); } else if (type instanceof Class) { - return deserializeToClass(o, (Class) type, path); + return deserializeToClass(o, (Class) type, context); } else if (type instanceof WildcardType) { Type[] lowerBounds = ((WildcardType) type).getLowerBounds(); if (lowerBounds.length > 0) { - throw deserializeError(path, "Generic lower-bounded wildcard types are not supported"); + throw deserializeError( + context.errorPath, "Generic lower-bounded wildcard types are not supported"); } // Upper bounded wildcards are of the form . Multiple upper bounds are allowed @@ -199,62 +199,63 @@ private static T deserializeToType(Object o, Type type, ErrorPath path) { // has at least an upper bound of Object. Type[] upperBounds = ((WildcardType) type).getUpperBounds(); hardAssert(upperBounds.length > 0, "Unexpected type bounds on wildcard " + type); - return deserializeToType(o, upperBounds[0], path); + return deserializeToType(o, upperBounds[0], context); } else if (type instanceof TypeVariable) { // As above, TypeVariables always have at least one upper bound of Object. Type[] upperBounds = ((TypeVariable) type).getBounds(); hardAssert(upperBounds.length > 0, "Unexpected type bounds on type variable " + type); - return deserializeToType(o, upperBounds[0], path); + return deserializeToType(o, upperBounds[0], context); } else if (type instanceof GenericArrayType) { - throw deserializeError(path, "Generic Arrays are not supported, please use Lists instead"); + throw deserializeError( + context.errorPath, "Generic Arrays are not supported, please use Lists instead"); } else { - throw deserializeError(path, "Unknown type encountered: " + type); + throw deserializeError(context.errorPath, "Unknown type encountered: " + type); } } @SuppressWarnings("unchecked") - private static T deserializeToClass(Object o, Class clazz, ErrorPath path) { + private static T deserializeToClass(Object o, Class clazz, DeserializeContext context) { if (o == null) { return null; } else if (clazz.isPrimitive() || Number.class.isAssignableFrom(clazz) || Boolean.class.isAssignableFrom(clazz) || Character.class.isAssignableFrom(clazz)) { - return deserializeToPrimitive(o, clazz, path); + return deserializeToPrimitive(o, clazz, context); } else if (String.class.isAssignableFrom(clazz)) { - return (T) convertString(o, path); + return (T) convertString(o, context); } else if (Date.class.isAssignableFrom(clazz)) { - return (T) convertDate(o, path); + return (T) convertDate(o, context); } else if (Timestamp.class.isAssignableFrom(clazz)) { - return (T) convertTimestamp(o, path); + return (T) convertTimestamp(o, context); } else if (Blob.class.isAssignableFrom(clazz)) { - return (T) convertBlob(o, path); + return (T) convertBlob(o, context); } else if (GeoPoint.class.isAssignableFrom(clazz)) { - return (T) convertGeoPoint(o, path); + return (T) convertGeoPoint(o, context); } else if (DocumentReference.class.isAssignableFrom(clazz)) { - return (T) convertDocumentReference(o, path); + return (T) convertDocumentReference(o, context); } else if (clazz.isArray()) { throw deserializeError( - path, "Converting to Arrays is not supported, please use Lists instead"); + context.errorPath, "Converting to Arrays is not supported, please use Lists instead"); } else if (clazz.getTypeParameters().length > 0) { throw deserializeError( - path, + context.errorPath, "Class " + clazz.getName() + " has generic type parameters, please use GenericTypeIndicator instead"); } else if (clazz.equals(Object.class)) { return (T) o; } else if (clazz.isEnum()) { - return deserializeToEnum(o, clazz, path); + return deserializeToEnum(o, clazz, context); } else { - return convertBean(o, clazz, path); + return convertBean(o, clazz, context); } } @SuppressWarnings({"unchecked", "TypeParameterUnusedInFormals"}) private static T deserializeToParameterizedType( - Object o, ParameterizedType type, ErrorPath path) { + Object o, ParameterizedType type, DeserializeContext context) { // getRawType should always return a Class Class rawType = (Class) type.getRawType(); if (List.class.isAssignableFrom(rawType)) { @@ -263,32 +264,40 @@ private static T deserializeToParameterizedType( List list = (List) o; List result = new ArrayList<>(list.size()); for (int i = 0; i < list.size(); i++) { - result.add(deserializeToType(list.get(i), genericType, path.child("[" + i + "]"))); + result.add( + deserializeToType( + list.get(i), + genericType, + context.newInstanceWithErrorPath(context.errorPath.child("[" + i + "]")))); } return (T) result; } else { - throw deserializeError(path, "Expected a List, but got a " + o.getClass()); + throw deserializeError(context.errorPath, "Expected a List, but got a " + o.getClass()); } } else if (Map.class.isAssignableFrom(rawType)) { Type keyType = type.getActualTypeArguments()[0]; Type valueType = type.getActualTypeArguments()[1]; if (!keyType.equals(String.class)) { throw deserializeError( - path, + context.errorPath, "Only Maps with string keys are supported, but found Map with key type " + keyType); } - Map map = expectMap(o, path); + Map map = expectMap(o, context); HashMap result = new HashMap<>(); for (Map.Entry entry : map.entrySet()) { result.put( entry.getKey(), - deserializeToType(entry.getValue(), valueType, path.child(entry.getKey()))); + deserializeToType( + entry.getValue(), + valueType, + context.newInstanceWithErrorPath(context.errorPath.child(entry.getKey())))); } return (T) result; } else if (Collection.class.isAssignableFrom(rawType)) { - throw deserializeError(path, "Collections are not supported, please use Lists instead"); + throw deserializeError( + context.errorPath, "Collections are not supported, please use Lists instead"); } else { - Map map = expectMap(o, path); + Map map = expectMap(o, context); BeanMapper mapper = (BeanMapper) loadOrCreateBeanMapperForClass(rawType); HashMap>, Type> typeMapping = new HashMap<>(); TypeVariable>[] typeVariables = mapper.clazz.getTypeParameters(); @@ -299,31 +308,33 @@ private static T deserializeToParameterizedType( for (int i = 0; i < typeVariables.length; i++) { typeMapping.put(typeVariables[i], types[i]); } - return mapper.deserialize(map, typeMapping, path); + return mapper.deserialize(map, typeMapping, context); } } @SuppressWarnings("unchecked") - private static T deserializeToPrimitive(Object o, Class clazz, ErrorPath path) { + private static T deserializeToPrimitive( + Object o, Class clazz, DeserializeContext context) { if (Integer.class.isAssignableFrom(clazz) || int.class.isAssignableFrom(clazz)) { - return (T) convertInteger(o, path); + return (T) convertInteger(o, context); } else if (Boolean.class.isAssignableFrom(clazz) || boolean.class.isAssignableFrom(clazz)) { - return (T) convertBoolean(o, path); + return (T) convertBoolean(o, context); } else if (Double.class.isAssignableFrom(clazz) || double.class.isAssignableFrom(clazz)) { - return (T) convertDouble(o, path); + return (T) convertDouble(o, context); } else if (Long.class.isAssignableFrom(clazz) || long.class.isAssignableFrom(clazz)) { - return (T) convertLong(o, path); + return (T) convertLong(o, context); } else if (Float.class.isAssignableFrom(clazz) || float.class.isAssignableFrom(clazz)) { - return (T) (Float) convertDouble(o, path).floatValue(); + return (T) (Float) convertDouble(o, context).floatValue(); } else { throw deserializeError( - path, + context.errorPath, String.format("Deserializing values to %s is not supported", clazz.getSimpleName())); } } @SuppressWarnings("unchecked") - private static T deserializeToEnum(Object object, Class clazz, ErrorPath path) { + private static T deserializeToEnum( + Object object, Class clazz, DeserializeContext context) { if (object instanceof String) { String value = (String) object; // We cast to Class without generics here since we can't prove the bound @@ -345,12 +356,12 @@ private static T deserializeToEnum(Object object, Class clazz, ErrorPath return (T) Enum.valueOf((Class) clazz, value); } catch (IllegalArgumentException e) { throw deserializeError( - path, + context.errorPath, "Could not find enum value of " + clazz.getName() + " for value \"" + value + "\""); } } else { throw deserializeError( - path, + context.errorPath, "Expected a String while deserializing to enum " + clazz + " but got a " @@ -371,17 +382,17 @@ private static BeanMapper loadOrCreateBeanMapperForClass(Class clazz) } @SuppressWarnings("unchecked") - private static Map expectMap(Object object, ErrorPath path) { + private static Map expectMap(Object object, DeserializeContext context) { if (object instanceof Map) { // TODO: runtime validation of keys? return (Map) object; } else { throw deserializeError( - path, "Expected a Map while deserializing, but got a " + object.getClass()); + context.errorPath, "Expected a Map while deserializing, but got a " + object.getClass()); } } - private static Integer convertInteger(Object o, ErrorPath path) { + private static Integer convertInteger(Object o, DeserializeContext context) { if (o instanceof Integer) { return (Integer) o; } else if (o instanceof Long || o instanceof Double) { @@ -390,18 +401,19 @@ private static Integer convertInteger(Object o, ErrorPath path) { return ((Number) o).intValue(); } else { throw deserializeError( - path, + context.errorPath, "Numeric value out of 32-bit integer range: " + value + ". Did you mean to use a long or double instead of an int?"); } } else { throw deserializeError( - path, "Failed to convert a value of type " + o.getClass().getName() + " to int"); + context.errorPath, + "Failed to convert a value of type " + o.getClass().getName() + " to int"); } } - private static Long convertLong(Object o, ErrorPath path) { + private static Long convertLong(Object o, DeserializeContext context) { if (o instanceof Integer) { return ((Integer) o).longValue(); } else if (o instanceof Long) { @@ -412,18 +424,19 @@ private static Long convertLong(Object o, ErrorPath path) { return value.longValue(); } else { throw deserializeError( - path, + context.errorPath, "Numeric value out of 64-bit long range: " + value + ". Did you mean to use a double instead of a long?"); } } else { throw deserializeError( - path, "Failed to convert a value of type " + o.getClass().getName() + " to long"); + context.errorPath, + "Failed to convert a value of type " + o.getClass().getName() + " to long"); } } - private static Double convertDouble(Object o, ErrorPath path) { + private static Double convertDouble(Object o, DeserializeContext context) { if (o instanceof Integer) { return ((Integer) o).doubleValue(); } else if (o instanceof Long) { @@ -433,7 +446,7 @@ private static Double convertDouble(Object o, ErrorPath path) { return doubleValue; } else { throw deserializeError( - path, + context.errorPath, "Loss of precision while converting number to " + "double: " + o @@ -443,85 +456,92 @@ private static Double convertDouble(Object o, ErrorPath path) { return (Double) o; } else { throw deserializeError( - path, "Failed to convert a value of type " + o.getClass().getName() + " to double"); + context.errorPath, + "Failed to convert a value of type " + o.getClass().getName() + " to double"); } } - private static Boolean convertBoolean(Object o, ErrorPath path) { + private static Boolean convertBoolean(Object o, DeserializeContext context) { if (o instanceof Boolean) { return (Boolean) o; } else { throw deserializeError( - path, "Failed to convert value of type " + o.getClass().getName() + " to boolean"); + context.errorPath, + "Failed to convert value of type " + o.getClass().getName() + " to boolean"); } } - private static String convertString(Object o, ErrorPath path) { + private static String convertString(Object o, DeserializeContext context) { if (o instanceof String) { return (String) o; } else { throw deserializeError( - path, "Failed to convert value of type " + o.getClass().getName() + " to String"); + context.errorPath, + "Failed to convert value of type " + o.getClass().getName() + " to String"); } } - private static Date convertDate(Object o, ErrorPath path) { + private static Date convertDate(Object o, DeserializeContext context) { if (o instanceof Date) { return (Date) o; } else if (o instanceof Timestamp) { return ((Timestamp) o).toDate(); } else { throw deserializeError( - path, "Failed to convert value of type " + o.getClass().getName() + " to Date"); + context.errorPath, + "Failed to convert value of type " + o.getClass().getName() + " to Date"); } } - private static Timestamp convertTimestamp(Object o, ErrorPath path) { + private static Timestamp convertTimestamp(Object o, DeserializeContext context) { if (o instanceof Timestamp) { return (Timestamp) o; } else if (o instanceof Date) { return new Timestamp((Date) o); } else { throw deserializeError( - path, "Failed to convert value of type " + o.getClass().getName() + " to Timestamp"); + context.errorPath, + "Failed to convert value of type " + o.getClass().getName() + " to Timestamp"); } } - private static Blob convertBlob(Object o, ErrorPath path) { + private static Blob convertBlob(Object o, DeserializeContext context) { if (o instanceof Blob) { return (Blob) o; } else { throw deserializeError( - path, "Failed to convert value of type " + o.getClass().getName() + " to Blob"); + context.errorPath, + "Failed to convert value of type " + o.getClass().getName() + " to Blob"); } } - private static GeoPoint convertGeoPoint(Object o, ErrorPath path) { + private static GeoPoint convertGeoPoint(Object o, DeserializeContext context) { if (o instanceof GeoPoint) { return (GeoPoint) o; } else { throw deserializeError( - path, "Failed to convert value of type " + o.getClass().getName() + " to GeoPoint"); + context.errorPath, + "Failed to convert value of type " + o.getClass().getName() + " to GeoPoint"); } } - private static DocumentReference convertDocumentReference(Object o, ErrorPath path) { + private static DocumentReference convertDocumentReference(Object o, DeserializeContext context) { if (o instanceof DocumentReference) { return (DocumentReference) o; } else { throw deserializeError( - path, + context.errorPath, "Failed to convert value of type " + o.getClass().getName() + " to DocumentReference"); } } - private static T convertBean(Object o, Class clazz, ErrorPath path) { + private static T convertBean(Object o, Class clazz, DeserializeContext context) { BeanMapper mapper = loadOrCreateBeanMapperForClass(clazz); if (o instanceof Map) { - return mapper.deserialize(expectMap(o, path), path); + return mapper.deserialize(expectMap(o, context), context); } else { throw deserializeError( - path, + context.errorPath, "Can't convert object of type " + o.getClass().getName() + " to type " + clazz.getName()); } } @@ -542,21 +562,36 @@ private static RuntimeException deserializeError(ErrorPath path, String reason) return new RuntimeException(reason); } + // Helper class to convert from maps to custom objects (Beans), and vice versa. private static class BeanMapper { private final Class clazz; private final Constructor constructor; + // Whether to throw exception if there are properties we don't know how to set to + // custom object fields/setters during deserialization. private final boolean throwOnUnknownProperties; + // Whether to log a message if there are properties we don't know how to set to + // custom object fields/setters during deserialization. private final boolean warnOnUnknownProperties; + // Case insensitive mapping of properties to their case sensitive versions private final Map properties; + // Below are maps to find getter/setter/field from a given property name. + // A property name is the name annotated by @PropertyName, if exists; or their property name + // following the Java Bean convention: field name is kept as-is while getters/setters will have + // their prefixes removed. See method propertyName for details. private final Map getters; private final Map setters; private final Map fields; - // A list of any properties that were annotated with @ServerTimestamp. + // A set of property names that were annotated with @ServerTimestamp. private final HashSet serverTimestamps; + // A set of property names that were annotated with @DocumentId. These properties will be + // populated with + // document ID values during deserialization, and be skipped during serialization. + private final HashSet documentIdPropertyNames; + BeanMapper(Class clazz) { this.clazz = clazz; throwOnUnknownProperties = clazz.isAnnotationPresent(ThrowOnExtraProperties.class); @@ -568,6 +603,7 @@ private static class BeanMapper { fields = new HashMap<>(); serverTimestamps = new HashSet<>(); + documentIdPropertyNames = new HashSet<>(); Constructor constructor; try { @@ -612,45 +648,48 @@ private static class BeanMapper { do { // Add any setters for (Method method : currentClass.getDeclaredMethods()) { - if (shouldIncludeSetter(method)) { - String propertyName = propertyName(method); - String existingPropertyName = properties.get(propertyName.toLowerCase(Locale.US)); - if (existingPropertyName != null) { - if (!existingPropertyName.equals(propertyName)) { - throw new RuntimeException( - "Found setter on " - + currentClass.getName() - + " with invalid case-sensitive name: " - + method.getName()); - } else { - Method existingSetter = setters.get(propertyName); - if (existingSetter == null) { - method.setAccessible(true); - setters.put(propertyName, method); - applySetterAnnotations(method); - } else if (!isSetterOverride(method, existingSetter)) { - // We require that setters with conflicting property names are - // overrides from a base class - if (currentClass == clazz) { - // TODO: Should we support overloads? - throw new RuntimeException( - "Class " - + clazz.getName() - + " has multiple setter overloads with name " - + method.getName()); - } else { - throw new RuntimeException( - "Found conflicting setters " - + "with name: " - + method.getName() - + " (conflicts with " - + existingSetter.getName() - + " defined on " - + existingSetter.getDeclaringClass().getName() - + ")"); - } - } - } + if (!shouldIncludeSetter(method)) { + continue; + } + String propertyName = propertyName(method); + String existingPropertyName = properties.get(propertyName.toLowerCase(Locale.US)); + if (existingPropertyName != null && !existingPropertyName.equals(propertyName)) { + throw new RuntimeException( + "Found setter on " + + currentClass.getName() + + " with invalid case-sensitive name: " + + method.getName()); + } + + // OK to add since existingPropertyName == propertyName. + properties.put(propertyName.toLowerCase(Locale.US), propertyName); + + // Handle `setter` update. + Method existingSetter = setters.get(propertyName); + if (existingSetter == null) { + method.setAccessible(true); + setters.put(propertyName, method); + applySetterAnnotations(method); + } else if (!isSetterOverride(method, existingSetter)) { + // We require that setters with conflicting property names are + // overrides from a base class + if (currentClass == clazz) { + // TODO: Should we support overloads? + throw new RuntimeException( + "Class " + + clazz.getName() + + " has multiple setter overloads with name " + + method.getName()); + } else { + throw new RuntimeException( + "Found conflicting setters " + + "with name: " + + method.getName() + + " (conflicts with " + + existingSetter.getName() + + " defined on " + + existingSetter.getDeclaringClass().getName() + + ")"); } } } @@ -676,6 +715,19 @@ private static class BeanMapper { if (properties.isEmpty()) { throw new RuntimeException("No properties to serialize found on class " + clazz.getName()); } + + // Make sure we can write to @DocumentId annotated properties before proceeding. + for (String docIdProperty : documentIdPropertyNames) { + if (setters.containsKey(docIdProperty) || fields.containsKey(docIdProperty)) { + continue; + } + + throw new RuntimeException( + "@DocumentId is annotated on property " + + docIdProperty + + " which provides no way to write to on class " + + clazz.getName()); + } } private void addProperty(String property) { @@ -688,15 +740,17 @@ private void addProperty(String property) { } } - T deserialize(Map values, ErrorPath path) { - return deserialize(values, Collections.emptyMap(), path); + T deserialize(Map values, DeserializeContext context) { + return deserialize(values, Collections.emptyMap(), context); } T deserialize( - Map values, Map>, Type> types, ErrorPath path) { + Map values, + Map>, Type> types, + DeserializeContext context) { if (constructor == null) { throw deserializeError( - path, + context.errorPath, "Class " + clazz.getName() + " does not define a no-argument constructor. If you are using ProGuard, make " @@ -706,7 +760,7 @@ T deserialize( T instance = newInstance(constructor); for (Map.Entry entry : values.entrySet()) { String propertyName = entry.getKey(); - ErrorPath childPath = path.child(propertyName); + ErrorPath childPath = context.errorPath.child(propertyName); if (setters.containsKey(propertyName)) { Method setter = setters.get(propertyName); Type[] params = setter.getGenericParameterTypes(); @@ -715,13 +769,15 @@ T deserialize( } Type resolvedType = resolveType(params[0], types); Object value = - CustomClassMapper.deserializeToType(entry.getValue(), resolvedType, childPath); + CustomClassMapper.deserializeToType( + entry.getValue(), resolvedType, context.newInstanceWithErrorPath(childPath)); invoke(setter, instance, value); } else if (fields.containsKey(propertyName)) { Field field = fields.get(propertyName); Type resolvedType = resolveType(field.getGenericType(), types); Object value = - CustomClassMapper.deserializeToType(entry.getValue(), resolvedType, childPath); + CustomClassMapper.deserializeToType( + entry.getValue(), resolvedType, context.newInstanceWithErrorPath(childPath)); try { field.set(instance, value); } catch (IllegalAccessException e) { @@ -740,6 +796,36 @@ T deserialize( } } } + + // Populate @DocumentId annotated fields. If there is a conflict, @DocumentId annotation will + // override whatever value set earlier with document ID from context. + for (String docIdPropertyName : documentIdPropertyNames) { + ErrorPath childPath = context.errorPath.child(docIdPropertyName); + if (setters.containsKey(docIdPropertyName)) { + Method setter = setters.get(docIdPropertyName); + Type[] params = setter.getGenericParameterTypes(); + if (params.length != 1) { + throw deserializeError(childPath, "Setter does not have exactly one parameter"); + } + Type resolvedType = resolveType(params[0], types); + if (resolvedType == String.class) { + invoke(setter, instance, context.documentRef.getId()); + } else { + invoke(setter, instance, context.documentRef); + } + } else { + Field docIdField = fields.get(docIdPropertyName); + try { + if (docIdField.getType() == String.class) { + docIdField.set(instance, context.documentRef.getId()); + } else { + docIdField.set(instance, context.documentRef); + } + } catch (IllegalAccessException e) { + throw new RuntimeException(e); + } + } + } return instance; } @@ -757,7 +843,6 @@ private Type resolveType(Type type, Map>, Type> types) { } Map serialize(T object, ErrorPath path) { - // TODO(wuandy): Add logic to skip @DocumentId annotated fields in serialization. if (!clazz.isAssignableFrom(object.getClass())) { throw new IllegalArgumentException( "Can't serialize object of class " @@ -767,6 +852,11 @@ Map serialize(T object, ErrorPath path) { } Map result = new HashMap<>(); for (String property : properties.values()) { + // Skip @DocumentId annotated properties; + if (documentIdPropertyNames.contains(property)) { + continue; + } + Object propertyValue; if (getters.containsKey(property)) { Method getter = getters.get(property); @@ -809,6 +899,19 @@ private void applyFieldAnnotations(Field field) { } serverTimestamps.add(propertyName(field)); } + + if (field.isAnnotationPresent(DocumentId.class)) { + Class fieldType = field.getType(); + if (fieldType != String.class && fieldType != DocumentReference.class) { + throw new IllegalArgumentException( + "Field " + + field.getName() + + " is annotated with @DocumentId but is " + + fieldType + + " instead of String or DocumentReference."); + } + documentIdPropertyNames.add(propertyName(field)); + } } private void applyGetterAnnotations(Method method) { @@ -824,6 +927,20 @@ private void applyGetterAnnotations(Method method) { } serverTimestamps.add(propertyName(method)); } + + // Even though the value will be skipped, we still check for type matching for consistency. + if (method.isAnnotationPresent(DocumentId.class)) { + Class returnType = method.getReturnType(); + if (returnType != String.class && returnType != DocumentReference.class) { + throw new IllegalArgumentException( + "Method " + + method.getName() + + " is annotated with @DocumentId but returns " + + returnType + + " instead of String or DocumentReference."); + } + documentIdPropertyNames.add(propertyName(method)); + } } private void applySetterAnnotations(Method method) { @@ -834,6 +951,19 @@ private void applySetterAnnotations(Method method) { + " is annotated with @ServerTimestamp but should not be. @ServerTimestamp can" + " only be applied to fields and getters, not setters."); } + + if (method.isAnnotationPresent(DocumentId.class)) { + Class paramType = method.getParameterTypes()[0]; + if (paramType != String.class && paramType != DocumentReference.class) { + throw new IllegalArgumentException( + "Method " + + method.getName() + + " is annotated with @DocumentId but sets " + + paramType + + " instead of String or DocumentReference."); + } + documentIdPropertyNames.add(propertyName(method)); + } } private static boolean shouldIncludeGetter(Method method) { @@ -1015,4 +1145,23 @@ public String toString() { } } } + + /** Holds information a deserialization operation needs to complete the job. */ + static class DeserializeContext { + + /** Current path to the field being deserialized, used for better error messages. */ + final ErrorPath errorPath; + + /** Value used to set to {@link DocumentId} annotated fields during deserialization, if any. */ + final DocumentReference documentRef; + + DeserializeContext(ErrorPath path, DocumentReference docRef) { + errorPath = path; + documentRef = docRef; + } + + DeserializeContext newInstanceWithErrorPath(ErrorPath newPath) { + return new DeserializeContext(newPath, documentRef); + } + } } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/util/DocumentId.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/util/DocumentId.java index 9ac8097bd0b..66b8a80fdf8 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/util/DocumentId.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/util/DocumentId.java @@ -23,16 +23,24 @@ import java.lang.annotation.Target; /** - * Annotation used to mark a POJO field to be automatically populated with the document's ID when + * Annotation used to mark a POJO property to be automatically populated with the document's ID when * the POJO is created from a Firestore document (e.g. via {@link DocumentSnapshot#toObject}). * - *

This annotation can only be applied to fields of String or {@link DocumentReference}, + *

This annotation can only be applied to properties of String or {@link DocumentReference}, * otherwise a runtime exception will be thrown. * - *

When writing a POJO to Firestore, the @DocumentId-annotated field will be ignored, to allow + *

A run time exception will be thrown if this annotation is applied to a property that is not + * writeable (eg, a Java Bean getter without a backing field.). + * + *

If there are conflicts between this annotation and property name matches, this annotation will + * take precedence. For example: If a POJO has a field `firstName` annotated by @DocumentId, and + * there is a property from the document named `firstName` as well, the SDK will set the value to be + * the document ID. + * + *

When writing a POJO to Firestore, the @DocumentId-annotated property will be ignored, to allow * writing to any documents that are not the origin of the POJO. */ @PublicApi @Retention(RetentionPolicy.RUNTIME) -@Target({ElementType.FIELD}) +@Target({ElementType.FIELD, ElementType.METHOD}) @interface DocumentId {} diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/util/MapperTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/util/MapperTest.java index c9ca1d59f01..6f0752ba0f8 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/util/MapperTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/util/MapperTest.java @@ -173,6 +173,13 @@ public String getValue() { } } + @ThrowOnExtraProperties + private static class GetterBeanNoField { + public String getValue() { + return "getter:value"; + } + } + private static class GetterPublicFieldBean { public String value; @@ -1498,6 +1505,21 @@ public void getterOverridesField() { assertJson("{'value': 'getter:foo'}", serialize(bean)); } + @Test + public void serializeGetterBeanWithNoBackingField() { + GetterBeanNoField bean = new GetterBeanNoField(); + assertJson("{'value': 'getter:value'}", serialize(bean)); + } + + @Test + public void deserializeGetterBeanWithNoBackingFieldThrows() { + assertExceptionContains( + "No setter/field", + () -> { + deserialize("{'value': 'foo'}", GetterBeanNoField.class); + }); + } + @Test public void getterOverridesPublicField() { GetterPublicFieldBean bean = new GetterPublicFieldBean(); @@ -2247,4 +2269,122 @@ public void deserializationFailureIncludesPath() { e.getMessage()); } } + + // Bean definitions with @DocumentId applied to wrong type. + private static class FieldWithDocumentIdOnWrongTypeBean { + @DocumentId public Integer intField; + } + + private static class SetterWithDocumentIdOnWrongTypeBean { + private int intField = 100; + + @DocumentId + public void setIntField(int value) { + intField = value; + } + } + + private static class GetterWithDocumentIdOnWrongTypeBean { + private int intField = 100; + + @DocumentId + public int getIntField() { + return intField; + } + } + + private static class PropertyWithDocumentIdOnWrongTypeBean { + @PropertyName("intField") + @DocumentId + public int intField = 100; + } + + @Test + public void documentIdAnnotateWrongTypeThrows() { + assertExceptionContains( + "instead of String or DocumentReference", + () -> serialize(new FieldWithDocumentIdOnWrongTypeBean())); + assertExceptionContains( + "instead of String or DocumentReference", + () -> deserialize("{'intField': 1}", FieldWithDocumentIdOnWrongTypeBean.class)); + + assertExceptionContains( + "instead of String or DocumentReference", + () -> serialize(new SetterWithDocumentIdOnWrongTypeBean())); + assertExceptionContains( + "instead of String or DocumentReference", + () -> deserialize("{'intField': 1}", SetterWithDocumentIdOnWrongTypeBean.class)); + + assertExceptionContains( + "instead of String or DocumentReference", + () -> serialize(new GetterWithDocumentIdOnWrongTypeBean())); + assertExceptionContains( + "instead of String or DocumentReference", + () -> deserialize("{'intField': 1}", GetterWithDocumentIdOnWrongTypeBean.class)); + + assertExceptionContains( + "instead of String or DocumentReference", + () -> serialize(new PropertyWithDocumentIdOnWrongTypeBean())); + assertExceptionContains( + "instead of String or DocumentReference", + () -> deserialize("{'intField': 1}", PropertyWithDocumentIdOnWrongTypeBean.class)); + } + + private static class GetterWithoutBackingFieldOnDocumentIdBean { + @DocumentId + public String getDocId() { + return "doc-id"; + } + } + + @Test + public void documentIdAnnotateReadOnlyThrows() { + // Serialization. + GetterWithoutBackingFieldOnDocumentIdBean bean = + new GetterWithoutBackingFieldOnDocumentIdBean(); + assertExceptionContains("provides no way to write", () -> serialize(bean)); + + // Deserialization. + assertExceptionContains( + "provides no way to write", + () -> deserialize("{'docId': 'id'}", GetterWithoutBackingFieldOnDocumentIdBean.class)); + } + + private static class DocumentIdOnStringField { + @DocumentId public String docId = "doc-id"; + } + + private static class DocumentIdOnDocRefGetter { + private DocumentReference docRef; + + @DocumentId + public DocumentReference getDocRef() { + return docRef; + } + + public void setDocRef(DocumentReference ref) { + docRef = ref; + } + } + + private static class DocumentIdOnInheritedDocRefSetter {} + + private static class DocumentIdOnNestObjects {} + + @Test + public void documentIdsDeserialize() {} + + @Test + public void documentIdsAreIgnoredWhenSerializing() {} + + private static class DocumentIdOnStringFieldWithConflict {} + + private static class DocumentIdOnDocRefGetterWithConflict {} + + private static class DocumentIdOnNestObjectStringProperyWithConflict {} + + private static class DocumentIdOnInheritedDocRefSetterWithConflict {} + + @Test + public void documentIdsDeserializeConflictThrows() {} } From 3cdfa8513acf7e2a16168f99e2fb34e9fb7a9cff Mon Sep 17 00:00:00 2001 From: Hui Wu Date: Sun, 26 May 2019 21:01:38 -0400 Subject: [PATCH 10/15] completes feature. --- .../firestore/util/CustomClassMapper.java | 18 ++- .../firebase/firestore/util/MapperTest.java | 108 +++++++++++++++++- 2 files changed, 118 insertions(+), 8 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/util/CustomClassMapper.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/util/CustomClassMapper.java index a75d55c0716..a0b92aba286 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/util/CustomClassMapper.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/util/CustomClassMapper.java @@ -758,6 +758,7 @@ T deserialize( } T instance = newInstance(constructor); + HashSet deserialzedProperties = new HashSet<>(); for (Map.Entry entry : values.entrySet()) { String propertyName = entry.getKey(); ErrorPath childPath = context.errorPath.child(propertyName); @@ -772,6 +773,7 @@ T deserialize( CustomClassMapper.deserializeToType( entry.getValue(), resolvedType, context.newInstanceWithErrorPath(childPath)); invoke(setter, instance, value); + deserialzedProperties.add(propertyName); } else if (fields.containsKey(propertyName)) { Field field = fields.get(propertyName); Type resolvedType = resolveType(field.getGenericType(), types); @@ -783,6 +785,7 @@ T deserialize( } catch (IllegalAccessException e) { throw new RuntimeException(e); } + deserialzedProperties.add(propertyName); } else { String message = "No setter/field for " + propertyName + " found on class " + clazz.getName(); @@ -797,9 +800,20 @@ T deserialize( } } - // Populate @DocumentId annotated fields. If there is a conflict, @DocumentId annotation will - // override whatever value set earlier with document ID from context. + // Populate @DocumentId annotated fields. If there is a conflict (@DocumentId annotation is + // applied to a property that is already deserialized from the firestore document) + // a runtime exception will be thrown. for (String docIdPropertyName : documentIdPropertyNames) { + if (deserialzedProperties.contains(docIdPropertyName)) { + String message = + "'" + + docIdPropertyName + + "' is found from document " + + context.documentRef.getPath() + + ", cannot apply @DocumentId on this property for class " + + clazz.getName(); + throw new RuntimeException(message); + } ErrorPath childPath = context.errorPath.child(docIdPropertyName); if (setters.containsKey(docIdPropertyName)) { Method setter = setters.get(docIdPropertyName); diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/util/MapperTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/util/MapperTest.java index 6f0752ba0f8..5bd738ac63e 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/util/MapperTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/util/MapperTest.java @@ -24,6 +24,7 @@ import com.google.firebase.firestore.DocumentReference; import com.google.firebase.firestore.Exclude; import com.google.firebase.firestore.PropertyName; +import com.google.firebase.firestore.TestUtil; import com.google.firebase.firestore.ThrowOnExtraProperties; import java.io.Serializable; import java.util.ArrayList; @@ -906,8 +907,12 @@ public void setValue(String value) { } private static T deserialize(String jsonString, Class clazz) { + return deserialize(jsonString, clazz, null); + } + + private static T deserialize(String jsonString, Class clazz, DocumentReference docRef) { Map json = fromSingleQuotedString(jsonString); - return CustomClassMapper.convertToCustomClass(json, clazz, null); + return CustomClassMapper.convertToCustomClass(json, clazz, docRef); } private static Object serialize(Object object) { @@ -2354,6 +2359,15 @@ private static class DocumentIdOnStringField { @DocumentId public String docId = "doc-id"; } + private static class DocumentIdOnStringFieldAsProperty { + @PropertyName("docIdProperty") + @DocumentId + public String docId = "doc-id"; + + @PropertyName("anotherProperty") + public int someOtherProperty = 0; + } + private static class DocumentIdOnDocRefGetter { private DocumentReference docRef; @@ -2367,15 +2381,75 @@ public void setDocRef(DocumentReference ref) { } } - private static class DocumentIdOnInheritedDocRefSetter {} + private static class DocumentIdOnInheritedDocRefSetter extends DocumentIdOnDocRefGetter { + + private DocumentReference inheritedDocRef; + + @DocumentId + public DocumentReference getInheritedDocRef() { + return inheritedDocRef; + } + + public void setInheritedDocRef(DocumentReference ref) { + inheritedDocRef = ref; + } + } - private static class DocumentIdOnNestObjects {} + private static class DocumentIdOnNestedObjects { + @PropertyName("nestedDocIdHolder") + public DocumentIdOnStringField nestedDocIdHolder; + } @Test - public void documentIdsDeserialize() {} + public void documentIdsDeserialize() { + DocumentReference ref = TestUtil.documentReference("coll/doc123"); + + assertEquals("doc123", deserialize("{}", DocumentIdOnStringField.class, ref).docId); + + DocumentIdOnStringFieldAsProperty target = + deserialize("{'anotherProperty': 100}", DocumentIdOnStringFieldAsProperty.class, ref); + assertEquals("doc123", target.docId); + assertEquals(100, target.someOtherProperty); + + assertEquals(ref, deserialize("{}", DocumentIdOnDocRefGetter.class, ref).getDocRef()); + + DocumentIdOnInheritedDocRefSetter target1 = + deserialize("{}", DocumentIdOnInheritedDocRefSetter.class, ref); + assertEquals(ref, target1.getInheritedDocRef()); + assertEquals(ref, target1.getDocRef()); + + assertEquals( + "doc123", + deserialize("{'nestedDocIdHolder': {}}", DocumentIdOnNestedObjects.class, ref) + .nestedDocIdHolder + .docId); + } @Test - public void documentIdsAreIgnoredWhenSerializing() {} + public void documentIdsRoundTrip() { + // Implicitly verifies @DocumentId is ignore during serialization. + + DocumentReference ref = TestUtil.documentReference("coll/doc123"); + + assertEquals( + Collections.emptyMap(), serialize(deserialize("{}", DocumentIdOnStringField.class, ref))); + + assertEquals( + Collections.singletonMap("anotherProperty", 100), + serialize( + deserialize("{'anotherProperty': 100}", DocumentIdOnStringFieldAsProperty.class, ref))); + + assertEquals( + Collections.emptyMap(), serialize(deserialize("{}", DocumentIdOnDocRefGetter.class, ref))); + + assertEquals( + Collections.emptyMap(), + serialize(deserialize("{}", DocumentIdOnInheritedDocRefSetter.class, ref))); + + assertEquals( + Collections.singletonMap("nestedDocIdHolder", Collections.emptyMap()), + serialize(deserialize("{'nestedDocIdHolder': {}}", DocumentIdOnNestedObjects.class, ref))); + } private static class DocumentIdOnStringFieldWithConflict {} @@ -2386,5 +2460,27 @@ private static class DocumentIdOnNestObjectStringProperyWithConflict {} private static class DocumentIdOnInheritedDocRefSetterWithConflict {} @Test - public void documentIdsDeserializeConflictThrows() {} + public void documentIdsDeserializeConflictThrows() { + DocumentReference ref = TestUtil.documentReference("coll/doc123"); + + assertExceptionContains( + "cannot apply @DocumentId on this property", + () -> deserialize("{'docId': 'toBeOverwritten'}", DocumentIdOnStringField.class, ref)); + + assertExceptionContains( + "cannot apply @DocumentId on this property", + () -> + deserialize( + "{'docIdProperty': 'toBeOverwritten', 'anotherProperty': 100}", + DocumentIdOnStringFieldAsProperty.class, + ref)); + + assertExceptionContains( + "cannot apply @DocumentId on this property", + () -> + deserialize( + "{'nestedDocIdHolder': {'docId': 'toBeOverwritten'}}", + DocumentIdOnNestedObjects.class, + ref)); + } } From d24ea62cbbed09bd476604644e54af59b7ed1ca9 Mon Sep 17 00:00:00 2001 From: Hui Wu Date: Sun, 26 May 2019 21:08:47 -0400 Subject: [PATCH 11/15] fix comments --- .../com/google/firebase/firestore/util/DocumentId.java | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/util/DocumentId.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/util/DocumentId.java index 66b8a80fdf8..7eab6674889 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/util/DocumentId.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/util/DocumentId.java @@ -32,10 +32,11 @@ *

A run time exception will be thrown if this annotation is applied to a property that is not * writeable (eg, a Java Bean getter without a backing field.). * - *

If there are conflicts between this annotation and property name matches, this annotation will - * take precedence. For example: If a POJO has a field `firstName` annotated by @DocumentId, and - * there is a property from the document named `firstName` as well, the SDK will set the value to be - * the document ID. + *

If there are conflicts between this annotation and property name matches, a runtime exception + * will be thrown. For example: If a POJO has a field `firstName` annotated by @DocumentId, and + * there is a property from the document named `firstName` as well, an exception will be thrown when + * you try to read document into the POJO via {@link DocumentSnapshot#toObject} or {@link + * DocumentReference#get}. * *

When writing a POJO to Firestore, the @DocumentId-annotated property will be ignored, to allow * writing to any documents that are not the origin of the POJO. From ef2382f974ba771eeddec6632cd8bc940f26b8e3 Mon Sep 17 00:00:00 2001 From: Hui Wu Date: Mon, 27 May 2019 10:04:07 -0400 Subject: [PATCH 12/15] clean up --- .../com/google/firebase/firestore/util/MapperTest.java | 8 -------- 1 file changed, 8 deletions(-) diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/util/MapperTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/util/MapperTest.java index 5bd738ac63e..29945748e21 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/util/MapperTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/util/MapperTest.java @@ -2451,14 +2451,6 @@ public void documentIdsRoundTrip() { serialize(deserialize("{'nestedDocIdHolder': {}}", DocumentIdOnNestedObjects.class, ref))); } - private static class DocumentIdOnStringFieldWithConflict {} - - private static class DocumentIdOnDocRefGetterWithConflict {} - - private static class DocumentIdOnNestObjectStringProperyWithConflict {} - - private static class DocumentIdOnInheritedDocRefSetterWithConflict {} - @Test public void documentIdsDeserializeConflictThrows() { DocumentReference ref = TestUtil.documentReference("coll/doc123"); From a5d91bbd18558f8a49a74727bf659b8fffc58f3b Mon Sep 17 00:00:00 2001 From: Hui Wu Date: Mon, 27 May 2019 10:27:27 -0400 Subject: [PATCH 13/15] more cleanup and refactoring --- .../firestore/util/CustomClassMapper.java | 21 +++++++++---------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/util/CustomClassMapper.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/util/CustomClassMapper.java index a0b92aba286..0d64ee1aa7e 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/util/CustomClassMapper.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/util/CustomClassMapper.java @@ -661,18 +661,10 @@ private static class BeanMapper { + method.getName()); } - // OK to add since existingPropertyName == propertyName. - properties.put(propertyName.toLowerCase(Locale.US), propertyName); - - // Handle `setter` update. Method existingSetter = setters.get(propertyName); - if (existingSetter == null) { - method.setAccessible(true); - setters.put(propertyName, method); - applySetterAnnotations(method); - } else if (!isSetterOverride(method, existingSetter)) { - // We require that setters with conflicting property names are - // overrides from a base class + + // Raise exception if a conflict between setters is found. + if (existingSetter != null && !isSetterOverride(method, existingSetter)) { if (currentClass == clazz) { // TODO: Should we support overloads? throw new RuntimeException( @@ -692,6 +684,13 @@ private static class BeanMapper { + ")"); } } + + // Make it accessible and process annotations if not yet. + if (existingSetter == null) { + method.setAccessible(true); + setters.put(propertyName, method); + applySetterAnnotations(method); + } } for (Field field : currentClass.getDeclaredFields()) { From 554ede50a71e2c3b54b155b1f63788e2cdbe4696 Mon Sep 17 00:00:00 2001 From: Hui Wu Date: Sun, 2 Jun 2019 22:04:12 -0400 Subject: [PATCH 14/15] addressing comments --- .../firestore/util/CustomClassMapper.java | 154 +++++++++--------- .../firebase/firestore/util/MapperTest.java | 48 ++---- 2 files changed, 91 insertions(+), 111 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/util/CustomClassMapper.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/util/CustomClassMapper.java index 0d64ee1aa7e..45f4c40df2c 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/util/CustomClassMapper.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/util/CustomClassMapper.java @@ -588,8 +588,8 @@ private static class BeanMapper { private final HashSet serverTimestamps; // A set of property names that were annotated with @DocumentId. These properties will be - // populated with - // document ID values during deserialization, and be skipped during serialization. + // populated with document ID values during deserialization, and be skipped during + // serialization. private final HashSet documentIdPropertyNames; BeanMapper(Class clazz) { @@ -648,49 +648,47 @@ private static class BeanMapper { do { // Add any setters for (Method method : currentClass.getDeclaredMethods()) { - if (!shouldIncludeSetter(method)) { - continue; - } - String propertyName = propertyName(method); - String existingPropertyName = properties.get(propertyName.toLowerCase(Locale.US)); - if (existingPropertyName != null && !existingPropertyName.equals(propertyName)) { - throw new RuntimeException( - "Found setter on " - + currentClass.getName() - + " with invalid case-sensitive name: " - + method.getName()); - } - - Method existingSetter = setters.get(propertyName); - - // Raise exception if a conflict between setters is found. - if (existingSetter != null && !isSetterOverride(method, existingSetter)) { - if (currentClass == clazz) { - // TODO: Should we support overloads? - throw new RuntimeException( - "Class " - + clazz.getName() - + " has multiple setter overloads with name " - + method.getName()); - } else { - throw new RuntimeException( - "Found conflicting setters " - + "with name: " - + method.getName() - + " (conflicts with " - + existingSetter.getName() - + " defined on " - + existingSetter.getDeclaringClass().getName() - + ")"); + if (shouldIncludeSetter(method)) { + String propertyName = propertyName(method); + String existingPropertyName = properties.get(propertyName.toLowerCase(Locale.US)); + if (existingPropertyName != null) { + if (!existingPropertyName.equals(propertyName)) { + throw new RuntimeException( + "Found setter on " + + currentClass.getName() + + " with invalid case-sensitive name: " + + method.getName()); + } else { + Method existingSetter = setters.get(propertyName); + if (existingSetter == null) { + method.setAccessible(true); + setters.put(propertyName, method); + applySetterAnnotations(method); + } else if (!isSetterOverride(method, existingSetter)) { + // We require that setters with conflicting property names are + // overrides from a base class + if (currentClass == clazz) { + // TODO: Should we support overloads? + throw new RuntimeException( + "Class " + + clazz.getName() + + " has multiple setter overloads with name " + + method.getName()); + } else { + throw new RuntimeException( + "Found conflicting setters " + + "with name: " + + method.getName() + + " (conflicts with " + + existingSetter.getName() + + " defined on " + + existingSetter.getDeclaringClass().getName() + + ")"); + } + } + } } } - - // Make it accessible and process annotations if not yet. - if (existingSetter == null) { - method.setAccessible(true); - setters.put(propertyName, method); - applySetterAnnotations(method); - } } for (Field field : currentClass.getDeclaredFields()) { @@ -717,15 +715,14 @@ private static class BeanMapper { // Make sure we can write to @DocumentId annotated properties before proceeding. for (String docIdProperty : documentIdPropertyNames) { - if (setters.containsKey(docIdProperty) || fields.containsKey(docIdProperty)) { - continue; + if (!setters.containsKey(docIdProperty) && !fields.containsKey(docIdProperty)) { + throw new RuntimeException( + "@DocumentId is annotated on property " + + docIdProperty + + " of class " + + clazz.getName() + + " but no field or public setter was found"); } - - throw new RuntimeException( - "@DocumentId is annotated on property " - + docIdProperty - + " which provides no way to write to on class " - + clazz.getName()); } } @@ -798,10 +795,19 @@ T deserialize( } } } + populateDocumentIdProperties(types, context, instance, deserialzedProperties); + + return instance; + } - // Populate @DocumentId annotated fields. If there is a conflict (@DocumentId annotation is - // applied to a property that is already deserialized from the firestore document) - // a runtime exception will be thrown. + // Populate @DocumentId annotated fields. If there is a conflict (@DocumentId annotation is + // applied to a property that is already deserialized from the firestore document) + // a runtime exception will be thrown. + private void populateDocumentIdProperties( + Map>, Type> types, + DeserializeContext context, + T instance, + HashSet deserialzedProperties) { for (String docIdPropertyName : documentIdPropertyNames) { if (deserialzedProperties.contains(docIdPropertyName)) { String message = @@ -839,7 +845,6 @@ T deserialize( } } } - return instance; } private Type resolveType(Type type, Map>, Type> types) { @@ -915,14 +920,7 @@ private void applyFieldAnnotations(Field field) { if (field.isAnnotationPresent(DocumentId.class)) { Class fieldType = field.getType(); - if (fieldType != String.class && fieldType != DocumentReference.class) { - throw new IllegalArgumentException( - "Field " - + field.getName() - + " is annotated with @DocumentId but is " - + fieldType - + " instead of String or DocumentReference."); - } + ensureValidDocumentIdType("Field", "is", fieldType); documentIdPropertyNames.add(propertyName(field)); } } @@ -944,14 +942,7 @@ private void applyGetterAnnotations(Method method) { // Even though the value will be skipped, we still check for type matching for consistency. if (method.isAnnotationPresent(DocumentId.class)) { Class returnType = method.getReturnType(); - if (returnType != String.class && returnType != DocumentReference.class) { - throw new IllegalArgumentException( - "Method " - + method.getName() - + " is annotated with @DocumentId but returns " - + returnType - + " instead of String or DocumentReference."); - } + ensureValidDocumentIdType("Method", "returns", returnType); documentIdPropertyNames.add(propertyName(method)); } } @@ -967,18 +958,23 @@ private void applySetterAnnotations(Method method) { if (method.isAnnotationPresent(DocumentId.class)) { Class paramType = method.getParameterTypes()[0]; - if (paramType != String.class && paramType != DocumentReference.class) { - throw new IllegalArgumentException( - "Method " - + method.getName() - + " is annotated with @DocumentId but sets " - + paramType - + " instead of String or DocumentReference."); - } + ensureValidDocumentIdType("Method", "sets", paramType); documentIdPropertyNames.add(propertyName(method)); } } + private void ensureValidDocumentIdType(String fieldDescription, String operation, Type type) { + if (type != String.class && type != DocumentReference.class) { + throw new IllegalArgumentException( + fieldDescription + + " is annotated with @DocumentId but " + + operation + + " " + + type + + " instead of String or DocumentReference."); + } + } + private static boolean shouldIncludeGetter(Method method) { if (!method.getName().startsWith("get") && !method.getName().startsWith("is")) { return false; diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/util/MapperTest.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/util/MapperTest.java index 29945748e21..3cd91f34684 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/util/MapperTest.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/util/MapperTest.java @@ -907,7 +907,7 @@ public void setValue(String value) { } private static T deserialize(String jsonString, Class clazz) { - return deserialize(jsonString, clazz, null); + return deserialize(jsonString, clazz, /*docRef=*/ null); } private static T deserialize(String jsonString, Class clazz, DocumentReference docRef) { @@ -2280,15 +2280,6 @@ private static class FieldWithDocumentIdOnWrongTypeBean { @DocumentId public Integer intField; } - private static class SetterWithDocumentIdOnWrongTypeBean { - private int intField = 100; - - @DocumentId - public void setIntField(int value) { - intField = value; - } - } - private static class GetterWithDocumentIdOnWrongTypeBean { private int intField = 100; @@ -2306,32 +2297,23 @@ private static class PropertyWithDocumentIdOnWrongTypeBean { @Test public void documentIdAnnotateWrongTypeThrows() { + final String expectedErrorMessage = "instead of String or DocumentReference"; assertExceptionContains( - "instead of String or DocumentReference", - () -> serialize(new FieldWithDocumentIdOnWrongTypeBean())); + expectedErrorMessage, () -> serialize(new FieldWithDocumentIdOnWrongTypeBean())); assertExceptionContains( - "instead of String or DocumentReference", + expectedErrorMessage, () -> deserialize("{'intField': 1}", FieldWithDocumentIdOnWrongTypeBean.class)); assertExceptionContains( - "instead of String or DocumentReference", - () -> serialize(new SetterWithDocumentIdOnWrongTypeBean())); - assertExceptionContains( - "instead of String or DocumentReference", - () -> deserialize("{'intField': 1}", SetterWithDocumentIdOnWrongTypeBean.class)); - - assertExceptionContains( - "instead of String or DocumentReference", - () -> serialize(new GetterWithDocumentIdOnWrongTypeBean())); + expectedErrorMessage, () -> serialize(new GetterWithDocumentIdOnWrongTypeBean())); assertExceptionContains( - "instead of String or DocumentReference", + expectedErrorMessage, () -> deserialize("{'intField': 1}", GetterWithDocumentIdOnWrongTypeBean.class)); assertExceptionContains( - "instead of String or DocumentReference", - () -> serialize(new PropertyWithDocumentIdOnWrongTypeBean())); + expectedErrorMessage, () -> serialize(new PropertyWithDocumentIdOnWrongTypeBean())); assertExceptionContains( - "instead of String or DocumentReference", + expectedErrorMessage, () -> deserialize("{'intField': 1}", PropertyWithDocumentIdOnWrongTypeBean.class)); } @@ -2344,14 +2326,15 @@ public String getDocId() { @Test public void documentIdAnnotateReadOnlyThrows() { + final String expectedErrorMessage = "but no field or public setter was found"; // Serialization. GetterWithoutBackingFieldOnDocumentIdBean bean = new GetterWithoutBackingFieldOnDocumentIdBean(); - assertExceptionContains("provides no way to write", () -> serialize(bean)); + assertExceptionContains(expectedErrorMessage, () -> serialize(bean)); // Deserialization. assertExceptionContains( - "provides no way to write", + expectedErrorMessage, () -> deserialize("{'docId': 'id'}", GetterWithoutBackingFieldOnDocumentIdBean.class)); } @@ -2427,7 +2410,7 @@ public void documentIdsDeserialize() { @Test public void documentIdsRoundTrip() { - // Implicitly verifies @DocumentId is ignore during serialization. + // Implicitly verifies @DocumentId is ignored during serialization. DocumentReference ref = TestUtil.documentReference("coll/doc123"); @@ -2453,14 +2436,15 @@ public void documentIdsRoundTrip() { @Test public void documentIdsDeserializeConflictThrows() { + final String expectedErrorMessage = "cannot apply @DocumentId on this property"; DocumentReference ref = TestUtil.documentReference("coll/doc123"); assertExceptionContains( - "cannot apply @DocumentId on this property", + expectedErrorMessage, () -> deserialize("{'docId': 'toBeOverwritten'}", DocumentIdOnStringField.class, ref)); assertExceptionContains( - "cannot apply @DocumentId on this property", + expectedErrorMessage, () -> deserialize( "{'docIdProperty': 'toBeOverwritten', 'anotherProperty': 100}", @@ -2468,7 +2452,7 @@ public void documentIdsDeserializeConflictThrows() { ref)); assertExceptionContains( - "cannot apply @DocumentId on this property", + expectedErrorMessage, () -> deserialize( "{'nestedDocIdHolder': {'docId': 'toBeOverwritten'}}", From 0e84b0f7e9a9d9469c7c49a7193a81524f659257 Mon Sep 17 00:00:00 2001 From: Hui Wu Date: Tue, 4 Jun 2019 09:54:14 -0400 Subject: [PATCH 15/15] address error message nits --- .../com/google/firebase/firestore/util/CustomClassMapper.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/util/CustomClassMapper.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/util/CustomClassMapper.java index 75c708a0363..f3230a6b1e3 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/util/CustomClassMapper.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/util/CustomClassMapper.java @@ -813,7 +813,7 @@ private void populateDocumentIdProperties( String message = "'" + docIdPropertyName - + "' is found from document " + + "' was found from document " + context.documentRef.getPath() + ", cannot apply @DocumentId on this property for class " + clazz.getName(); @@ -959,7 +959,7 @@ private void applySetterAnnotations(Method method) { if (method.isAnnotationPresent(DocumentId.class)) { Class paramType = method.getParameterTypes()[0]; - ensureValidDocumentIdType("Method", "sets", paramType); + ensureValidDocumentIdType("Method", "accepts", paramType); documentIdPropertyNames.add(propertyName(method)); } }