diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ValidationTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ValidationTest.java index 460de9e37b7..777717604ff 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ValidationTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ValidationTest.java @@ -701,11 +701,6 @@ public void queriesFilteredByDocumentIDMustUseStringsOrDocumentReferences() { + "a valid String or DocumentReference, but it was of type: java.lang.Integer"; expectError(() -> collection.whereGreaterThanOrEqualTo(FieldPath.documentId(), 1), reason); - reason = - "Invalid query. When querying with FieldPath.documentId() you must provide " - + "a valid String or DocumentReference, but it was of type: java.util.Arrays$ArrayList"; - expectError(() -> collection.whereIn(FieldPath.documentId(), asList(1, 2)), reason); - reason = "Invalid query. When querying a collection group by FieldPath.documentId(), the value " + "provided must result in a valid document path, but 'foo' is not because it has " @@ -726,6 +721,38 @@ public void queriesFilteredByDocumentIDMustUseStringsOrDocumentReferences() { () -> collection.whereArrayContainsAny(FieldPath.documentId(), asList(1, 2)), reason); } + @Test + public void queriesUsingInAndDocumentIdMustHaveProperDocumentReferencesInArray() { + CollectionReference collection = testCollection(); + collection.whereIn(FieldPath.documentId(), asList(collection.getPath())); + + String reason = + "Invalid query. When querying with FieldPath.documentId() you must provide " + + "a valid document ID, but it was an empty string."; + expectError(() -> collection.whereIn(FieldPath.documentId(), asList("")), reason); + + reason = + "Invalid query. When querying a collection by FieldPath.documentId() you must provide " + + "a plain document ID, but 'foo/bar/baz' contains a '/' character."; + expectError(() -> collection.whereIn(FieldPath.documentId(), asList("foo/bar/baz")), reason); + + reason = + "Invalid query. When querying with FieldPath.documentId() you must provide " + + "a valid String or DocumentReference, but it was of type: java.lang.Integer"; + expectError(() -> collection.whereIn(FieldPath.documentId(), asList(1, 2)), reason); + + reason = + "Invalid query. When querying a collection group by FieldPath.documentId(), the value " + + "provided must result in a valid document path, but 'foo' is not because it has " + + "an odd number of segments (1)."; + expectError( + () -> + testFirestore() + .collectionGroup("collection") + .whereIn(FieldPath.documentId(), asList("foo")), + reason); + } + // Helpers /** Performs a write using each write API and makes sure it fails with the expected reason. */ diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java index 92e15387128..16cc3da0770 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java @@ -39,6 +39,7 @@ import com.google.firebase.firestore.model.Document; import com.google.firebase.firestore.model.DocumentKey; import com.google.firebase.firestore.model.ResourcePath; +import com.google.firebase.firestore.model.value.ArrayValue; import com.google.firebase.firestore.model.value.FieldValue; import com.google.firebase.firestore.model.value.ReferenceValue; import com.google.firebase.firestore.model.value.ServerTimestampValue; @@ -84,76 +85,6 @@ public FirebaseFirestore getFirestore() { return firestore; } - private void validateOrderByFieldMatchesInequality( - com.google.firebase.firestore.model.FieldPath orderBy, - com.google.firebase.firestore.model.FieldPath inequality) { - if (!orderBy.equals(inequality)) { - String inequalityString = inequality.canonicalString(); - throw new IllegalArgumentException( - String.format( - "Invalid query. You have an inequality where filter (whereLessThan(), " - + "whereGreaterThan(), etc.) on field '%s' and so you must also have '%s' as " - + "your first orderBy() field, but your first orderBy() is currently on field " - + "'%s' instead.", - inequalityString, inequalityString, orderBy.canonicalString())); - } - } - - private void validateNewFilter(Filter filter) { - if (filter instanceof RelationFilter) { - Operator filterOp = ((RelationFilter) filter).getOperator(); - List arrayOps = Arrays.asList(Operator.ARRAY_CONTAINS, Operator.ARRAY_CONTAINS_ANY); - List disjunctiveOps = Arrays.asList(Operator.ARRAY_CONTAINS_ANY, Operator.IN); - boolean isArrayOp = arrayOps.contains(filterOp); - boolean isDisjunctiveOp = disjunctiveOps.contains(filterOp); - - RelationFilter relationFilter = (RelationFilter) filter; - if (relationFilter.isInequality()) { - com.google.firebase.firestore.model.FieldPath existingInequality = query.inequalityField(); - com.google.firebase.firestore.model.FieldPath newInequality = filter.getField(); - - if (existingInequality != null && !existingInequality.equals(newInequality)) { - throw new IllegalArgumentException( - String.format( - "All where filters other than whereEqualTo() must be on the same field. But you " - + "have filters on '%s' and '%s'", - existingInequality.canonicalString(), newInequality.canonicalString())); - } - com.google.firebase.firestore.model.FieldPath firstOrderByField = - query.getFirstOrderByField(); - if (firstOrderByField != null) { - validateOrderByFieldMatchesInequality(firstOrderByField, newInequality); - } - } else if (isDisjunctiveOp || isArrayOp) { - // You can have at most 1 disjunctive filter and 1 array filter. Check if the new filter - // conflicts with an existing one. - Operator conflictingOp = null; - if (isDisjunctiveOp) { - conflictingOp = this.query.findOperatorFilter(disjunctiveOps); - } - if (conflictingOp == null && isArrayOp) { - conflictingOp = this.query.findOperatorFilter(arrayOps); - } - if (conflictingOp != null) { - // We special case when it's a duplicate op to give a slightly clearer error message. - if (conflictingOp == filterOp) { - throw new IllegalArgumentException( - "Invalid Query. You cannot use more than one '" - + filterOp.toString() - + "' filter."); - } else { - throw new IllegalArgumentException( - "Invalid Query. You cannot use '" - + filterOp.toString() - + "' filters with '" - + conflictingOp.toString() - + "' filters."); - } - } - } - } - } - /** * Creates and returns a new Query with the additional filter that documents must contain the * specified field and the value should be equal to the specified value. @@ -421,66 +352,19 @@ private Query whereHelper(@NonNull FieldPath fieldPath, Operator op, Object valu "Invalid query. You can't perform '" + op.toString() + "' queries on FieldPath.documentId()."); - } - if (value instanceof String) { - String documentKey = (String) value; - if (documentKey.isEmpty()) { - throw new IllegalArgumentException( - "Invalid query. When querying with FieldPath.documentId() you must provide a valid " - + "document ID, but it was an empty string."); + } else if (op == Operator.IN) { + validateDisjunctiveFilterElements(value, op); + List referenceList = new ArrayList<>(); + for (Object arrayValue : (List) value) { + referenceList.add(parseDocumentIdValue(arrayValue)); } - if (!query.isCollectionGroupQuery() && documentKey.contains("/")) { - throw new IllegalArgumentException( - "Invalid query. When querying a collection by FieldPath.documentId() you must " - + "provide a plain document ID, but '" - + documentKey - + "' contains a '/' character."); - } - ResourcePath path = query.getPath().append(ResourcePath.fromString(documentKey)); - if (!DocumentKey.isDocumentKey(path)) { - throw new IllegalArgumentException( - "Invalid query. When querying a collection group by FieldPath.documentId(), the " - + "value provided must result in a valid document path, but '" - + path - + "' is not because it has an odd number of segments (" - + path.length() - + ")."); - } - fieldValue = - ReferenceValue.valueOf(this.getFirestore().getDatabaseId(), DocumentKey.fromPath(path)); - } else if (value instanceof DocumentReference) { - DocumentReference ref = (DocumentReference) value; - fieldValue = ReferenceValue.valueOf(this.getFirestore().getDatabaseId(), ref.getKey()); + fieldValue = ArrayValue.fromList(referenceList); } else { - throw new IllegalArgumentException( - "Invalid query. When querying with FieldPath.documentId() you must provide a valid " - + "String or DocumentReference, but it was of type: " - + Util.typeName(value)); + fieldValue = parseDocumentIdValue(value); } } else { if (op == Operator.IN || op == Operator.ARRAY_CONTAINS_ANY) { - if (!(value instanceof List) || ((List) value).size() == 0) { - throw new IllegalArgumentException( - "Invalid Query. A non-empty array is required for '" + op.toString() + "' filters."); - } - if (((List) value).size() > 10) { - throw new IllegalArgumentException( - "Invalid Query. '" - + op.toString() - + "' filters support a maximum of 10 elements in the value array."); - } - if (((List) value).contains(null)) { - throw new IllegalArgumentException( - "Invalid Query. '" - + op.toString() - + "' filters cannot contain 'null' in the value array."); - } - if (((List) value).contains(Double.NaN) || ((List) value).contains(Float.NaN)) { - throw new IllegalArgumentException( - "Invalid Query. '" - + op.toString() - + "' filters cannot contain 'NaN' in the value array."); - } + validateDisjunctiveFilterElements(value, op); } fieldValue = firestore.getDataConverter().parseQueryValue(value); } @@ -497,6 +381,144 @@ private void validateOrderByField(com.google.firebase.firestore.model.FieldPath } } + /** + * Parses the given documentIdValue into a ReferenceValue, throwing appropriate errors if the + * value is anything other than a DocumentReference or String, or if the string is malformed. + */ + private ReferenceValue parseDocumentIdValue(Object documentIdValue) { + if (documentIdValue instanceof String) { + String documentId = (String) documentIdValue; + if (documentId.isEmpty()) { + throw new IllegalArgumentException( + "Invalid query. When querying with FieldPath.documentId() you must provide a valid " + + "document ID, but it was an empty string."); + } + if (!query.isCollectionGroupQuery() && documentId.contains("/")) { + throw new IllegalArgumentException( + "Invalid query. When querying a collection by FieldPath.documentId() you must " + + "provide a plain document ID, but '" + + documentId + + "' contains a '/' character."); + } + ResourcePath path = query.getPath().append(ResourcePath.fromString(documentId)); + if (!DocumentKey.isDocumentKey(path)) { + throw new IllegalArgumentException( + "Invalid query. When querying a collection group by FieldPath.documentId(), the " + + "value provided must result in a valid document path, but '" + + path + + "' is not because it has an odd number of segments (" + + path.length() + + ")."); + } + return ReferenceValue.valueOf( + this.getFirestore().getDatabaseId(), DocumentKey.fromPath(path)); + } else if (documentIdValue instanceof DocumentReference) { + DocumentReference ref = (DocumentReference) documentIdValue; + return ReferenceValue.valueOf(this.getFirestore().getDatabaseId(), ref.getKey()); + } else { + throw new IllegalArgumentException( + "Invalid query. When querying with FieldPath.documentId() you must provide a valid " + + "String or DocumentReference, but it was of type: " + + Util.typeName(documentIdValue)); + } + } + + /** Validates that the value passed into a disjunctive filter satisfies all array requirements. */ + private void validateDisjunctiveFilterElements(Object value, Operator op) { + if (!(value instanceof List) || ((List) value).size() == 0) { + throw new IllegalArgumentException( + "Invalid Query. A non-empty array is required for '" + op.toString() + "' filters."); + } + if (((List) value).size() > 10) { + throw new IllegalArgumentException( + "Invalid Query. '" + + op.toString() + + "' filters support a maximum of 10 elements in the value array."); + } + if (((List) value).contains(null)) { + throw new IllegalArgumentException( + "Invalid Query. '" + + op.toString() + + "' filters cannot contain 'null' in the value array."); + } + if (((List) value).contains(Double.NaN) || ((List) value).contains(Float.NaN)) { + throw new IllegalArgumentException( + "Invalid Query. '" + + op.toString() + + "' filters cannot contain 'NaN' in the value array."); + } + } + + private void validateOrderByFieldMatchesInequality( + com.google.firebase.firestore.model.FieldPath orderBy, + com.google.firebase.firestore.model.FieldPath inequality) { + if (!orderBy.equals(inequality)) { + String inequalityString = inequality.canonicalString(); + throw new IllegalArgumentException( + String.format( + "Invalid query. You have an inequality where filter (whereLessThan(), " + + "whereGreaterThan(), etc.) on field '%s' and so you must also have '%s' as " + + "your first orderBy() field, but your first orderBy() is currently on field " + + "'%s' instead.", + inequalityString, inequalityString, orderBy.canonicalString())); + } + } + + private void validateNewFilter(Filter filter) { + if (filter instanceof RelationFilter) { + Operator filterOp = ((RelationFilter) filter).getOperator(); + List arrayOps = Arrays.asList(Operator.ARRAY_CONTAINS, Operator.ARRAY_CONTAINS_ANY); + List disjunctiveOps = Arrays.asList(Operator.ARRAY_CONTAINS_ANY, Operator.IN); + boolean isArrayOp = arrayOps.contains(filterOp); + boolean isDisjunctiveOp = disjunctiveOps.contains(filterOp); + + RelationFilter relationFilter = (RelationFilter) filter; + if (relationFilter.isInequality()) { + com.google.firebase.firestore.model.FieldPath existingInequality = query.inequalityField(); + com.google.firebase.firestore.model.FieldPath newInequality = filter.getField(); + + if (existingInequality != null && !existingInequality.equals(newInequality)) { + throw new IllegalArgumentException( + String.format( + "All where filters other than whereEqualTo() must be on the same field. But you " + + "have filters on '%s' and '%s'", + existingInequality.canonicalString(), newInequality.canonicalString())); + } + com.google.firebase.firestore.model.FieldPath firstOrderByField = + query.getFirstOrderByField(); + if (firstOrderByField != null) { + validateOrderByFieldMatchesInequality(firstOrderByField, newInequality); + } + } else if (isDisjunctiveOp || isArrayOp) { + // You can have at most 1 disjunctive filter and 1 array filter. Check if the new filter + // conflicts with an existing one. + Operator conflictingOp = null; + if (isDisjunctiveOp) { + conflictingOp = this.query.findOperatorFilter(disjunctiveOps); + } + if (conflictingOp == null && isArrayOp) { + conflictingOp = this.query.findOperatorFilter(arrayOps); + } + if (conflictingOp != null) { + // We special case when it's a duplicate op to give a slightly clearer error message. + if (conflictingOp == filterOp) { + throw new IllegalArgumentException( + "Invalid Query. You cannot use more than one '" + + filterOp.toString() + + "' filter."); + } else { + throw new IllegalArgumentException( + "Invalid Query. You cannot use '" + + filterOp.toString() + + "' filters with '" + + conflictingOp.toString() + + "' filters."); + } + } + } + } + } + /** * Creates and returns a new Query that's additionally sorted by the specified field. *