Refactor discriminator

justin-tay · justin-tay · commit 52210c4dd906 · 2025-09-26T21:31:11.000+08:00
diff --git a/src/main/java/com/networknt/schema/keyword/AnyOfValidator.java b/src/main/java/com/networknt/schema/keyword/AnyOfValidator.java
@@ -117,14 +117,14 @@ && canShortCircuit(executionContext)) {
                         boolean discriminatorMatchFound = false;
                         if (refNode != null) {
                             // Check if there is a match
-                            String discriminatingValue = state.getMappedSchema();
-                            if (discriminatingValue != null) {
+                            String mappedSchema = state.getMappedSchema();
+                            if (mappedSchema != null) {
                                 String ref = refNode.asText();
-                                if (state.isExplicitMapping() && ref.equals(discriminatingValue)) {
+                                if (state.isExplicitMapping() && ref.equals(mappedSchema)) {
                                     // Explicit matching
                                     discriminatorMatchFound = true;
                                     state.setMatchedSchema(ref);
-                                } else if (!state.isExplicitMapping() && ref.endsWith(discriminatingValue)) {
+                                } else if (!state.isExplicitMapping() && ref.endsWith(mappedSchema)) {
                                     // Implicit matching
                                     discriminatorMatchFound = true;
                                     state.setMatchedSchema(ref);
@@ -180,11 +180,13 @@ && canShortCircuit(executionContext)) {
                  * string values, but tooling MAY convert response values to strings for
                  * comparison.
                  * 
-                 * https://spec.openapis.org/oas/v3.1.0#discriminator-object
+                 * https://spec.openapis.org/oas/v3.1.2#examples-0
                  */
                 DiscriminatorState state = executionContext.getDiscriminatorMapping().get(instanceLocation);
-                if (state != null && state.getMatchedSchema() == null && state.getDiscriminatingValue() != null) {
-                    // state.getPropertyValue() == null means there is no value at propertyName
+                if (state != null && !state.hasMatchedSchema() && state.hasDiscriminatingValue()) {
+                    // The check for state.hasDiscriminatingValue is due to issue 988
+                    // Note that this is related to the DiscriminatorValidator by default not generating an assertion
+                    // if the discriminatingValue is not set in the payload
                     existingErrors.add(error().keyword("discriminator").instanceNode(node)
                             .instanceLocation(instanceLocation)
                             .locale(executionContext.getExecutionConfig().getLocale())
diff --git a/src/main/java/com/networknt/schema/keyword/DiscriminatorState.java b/src/main/java/com/networknt/schema/keyword/DiscriminatorState.java
@@ -1,3 +1,19 @@
+/*
+ * Copyright (c) 2025 the original author or authors.
+ *
+ * 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.networknt.schema.keyword;
 
 /**
@@ -48,6 +64,17 @@ public void setDiscriminatingValue(String discriminatingValue) {
         this.discriminatingValue = discriminatingValue;
     }
 
+    /**
+     * Returns ture if the discriminating value is found in the payload
+     * corresponding to the property name.
+     * 
+     * @return true if the discriminating value is found in the payload
+     *         corresponding to the property name
+     */
+    public boolean hasDiscriminatingValue() {
+        return this.discriminatingValue != null;
+    }
+
     /**
      * Gets the mapped schema which is the discriminating value mapped to the schema
      * name or uri.
@@ -68,19 +95,59 @@ public void setMappedSchema(String mappedSchema) {
         this.mappedSchema = mappedSchema;
     }
 
+    /**
+     * Gets whether the mapping is explicit using the mappings on the discriminator
+     * keyword.
+     * 
+     * @return true if the mapping is explicitly mapped using mappings
+     */
     public boolean isExplicitMapping() {
         return explicitMapping;
     }
 
+    /**
+     * Sets whether the mapping is explicit using the mappings on the discriminator
+     * keyword.
+     * 
+     * @param explicitMapping true if explicitly mapped using mappings
+     */
     public void setExplicitMapping(boolean explicitMapping) {
         this.explicitMapping = explicitMapping;
     }
 
+    /**
+     * Sets the matched schema $ref.
+     * 
+     * @param matchedSchema the matched schema $ref
+     */
     public void setMatchedSchema(String matchedSchema) {
         this.matchedSchema = matchedSchema;
     }
 
+    /**
+     * Gets the matched schema $ref.
+     * 
+     * @return the matched schema $ref
+     */
     public String getMatchedSchema() {
         return this.matchedSchema;
     }
+
+    /**
+     * Returns true if there was a schema that matched the discriminating value.
+     * <p>
+     * If the discriminating value does not match an implicit or explicit mapping,
+     * no schema can be determined and validation SHOULD fail. Therefore if this
+     * returns false validation should fail.
+     * <p>
+     * <a href="https://spec.openapis.org/oas/v3.1.2#examples-0">4.8.25.4
+     * Examples</a>
+     * 
+     * @return true if there was a schema that matched the discriminating value.
+     */
+    public boolean hasMatchedSchema() {
+        return this.matchedSchema != null;
+    }
+
+    
 }
diff --git a/src/main/java/com/networknt/schema/keyword/DiscriminatorValidator.java b/src/main/java/com/networknt/schema/keyword/DiscriminatorValidator.java
@@ -33,7 +33,13 @@
 import com.networknt.schema.SchemaContext;
 
 /**
- * {@link KeywordValidator} that resolves discriminator.
+ * {@link KeywordValidator} for discriminator.
+ * <p>
+ * Note that discriminator MUST NOT change the validation outcome of the schema.
+ * <p>
+ * <a href=
+ * "https://spec.openapis.org/oas/v3.1.2#discriminator-object">Discriminator
+ * Object</>
  */
 public class DiscriminatorValidator extends BaseKeywordValidator {
     /**
@@ -83,19 +89,31 @@ public void validate(ExecutionContext executionContext, JsonNode node, JsonNode
         DiscriminatorState state = null;
         DiscriminatorState existing = executionContext.getDiscriminatorMapping().get(instanceLocation);
         if (existing != null) {
+            /*
+             * By default this does not throw an exception unless strict for discriminator
+             * is set to true.
+             * 
+             * This default is in line with the fact that the discriminator keyword doesn't
+             * affect validation but just helps to filter the messages.
+             */
+            if (this.schemaContext.getSchemaRegistryConfig().isStrict("discriminator", Boolean.FALSE)) {
+                throw new SchemaException("Schema at " + this.schemaLocation
+                        + " has a discriminator keyword for which another discriminator keyword has already been set for at "
+                        + instanceLocation);
+            }
             /*
              * This allows a new discriminator keyword if the propertyName is empty or if
              * the propertyName value is the same as the existing one.
              * 
-             * This is not specification compliant. There shouldn't be multiple matching
-             * discriminator keywords for the same instance.
+             * In the specification the behavior of this is undefined. There shouldn't be
+             * multiple matching discriminator keywords for the same instance.
              * 
              * Also propertyName for the discriminator keyword should not be empty according
              * to the specification.
              */
             if (!"".equals(this.propertyName) && !existing.getPropertyName().equals(this.propertyName)) {
                 throw new SchemaException("Schema at " + this.schemaLocation
-                        + " is redefining the discriminator property that has already been set for "
+                        + " is redefining the discriminator property that has already been set for at "
                         + instanceLocation);
             }
             state = existing;
@@ -126,16 +144,26 @@ public void validate(ExecutionContext executionContext, JsonNode node, JsonNode
                 state.setMappedSchema(mappedSchema);
                 state.setExplicitMapping(true);
             } else {
-                // If explicit mapping not found use implicit value
-                state.setMappedSchema(discriminatingValue);
-                state.setExplicitMapping(false);
+                if (!state.isExplicitMapping()) { // only sets implicit if an explicit mapping was not previously set
+                    // If explicit mapping not found use implicit value
+                    state.setMappedSchema(discriminatingValue);
+                    state.setExplicitMapping(false);
+                }
             }
         } else {
             /*
              * The property is not present in the payload. This property SHOULD be required
              * in the payload schema, as the behavior when the property is absent is
              * undefined.
              */
+            /*
+             * By default this does not generate an assertion unless strict for
+             * discriminator is set to true.
+             * 
+             * This default is in line with the intent that discriminator should be an
+             * annotation and not an assertion and shouldn't change the result which was why
+             * the specification changed from MUST to SHOULD.
+             */
             if (this.schemaContext.getSchemaRegistryConfig().isStrict("discriminator", Boolean.FALSE)) {
                 executionContext.addError(error().instanceNode(node).instanceLocation(instanceLocation)
                         .messageKey("discriminator.missing_discriminating_value").arguments(this.propertyName).build());
diff --git a/src/main/resources/jsv-messages.properties b/src/main/resources/jsv-messages.properties
@@ -68,4 +68,5 @@ uniqueItems = must have only unique items in the array
 writeOnly = is a write-only field, it cannot appear in the data
 contentEncoding = does not match content encoding {0}
 contentMediaType = is not a content media type
-discriminator.missing_discriminating_value = required property ''{0}'' for discriminator not found 
+discriminator.missing_discriminating_value = required property ''{0}'' for discriminator not found
+discriminator.anyOf. 
diff --git a/src/test/java/com/networknt/schema/DiscriminatorValidatorTest.java b/src/test/java/com/networknt/schema/DiscriminatorValidatorTest.java
@@ -969,7 +969,10 @@ void anyOfRedefinedDiscriminatorAndDiscriminatorWithMissingPropertyName() {
         Schema schema = factory.getSchema(schemaData);
         List<Error> messages =  schema.validate(inputData, InputFormat.JSON);
         List<Error> list = messages.stream().collect(Collectors.toList());
+        // There should be no errors as discriminator should not affect the validation result of anyOf
+        // Although the matched schema has the following error
         // : required property 'numberOfBeds' not found
+        // There is still a schema in the anyOf that matches
         assertTrue(list.isEmpty());
     }
  
diff --git a/src/test/java/com/networknt/schema/OpenAPI30JsonSchemaTest.java b/src/test/java/com/networknt/schema/OpenAPI30JsonSchemaTest.java
@@ -30,12 +30,11 @@ private void runTestFile(String testCaseFile) throws Exception {
             try {
                 JsonNode testCase = testCases.get(j);
                 System.out.println("Test Case ["+(j+1)+"]: "+testCase.get("description"));
-                System.out.println(mapper.writerWithDefaultPrettyPrinter().writeValueAsString(testCase.get("schema")));
-                System.out.println("Tests:");
+                //System.out.println(mapper.writerWithDefaultPrettyPrinter().writeValueAsString(testCase.get("schema")));
                 ArrayNode testNodes = (ArrayNode) testCase.get("tests");
                 for (int i = 0; i < testNodes.size(); i++) {
                     JsonNode test = testNodes.get(i);
-                    System.out.println(test);
+                    System.out.println("> Test Data ["+(i+1)+"]: "+test);
                     JsonNode node = test.get("data");
                     JsonNode typeLooseNode = test.get("isTypeLoose");
                     // Configure the schemaValidator to set typeLoose's value based on the test file,
@@ -51,10 +50,10 @@ private void runTestFile(String testCaseFile) throws Exception {
 
                     if (test.get("valid").asBoolean()) {
                         if (!errors.isEmpty()) {
-                            System.out.println("---- test case failed ----");
-                            System.out.println("schema: " + schema);
-                            System.out.println("data: " + test.get("data"));
-                            System.out.println("errors:");
+                            System.out.println("---- Test Data ["+(i+1)+"] FAILED [Unexpected Errors] ----");
+                            System.out.println("> Schema: " + schema);
+                            System.out.println("> Data  : " + test.get("data"));
+                            System.out.println("> Errors:");
                             for (Error error : errors) {
                                 System.out.println(error);
                             }
@@ -63,16 +62,17 @@ private void runTestFile(String testCaseFile) throws Exception {
                     } else {
 //                    	System.out.println(mapper.writerWithDefaultPrettyPrinter().writeValueAsString(testCase.get("schema")));
                         if (errors.isEmpty()) {
-                            System.out.println("---- test case failed ----");
-                            System.out.println("schema: " + schema);
-                            System.out.println("data: " + test.get("data"));
+                            System.out.println("---- Test Data ["+(i+1)+"] FAILED [Unexpected Success] ----");
+                            System.out.println("> Schema: " + schema);
+                            System.out.println("> Data  : " + test.get("data"));
                         } else {
                             JsonNode errorCount = test.get("errorCount");
                             if (errorCount != null && errorCount.isInt() && errors.size() != errorCount.asInt()) {
-                                System.out.println("---- test case failed ----");
-                                System.out.println("schema: " + schema);
-                                System.out.println("data: " + test.get("data"));
-                                System.out.println("errors: " + errors);
+                                System.out.println("---- Test Data [" + (i + 1) + "] FAILED [Expected "
+                                        + errorCount.asInt() + " Errors but was " + errors.size() + "] ----");
+                                System.out.println("> Schema: " + schema);
+                                System.out.println("> Data  : " + test.get("data"));
+                                System.out.println("> Errors: " + errors);
                                 for (Error error : errors) {
                                     System.out.println(error);
                                 }
