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

DRY up the PATCH <- POST <- GET inheritance chain #2201

Closed
am17torres opened this issue Apr 9, 2020 · 6 comments
Closed

DRY up the PATCH <- POST <- GET inheritance chain #2201

am17torres opened this issue Apr 9, 2020 · 6 comments
Labels
Moved to Moonwalk Issues that can be closed or migrated as being addressed in Moonwalk

Comments

@am17torres
Copy link

am17torres commented Apr 9, 2020

I think it's fair to say that a pretty common API pattern might include the following resource routes for a hypothetical model schema. If we were to define the models using the minimum OpenApi 3.0 Specification it would look something like this:

openapi: 3.0.0
info:
  title: My API
  version: '1.0.0'
paths:

  /model:
    post:
      summary: Create Model
      requestBody:
        $ref: '#/components/requestBodies/ModelPost'
      responses:
        201:
          $ref: '#/components/responses/Model'

  /model/{id}/:
    parameters:
      - $ref: '#/components/parameters/id'

    get:
      summary: Get Model by ID
      responses:
        200:
          $ref: '#/components/responses/Model'

    patch:
      summary: Update Model by ID
      requestBody:
        $ref: '#/components/requestBodies/ModelPatch'
      responses:
        200:
          $ref: '#/components/responses/Model'
          

components:

  parameters:
    id: 
      name: id
      in: path
      description: The id of the resource
      schema:
        type: string
      required: true
      
  requestBodies:
    ModelPost:
      content:
          application/json:
            schema:
              $ref: '#/components/schemas/ModelPost'
              
    ModelPatch:
      content:
          application/json:
            schema:
              $ref: '#/components/schemas/ModelPatch'
    
  responses:
    Model:
      description: The Model
      content:
        application/json:
          schema:
            $ref: '#/components/schemas/Model'
            
  schemas:
    ModelPatch:
      required:
        - prop1
      properties:
        prop1:
          type: string
          description: required POST property, required PATCH property
        prop2:
          type: string
          description: required POST property, optional PATCH property
        prop3:
          type: string
          description: optional POST property, optional PATCH property
    
    ModelPost:
      allOf:
        - $ref: '#/components/schemas/ModelPatch'
      required: 
        - prop2
    
    Model:
      allOf:
        - $ref: '#/components/schemas/ModelPost'
      required: 
        - id
      properties:
        id:
          type: string

In the case of PATCH we need to mark most, if not all, properties as optional. For POST we override the list of required properties to include the minimum set necessary for object creation. Finally, we need to create a third model which contains the id field after a successful POST. The third model containing the id property is also used as the response object for GET requests.

As I see it, this issue stems from how required properties are handled. We could significantly reduce the number of models if the required list was augmented to accepted one or more http verbs.

Instead having the two separate request bodies and three schemata above, we could instead have a model that looks something like this.

 requestBodies:
    Model:
      content:
          application/json:
            schema:
              $ref: '#/components/schemas/Model'
schemas:
    Model:
      required:
        # this works as before, it is required in all cases
        - prop1 
        # this property is only required on GET and POST routes
        - prop2: [GET, POST]
        # this property is only required on GET routes.
        - id: GET
      properties:
        id:
          type: string
          description: required GET property
        prop1:
          type: string
          description: required GET, POST and PATCH property
        prop2:
          type: string
          description: required POST property, optional PATCH property
        prop3:
          type: string
          description: optional for all verbs

This approach is fully backwards compatible with the existing spec and offers spec writers to cut back on boilerplate!

@handrews
Copy link
Member

In general, I agree that this is all way too much boilerplate, and when I last used OAS 3 I wrote a preprocessor that generated all of the boilerplate from a model and a list of verbs. Although for PATCH you should really be using a patch media type such as application/merge-patch+json or application/json-patch+json rather than just modifying the request schema.

As for this specific approach, you can't change required like that because it is part of JSON Schema, which is a separate project and doesn't know about HTTP methods. Nor should it- that's what Hyper-Schema is for, at least in theory.

However, OAS 3.1 will use JSON Schema draft 2019-09 or later, which has actual support for re-usable extension vocabularies, so something could be done with extra, rather than changed, keywords. The question then is how much you want to bake certain usage patterns into the OAS spec. OAS has generally avoided doing that, again in contrast to Hyper-Schema, which assumes/requires pretty strict adherence to HTTP resource interaction semantics. Notably, a lot more people use OAS than use Hyper-Schema :-)

I'd personally love to see some shorthand options for resources that actually fit the HTTP uniform interface, which is one of the main points of REST, but I'm not sure that fits with the current direction of OAS.

@hkosova
Copy link
Contributor

hkosova commented Apr 12, 2020

@chandra-gh
Copy link

I have been using OpenAPI for sometime and I feel that defining operation specific constraints in the API spec can be improved. Right now "operation constraints" need to be baked into the model leading to multiple copies of the same model - UserCreateModel, UserUpdateModel and UserReadModel. Each model has the same attributes but different requirements. Rather than baking the operation specific requirements into the model, it will be cleaner if the spec is extended to decouple operation specific requirements from the model.

My proposal is something along the following lines:

operation_foo:
  - input (or request)
      - model: user
      - constraints: user with an ID
  - output (or response)
      - model: user
      - constraints: user with an ID and email

operation_bar:
  - input (or request)
      - number: user_id
  - output (or response)
      - model: user
      - constraints: user with an ID, email and status

The model is defined once but the constraints vary by the operation.

@handrews
Copy link
Member

See #1622 for why method-specific validation behavior is a problem for JSON Schema compatibility, which is a goal of OAS 3.1.

While this approach is better in that it uses new keywords instead of changing the behavior of existing keywords (readOnly and writeOnly have been part of JSON Schema for several drafts now), it's still essentially defining a schema template and asking for multiple schemas to be generated behind-the-scenes. There needs to be a clear separation between what is and is not a JSON Schema or else people will try to run the not-quite-json-schema through a validator and it's not going to work because validators don't understand HTTP context. Nor should they.

@philsturgeon
Copy link
Contributor

philsturgeon commented Apr 28, 2020

This sounds like a DSL, of which there are quite a few. There's visual editors too. There's no reason to write OpenAPI by hand.

@handrews
Copy link
Member

This is being discussed for Moonwalk at OAI/sig-moonwalk#30. Since we can't make this sort of structural change in a 3.x release, I'm going to close this issue in favor of that Moonwalk discussion - please chime in there!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Moved to Moonwalk Issues that can be closed or migrated as being addressed in Moonwalk
Projects
None yet
Development

No branches or pull requests

5 participants