Skip to content

NullPointerException in parser with additionalProperties: false #1437

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

Closed
MartinGeisse opened this issue Sep 11, 2015 · 5 comments
Closed

NullPointerException in parser with additionalProperties: false #1437

MartinGeisse opened this issue Sep 11, 2015 · 5 comments

Comments

@MartinGeisse
Copy link

I'm trying to restrict properties by setting additionalProperties: false. However, this seems to crash the YAML parser with a NullPointerException. When testing, please note that the most visible error is a syntax error from the JSON parser which kicks in after the YAML parser failed.

Main.java:

public class NpeDemo {
    public static void main(String[] args) {
        Swagger swagger = new SwaggerParser().read("npe-demo.yaml", null, true);
    }
}

npe-demo.yaml:

swagger: '2.0'

definitions:
  Demo:
    type: object
    additionalProperties: false
    properties:
      foo:
        type: string

paths:
  /demo:
    get:
      responses:
        200:
          description: blah

The problem happens in PropertyDeserializer.deserialize(JsonParser jp, DeserializationContext ctxt): The propertyFromNode(node) call returns null, which causes the NPE in the next line.

@webron
Copy link
Contributor

webron commented Oct 19, 2015

While it shouldn't throw NPE, please note that setting additionalProperties: false is not needed as unlike JSON schema, that's the assumption in Swagger. In a way, that's not even supported in the description and the schema validation may soon impose this restriction.

@fehguy
Copy link
Contributor

fehguy commented Oct 19, 2015

Parser is a separate project, and this is addressed in the latest parser snapshot, which will be released soon after swagger-core-1.5.4 is out.

@fehguy fehguy closed this as completed Oct 19, 2015
@tedepstein
Copy link

Hi @webron, I came across this comment while researching a related issue:

While it shouldn't throw NPE, please note that setting additionalProperties: false is not needed as unlike JSON schema, that's the assumption in Swagger. In a way, that's not even supported in the description and the schema validation may soon impose this restriction.

That's not consistent with the specification, which says additionalProperties "...is the same as the one from JSON Schema, only where the original definition references the JSON Schema definition, the Schema Object definition is used instead."

According to that specification, we would expect additionalProperties to have a default value of true, with the option to override it with a false value, or with a schema. Please let me know if I am missing anything here.

@webron
Copy link
Contributor

webron commented Jun 17, 2016

@tedepstein - you're right that it's inconsistent with what the spec says, but the actual error is in the spec as that was the implied intent which never made it to the docs (unfortunately). There may even still be an open ticket to change that in the doc in 2.0...

@tedepstein
Copy link

@webron OK. It looks like this is all getting revisited in OpenAPI 3.0 anyway.

For now, I'll assume the following.

  • additionalProperties has a default value of false
  • if specified, additionalProperties must have a value that is a valid OpenAPI Schema Object.
  • additionalProperties cannot have a value of true.
  • additionalProperties can be combined with regular, statically defined properties.
    • Note that there are some contexts in swagger-models that don't support this: ObjectProperty doesn't have additionalProperties, and MapProperty doesn't have regular properties. So in the current implementation, you can only use both regular properties and additionalProperties in a ModelImpl.
    • We will assume that this is incomplete implementation of the spec in Swagger-Models. We'll work around it where possible, and look to the OpenAPI 3.0 release for a more complete implementation.

Please let me know if any of this is not correct.

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

No branches or pull requests

4 participants