Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bug: null test missing for missing discriminator property. #985

Closed
GregoryEssertel opened this issue Mar 11, 2024 · 2 comments · Fixed by #988
Closed

bug: null test missing for missing discriminator property. #985

GregoryEssertel opened this issue Mar 11, 2024 · 2 comments · Fixed by #988

Comments

@GregoryEssertel
Copy link

Where:

String discriminatorPropertyValue = node.get(discriminator.getPropertyName()).asText();

To trigger:
schema:

    {
          "type": "object",
          "discriminator": { "propertyName": "name" },
          "oneOf": [
            {
              "$ref": "#/defs/Foo"
            },
            {
              "$ref": "#/defs/Bar"
            }
          ],
          "defs": {
            "Foo": {
              "type": "object",
              "properties": {
                "name": {
                  "const": "Foo"
                }
              },
              "required": [ "name" ],
              "additionalProperties": false
            },
            "Bar": {
              "type": "object",
              "properties": {
                "name": {
                  "const": "Bar"
                }
              },
              "required": [ "name" ],
              "additionalProperties": false
            }
          }
        }

value:

{}

I want to try to make a PR, what should be the expected behavior in this case. Ideas:
1 - returning an error similar to the missing required property, but specifying that it is the discriminator.
2 - ignoring discriminator and return all errors

@justin-tay
Copy link
Contributor

Thanks for raising the issue.

I think the simple fix is to have it fall back on

if (this.validationContext.getConfig().isOpenAPI3StyleDiscriminators()
&& (discriminator != null || executionContext.getCurrentDiscriminatorContext().isActive())
&& !executionContext.getCurrentDiscriminatorContext().isDiscriminatorMatchFound()) {
errors = Collections.singleton(message().instanceNode(node).instanceLocation(instanceLocation)
.locale(executionContext.getExecutionConfig().getLocale())
.arguments(
"based on the provided discriminator. No alternative could be chosen based on the discriminator property")
.build());
}

I don't really have an objection to a specific error message for the missing property, since the spec states If the discriminator value does not match an implicit or explicit mapping, no schema can be determined and validation SHOULD fail.

But if you want to do so then ideally the PR should have a proper fix, meaning that

  • Ensure that the missing property message raised is consistent for all the discriminator usages eg. allOf, anyOf, oneOf
  • The messages should all be localized (Currently even the current message isn't properly localized and would tack on an english message)

@justin-tay
Copy link
Contributor

After looking at this closer it looks like due to the following

To make the behavior consistent with the rest of the usages the logic needs to treat the discriminator as matching if the property is not present in the data and rely on the required validator to catch the missing discriminator.

I shall probably create the PR to resolve this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants