Improve error message for invalid field names (#985)

For replacement documents, it's now:
  "Field names in a replacement document can not start with '$' but '%s' does"
For update documents, it's now:
  "All update operators must start with '$', but '%s' does not"

JAVA-2114

Author: jyemin

bson/src/main/org/bson/AbstractBsonWriter.java

@@ -530,8 +530,9 @@ public void writeName(final String name) {
         if (state != State.NAME) {
             throwInvalidState("WriteName", State.NAME);
         }
-        if (!fieldNameValidatorStack.peek().validate(name)) {
-            throw new IllegalArgumentException(format("Invalid BSON field name %s", name));
+        FieldNameValidator fieldNameValidator = fieldNameValidatorStack.peek();
+        if (!fieldNameValidator.validate(name)) {
+            throw new IllegalArgumentException(fieldNameValidator.getValidationErrorMessage(name));
         }
         doWriteName(name);
         context.name = name;

bson/src/main/org/bson/FieldNameValidator.java

@@ -16,6 +16,9 @@
 
 package org.bson;
 
+import static java.lang.String.format;
+import static org.bson.assertions.Assertions.isTrue;
+
 /**
  * A field name validator, for use by BSON writers to validate field names as documents are encoded.
  *
@@ -30,6 +33,18 @@ public interface FieldNameValidator {
      */
     boolean validate(String fieldName);
 
+    /**
+     * Return the validation error message for an invalid field
+     *
+     * @param fieldName the field name
+     * @return the validation error message
+     * @throws IllegalArgumentException if fieldName is actually valid
+     */
+    default String getValidationErrorMessage(final String fieldName) {
+        isTrue(fieldName + " is valid", !validate(fieldName));
+        return format("Invalid BSON field name %s", fieldName);
+    }
+
     /**
      * Gets a new validator to use for the value of the field with the given name.
      *

bson/src/test/unit/org/bson/BsonWriterSpecification.groovy

@@ -384,7 +384,8 @@ class BsonWriterSpecification extends Specification {
         writer.writeString('bad-child', 'string')
 
         then:
-        thrown(IllegalArgumentException)
+        def e = thrown(IllegalArgumentException)
+        e.getMessage() == 'testFieldNameValidator error'
 
         where:
         writer << [new BsonBinaryWriter(new BasicOutputBuffer(), new TestFieldNameValidator('bad'))]
@@ -402,6 +403,11 @@ class BsonWriterSpecification extends Specification {
             fieldName != badFieldName
         }
 
+        @Override
+        String getValidationErrorMessage(final String fieldName) {
+            'testFieldNameValidator error'
+        }
+
         @Override
         FieldNameValidator getValidatorForField(final String fieldName) {
             new TestFieldNameValidator(badFieldName + '-child')

driver-core/src/main/com/mongodb/internal/validator/MappedFieldNameValidator.java

@@ -48,6 +48,11 @@ public boolean validate(final String fieldName) {
         return defaultValidator.validate(fieldName);
     }
 
+    @Override
+    public String getValidationErrorMessage(final String fieldName) {
+        return defaultValidator.getValidationErrorMessage(fieldName);
+    }
+
     @Override
     public FieldNameValidator getValidatorForField(final String fieldName) {
         if (fieldNameToValidatorMap.containsKey(fieldName)) {

driver-core/src/main/com/mongodb/internal/validator/ReplacingDocumentFieldNameValidator.java

@@ -21,6 +21,9 @@
 import java.util.Arrays;
 import java.util.List;
 
+import static com.mongodb.assertions.Assertions.assertFalse;
+import static java.lang.String.format;
+
 /**
  * A field name validator for documents that are meant for storage in MongoDB collections via replace operations. It ensures that no
  * top-level fields start with '$' (with the exception of "$db", "$ref", and "$id", so that DBRefs are not rejected).
@@ -34,13 +37,15 @@ public class ReplacingDocumentFieldNameValidator implements FieldNameValidator {
 
     @Override
     public boolean validate(final String fieldName) {
-        if (fieldName == null) {
-            throw new IllegalArgumentException("Field name can not be null");
-        }
-
         return !fieldName.startsWith("$") || EXCEPTIONS.contains(fieldName);
     }
 
+    @Override
+    public String getValidationErrorMessage(final String fieldName) {
+        assertFalse(validate(fieldName));
+        return format("Field names in a replacement document can not start with '$' but '%s' does", fieldName);
+    }
+
     @Override
     public FieldNameValidator getValidatorForField(final String fieldName) {
         // Only top-level fields are validated

driver-core/src/main/com/mongodb/internal/validator/UpdateFieldNameValidator.java

@@ -18,6 +18,9 @@
 
 import org.bson.FieldNameValidator;
 
+import static com.mongodb.assertions.Assertions.assertFalse;
+import static java.lang.String.format;
+
 /**
  * A field name validator for update documents.  It ensures that all top-level fields start with a '$'.
  *
@@ -32,6 +35,12 @@ public boolean validate(final String fieldName) {
         return fieldName.startsWith("$");
     }
 
+    @Override
+    public String getValidationErrorMessage(final String fieldName) {
+        assertFalse(fieldName.startsWith("$"));
+        return format("All update operators must start with '$', but '%s' does not", fieldName);
+    }
+
     @Override
     public FieldNameValidator getValidatorForField(final String fieldName) {
         return new NoOpFieldNameValidator();

driver-core/src/test/functional/com/mongodb/internal/operation/FindAndReplaceOperationSpecification.groovy

@@ -189,7 +189,8 @@ class FindAndReplaceOperationSpecification extends OperationFunctionalSpecificat
         execute(operation, async)
 
         then:
-        thrown(IllegalArgumentException)
+        def e = thrown(IllegalArgumentException)
+        e.getMessage() == 'Field names in a replacement document can not start with \'$\' but \'$inc\' does'
 
         where:
         async << [true, false]

driver-core/src/test/functional/com/mongodb/internal/operation/FindAndUpdateOperationSpecification.groovy

@@ -293,7 +293,8 @@ class FindAndUpdateOperationSpecification extends OperationFunctionalSpecificati
         execute(operation, async)
 
         then:
-        thrown(IllegalArgumentException)
+        def e = thrown(IllegalArgumentException)
+        e.getMessage() == 'All update operators must start with \'$\', but \'x\' does not'
 
         where:
         async << [true, false]

driver-core/src/test/functional/com/mongodb/internal/operation/MixedBulkWriteOperationSpecification.groovy

@@ -352,7 +352,44 @@ class MixedBulkWriteOperationSpecification extends OperationFunctionalSpecificat
         execute(operation, async)
 
         then:
-        thrown(IllegalArgumentException)
+        def e = thrown(IllegalArgumentException)
+        e.getMessage() == 'All update operators must start with \'$\', but \'a\' does not'
+
+        where:
+        [async, ordered] << [[true, false], [true, false]].combinations()
+    }
+
+    def 'when replacing an invalid document, replace should throw IllegalArgumentException'() {
+        given:
+        def id = new ObjectId()
+        def operation = new MixedBulkWriteOperation(getNamespace(),
+                [new UpdateRequest(new BsonDocument('_id', new BsonObjectId(id)),
+                        new BsonDocument('$set', new BsonDocument('x', new BsonInt32(1))), REPLACE)],
+                true, ACKNOWLEDGED, false)
+
+        when:
+        execute(operation, async)
+
+        then:
+        def e = thrown(IllegalArgumentException)
+        e.getMessage() == 'Field names in a replacement document can not start with \'$\' but \'$set\' does'
+
+        where:
+        [async, ordered] << [[true, false], [true, false]].combinations()
+    }
+
+    @IgnoreIf({ serverVersionLessThan(5, 0) })
+    def 'when inserting a document with a field starting with a dollar sign, insert should not throw'() {
+        given:
+        def operation = new MixedBulkWriteOperation(getNamespace(),
+                [new InsertRequest(new BsonDocument('$inc', new BsonDocument('x', new BsonInt32(1))))],
+                true, ACKNOWLEDGED, false)
+
+        when:
+        execute(operation, async)
+
+        then:
+        notThrown(IllegalArgumentException)
 
         where:
         [async, ordered] << [[true, false], [true, false]].combinations()

driver-core/src/test/unit/com/mongodb/internal/validator/ReplacingDocumentFieldNameValidatorTest.java

@@ -30,11 +30,6 @@ public void testFieldValidationSuccess() {
         assertTrue(fieldNameValidator.validate("ok"));
     }
 
-    @Test(expected = IllegalArgumentException.class)
-    public void testNullFieldNameValidation() {
-        fieldNameValidator.validate(null);
-    }
-
     @Test
     public void testFieldNameStartsWithDollarValidation() {
         assertFalse(fieldNameValidator.validate("$1"));