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

Expand schemas #1

Closed
loganvolkers opened this issue Aug 4, 2015 · 4 comments
Closed

Expand schemas #1

loganvolkers opened this issue Aug 4, 2015 · 4 comments

Comments

@loganvolkers
Copy link

Original spec from here: jdorn/json-editor#49

{
  "type": "object",
  "allOf": [
    {
      "properties": {
        "key1": {
          "type": "string"
        }
      },
      "required": ["key1"]
    },
    {
      "properties": {
        "key2": {
          "type": "string"
        }
      },
      "required": ["key2"]
    }
  ]
}

Should produce the following after it's expanded:

{
  "type": "object",
  "properties": {
    "key1": {
      "type": "string"
    },
    "key2": {
      "type": "string"
    }
  },
  "required": ["key1","key2"]
}

Moved from APIDevTools/swagger-parser#15

@loganvolkers
Copy link
Author

@BigstickCarpet - is this a feature you'd be willing to include, or should I hack something in and submit a PR?

It seems like expanding an allOf would be included after the dereference step. It doesn't really make sense unless a schema has already been dereferenced.

@JamesMessinger
Copy link
Member

Ok, I finally got a chance to look into this today. Thank you for submitting the PR and writing some tests. That really helped me get a better understanding of the proposed behavior.

After reviewing the relevant specs (Swagger 2.0 and JSON Schema draft v4), I don't think it would be a good idea to implement this feature. The allOf keyword isn't intended to behave like inheritance in an object-oriented language. Its purpose is composition, not inheritance. It's a subtle difference, but an important one. An expand() method that combines multiple schemas into a single schema would not adhere to the specs.

The good news is that there are proposals to add true inheritance to JSON Schema draft v5. Possible solutions include the switch keyword, JSON Patch, and JSON Merge Patch. But I'll wait until one of those is officially part of the spec before implementing it here.

@loganvolkers
Copy link
Author

The intention of this PR was to support composition. Inheritance and polymorphism are not a goal, and are not implemented in any of the code.

It looks like the main contributors of Swagger agree with you about the technical implications of allOf, and that it is not a "merge".

There are many subtle technicalities when it comes to JSON Schema, allOf being one of them. The actual meaning of it is given a JSON structure that's to be validated against a JSON Schema containing allOf, that JSON structure must be validated against each of the schema described by the allOf array separately.

Source: swagger-api/swagger-js#188 (comment)

The swagger team has laid out a section on composition and inheritance polymorphism that goes above and beyond JSON Schema. While composition and inheritance and laid out as two separate things in the spec, only composition seems to be well supported -- polymorphism is in progress: swagger-api/swagger-js#567

@JamesMessinger
Copy link
Member

Thanks for linking to that conversation. I wasn't aware that the Swagger team already had pretty much this same conversation. Ultimately, I agree with @webron's stance that...

the meaning of allOf is not "merge", so we can't "merge" it to a single schema.

I think you and I are both in agreement then, that merging multiple schemas into a single schema based on the allOf keyword is a bad idea. It might be possible to do a merge based on the combination of allOf and descriminator, but even that is problematic and would require some clever coding and comprehensive testing.

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

Successfully merging a pull request may close this issue.

2 participants