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

Adding merge construct #674

Closed
fehguy opened this issue Apr 28, 2016 · 13 comments
Closed

Adding merge construct #674

fehguy opened this issue Apr 28, 2016 · 13 comments

Comments

@fehguy
Copy link
Contributor

fehguy commented Apr 28, 2016

Propose that we introduce a construct from JSON Schema draft-5, but not exactly per the draft-5 proposal.

Background

The allOf construct requires that all members be proper JSON Schemas. For JSON-Schema draft-5,
a $merge operator was proposed. The behavioral differences between the two include:

  • merge members do not need to be valid schemas
  • the $merge operator allows for subtraction of values
  • the $merge operator has operational order, which allows for a defined behavior when keys clash and have different values
  • the $merge operator is pre-processed, constructing a (valid) schema before validation is actually performed

The JSON schema draft has a different syntax than proposed. Why? It's too verbose, and supports a source and with syntax (i.e. merge two objects). We need to merge multiple objects, and the source, with is more wordy than we need.

Examples

Similar to allOf, but with partial schemas:

definitions:
  Dog:
    $merge:
      - $ref: '#/definitions/Animal'
      - description: a dog

  Animal:
    type: object
    properties:
      numberOfFeet:
        type: integer
        format: int32

result:

definitions:
  Dog:
    properties:
      numberOfFeet:
        type: integer
        format: int32
    description: a dog

In addition, since we can use $merge without full schemas, I propose a fragments section where non-validated schemas can be used.

parameters:
  - $merge:
    - $ref: '#/fragments/skip'
    - in: query
      description: the total records to skip
      minimum: null

fragments:
  skip:
    # this is a partial schema!  It's not a proper parameter
    name: skip
    type: integer
    format: int32
    minimum: 0

The effective result:

parameters:
  - in: query
    description: the total records to skip
    name: skip
    type: integer
    format: int32
    # note the minimum is removed by the `null`!!!

Modeling a common pattern

components:
  definitions:
    PaginatedResult:
      type: object
      properties:
        totalRecords:
          type: integer
          format: int32
        cursorId:
          type: string
    UserPaginatedResult:
      $merge:
        - $ref: '#/components/definitions/PaginatedResult'
        - properties:
            data:
              type: array
              items:
                $ref: '#/components/definitions/User'
    User:
      properties:
        name:
          type: string

Effective result:

components:
  definitions:
    UserPaginatedResult:
      type: object
      properties:
        totalRecords:
          type: integer
          format: int32
        cursorId:
          type: string
        # note `data` is merged, not replacing `properties`!!!
        data:
          type: array
          items:
            $ref: '#/components/definitions/User'
@fehguy
Copy link
Contributor Author

fehguy commented Apr 28, 2016

Parent issue #579

@fehguy
Copy link
Contributor Author

fehguy commented Apr 28, 2016

This helps with #556, #537, #519, #629,

@fehguy
Copy link
Contributor Author

fehguy commented Apr 28, 2016

@OAI/tdc we can discuss in the next call

@webron
Copy link
Member

webron commented Apr 28, 2016

It looks like the first dog/animal example is missing the type in the response.

I think we're misusing terminology here and mixing json schema with the spec structure. Until now, we haven't used any JSON Schema directives outside the Schema object, and now we're 'borrowing' $mergeto the spec itself (merging parameters). Before anyone jumps, $ref's are not defined by JSON Schema.

In the JSON Schema scope, I don't see how there can be schemas that are not valid (and that would become valid with a $merge).

Of course, there's a tooling issue as well. I think it's a welcome change that will tackle several issues, but will it be widely supported?

@DavidBiesack
Copy link

@webron 's point reflects my view that I've tried to express in other issues --- the OAS should be expressed at an abstraction layer above the syntax language in which it is expressed, and not rely too much on tricks/semantics of that encoding. It should not rely on YAML constructs (see OAI/Overlay-Specification#38 Traits or Mixins) or JSON constructs except where natural/necessary. Of course in the past the ability to copy/paste directly from Swagger definitions into JSON schema draft 4 capable tools was important, and will continue to be (does OAS have any Guiding Prinicples?), and hence using the notation the JSON Schema uses (JSON Pointer, JSON Reference) makes sense. I would be very wary of adopting something from draft 5 without fully understanding it (and I've found JSON schema specs particularly hard to grok). Any deviations must be considered with utmost care and probably only adopted if there are simple transformations (and tools) from OAS notation to external standards such as JSON schema. If OAS deviates, it may be better to not use $merge to avoid confusion - instead, use a term that names the abstraction we want to use (whether that is $inheritance or $include or mixins/traits, etc.

@fehguy
Copy link
Contributor Author

fehguy commented Apr 28, 2016

not merging, lots to discuss on this still.

@webron
Copy link
Member

webron commented Jul 21, 2016

Tackling PR: #741

@handrews
Copy link
Member

@fehguy @webron Note that $merge remains the most deeply divisive proposal in JSON Schema (I'm on the against side, personally). It should be resolved one way or the other in draft-08

@handrews
Copy link
Member

handrews commented Dec 27, 2017

@fehguy @webron An update on $merge and draft-08: It is very unlikely that $merge will be added to JSON Schema. $merge causes many problems in both implementation and schema design, and at this point has few supporters. (Also paging @darrelmiller as this would be a major divergence and therefore relevant to #1443.)

We are still working through the details for draft-08, but the most likely approaches to support the use cases in the various issues reference here are:

  • We have formalized how annotations like description, default, and readOnly are collected, to allow more precise behavior. Part of this is to allow applications to define their own behavior on a per-keyword basis.
    • Some, like readOnly, are described in the specification (you OR the booleans).
    • Others, like default, provide enough information for applications to define their own behavior (such as "values in a parent schema override values in its children, whether inlined or referenced").
    • This avoids the need for even a more restricted form of $merge such as $use (for those who remember that proposal).
  • For Extra JSON Reference properties ignored - reduces reusability of references #556 (keywords adjacent to $ref are ignored):
    • This was done because $ref originally indicated that it was replaceable by the target schema, during which the entire reference object is replaced. As it turns out, replacement causes many problems.
    • We are changing this to delegation, meaning that the result (both the validation outcome and collecting annotations) of $ref is the result of its target schema, and it works with adjacent keywords just like everything else.
    • The goal is for $ref to behave more like people expect, rather than getting people to change their expectations.
  • For inheritance and composition difficulties arising from the general unsuitability of allOf for this purpose (as noted in #519):
    • We will add a new data definition / code generation vocabulary to supplement the validation vocabulary and address these use cases directly.
    • Keywords like allOf will still be involved, but additional keywords will solve the well-known problems (like identifying whether it represents inheritance, and if so which subschemas are bases for which other subschemas)
    • The additionalProperties + allOf problems will be addressed by a new keyword (shared by validation and data definition)

The data definition / code generation vocabulary should have its first draft either alongside of draft-08 or quickly following it. There are a lot of unknowns still, but there's also a lot of demand and interest which always helps move things along.

The advantage of the data definition vocabulary approach is that a schema using its terms is still usable as a standard validation schema. The data definition keywords provide extra information of interest to tools that generate classes, database tables, etc., without interfering with or diverging from validation.

@dolmen
Copy link

dolmen commented Dec 12, 2018

My openapi-preprocessor tool provides a $merge keyword with a strong implementation, but also a $inline keyword.

@handrews
Copy link
Member

@webron any chance we can close this as won't fix? $merge is dead as a doornail as far as JSON Schema is concerned, after an exhaustive multi-year 500+-comment discussion. Annotation collection, unevaluatedProperties, and potentially extension vocabularies solve the various use cases that this keyword was proposed for. The $merge keyword actually does not solve all of the use cases that people had, and is hugely problematic for JSON Schema's processing model.

@handrews
Copy link
Member

handrews commented Jan 23, 2020

Implementing it as a strict preprocessor as @dolmen notes is a viable approach. What's not viable is the complex double-lazy evaluation needed when you start getting fancy with combinations of $merge and $ref and recursion.

To the extent that that would be an official OAS thing, I think it would be folded in under the Overlays concept.

@handrews
Copy link
Member

I'm going to be daring and go ahead and close this for two reasons:

  1. It's well-established at this point that with the full adoption of JSON Schema in OAS 3.1, we don't want to diverge from it by adding $merge into the Schema Object.
  2. Outside of the Schema Object, this is best discussed in the context of the various overlay proposals, which are tracked elsewhere: RAML Overlay equivalent? Overlay-Specification#35 Yet another Overlay proposal Overlay-Specification#36 Unification of Overlays and Traits Overlay-Specification#39 (#1442 in particular duplicates much of this issue's discussion).

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

5 participants