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

Vary presence and requirement of properties with CRUD operation #1497

Open
pplr opened this issue Mar 1, 2018 · 21 comments
Open

Vary presence and requirement of properties with CRUD operation #1497

pplr opened this issue Mar 1, 2018 · 21 comments

Comments

@pplr
Copy link

pplr commented Mar 1, 2018

Properties of a schema could be marked as readOnly or writeOnly if they should not be used respectively in a request or response. Additionally, required properties are present in the required field. Request vs response is the only criteria that can alter a resource schema.

This was not sufficient to describe our legacy REST APIs where presence and requirement of resource's properties is highly dependent of CRUD operation. The was due, in part, to wrong design choices. However, we faced generic situations that cannot be expressed for example:

  • Write once / immutable property: for properties like slug or identifier. Those properties should not be part of update request.
  • Server side default value: this kind of property is required only in response.

All this can be achieved using composition. I don't think that's a reasonable solution because:

  • contract is not more human readable as resources are split in many schema
  • for polymorphic resources (using oneOf), there is too many different schema to create and duplication is inevitable

We used following vendor extensions to be able to produce the documentation of our API:

Field Name Type Description
x-createOnly boolean Relevant only for Schema "properties" definitions. Declares the property as "create only". This means that it MAY be sent as part of a POST request but SHOULD NOT be sent as part of the response or any other request type. If the property is marked as x-createOnly being true and is in the required list, the required will take effect on the POST request only. A property MUST NOT be marked as both x-createOnly and readOnly, writeOnly, x-updateOnly, x-createForbidden or x-updateForbidden being true. Default value is false.
x-updateOnly boolean Relevant only for Schema "properties" definitions. Declares the property as "update only". This means that it MAY be sent as part of a PUT request but SHOULD NOT be sent as part of the response or any other request type. If the property is marked as x-updateOnly being true and is in the required list, the required will take effect on the PUT request only. A property MUST NOT be marked as both x-updateOnly and readOnly, writeOnly, x-createOnly, x-createForbidden or x-updateForbidden being true. Default value is false.
x-createForbidden boolean Relevant only for Schema "properties" definitions. Declares the property as "create forbidden". This means that it MAY be sent as part of a PUT request or be sent as part of the response but SHOULD NOT be sent as part of any other request type. If the property is marked as x-createForbidden being true and is in the required list, the required will take effect on the PUT request and the response only. A property MUST NOT be marked as both x-createForbidden and readOnly, writeOnly, x-createOnly, x-updateOnly or x-updateForbidden being true. Default value is false.
x-updateForbidden boolean Relevant only for Schema "properties" definitions. Declares the property as "create and read only". This means that it MAY be sent as part of a POST request or be sent as part of the response but SHOULD NOT be sent as part of any other request type. If the property is marked as x-updateForbidden being true and is in the required list, the required will take effect on the POST request and the response only. A property MUST NOT be marked as both x-updateForbidden and readOnly, writeOnly, x-createOnly, x-updateOnly or x-createForbidden being true. Default value is false.
x-requiredCreate [string] List properties required in the POST request.
x-requiredUpdate [string] List properties requires in the PUT request.
x-requiredRead [string] List properties required in the response.

Being able to describe requirement and presence of resource's properties based on usage context could be a great enhancement.

@handrews
Copy link
Member

handrews commented Mar 6, 2018

@pplr I want to explore your use case a bit with existing JSON Schema features. This also comes up in hyper-schema a lot so if possible I'd like to align Hyper-Schema (the spec for which I edit) and OpenAPI on this.

All this can be achieved using composition. I don't think that's a reasonable solution because:
contract is not more human readable as resources are split in many schema

I've seen this get unmaintainable when people treat the JSON Schema like an additive definition system, but it works well when JSON Schema is used for what it is: a constraint layering system.

The "base" schema should define all possible fields, and only mark as "required", "readOnly", etc. the ones that should meet that constraint in all situations.

Then for each usage, just layer on the "required", "readOnly", etc., and filter out parameters by setting their property schemas to {"not": {}} (the simplest impossible schema - in new JSON Schema you can make this more clear and encourage tools to optimize by setting a boolean schema of false, but that's no available in OpenAPI yet).

Example:

#/components/schemas/base:

{
    "required": ["alwaysHere"],
    "properties": {
        "id": {"type": "integer", "minimum": 0, "readOnly": true},
        "alwaysHere": {"type": "string"},
        "onlyOnCreate": {"type": "boolean"},
        "onlyOnUpdate": {"type": "string"},
        "sensitiveInformation": {"type": "string", "writeOnly": true}
    }
}

For POST to create:

{
    "allOf": [{"$ref": "#/components/schemas/base"}],
    "required": ["onlyOnCreate"],
    "properties": {
        "id": {"not": {}},
        "onlyOnUpdate": {"not": {}}
    }
}

For PUT requests:

{
    "allOf": [{"$ref": "#/components/schemas/base"}],
    "required": ["id"],
    "properties": {
        "onlyOnCreate": {"not": {}}
    }
}

For GET responses:

{
    "allOf": [{"$ref": "#/components/schemas/base"}],
    "required": ["id", "onlyOnUpdate"],
    "properties": {
        "onlyOnCreate": {"not": {}},
        "sensitiveInformation": {"not": {}}
    }
}

I think this covers most of your examples. It is more verbose, but it works and clearly shows the difference between the base and each usage. It also produces the behavior out of simple constraints rather than being a complex function of the extension keywords plus which other constraints are present with which value. In general, JSON Schema keywords that involve adjacent keywords are much harder to support and work with.

for polymorphic resources (using oneOf), there is too many different schema to create and duplication is inevitable

@pplr Is the duplication due to also using "additionalProperties": false or do you need to redefine properties across the oneOfs for other reasons? The next JSON Schema draft (which admittedly won't help OpenAPI 3.x) will have a keyword that fixes the problems of using "additionalProperties": false with the *Of keywords.

@mewalig
Copy link

mewalig commented Mar 7, 2018

I think PUT should be for creating a resource where the resource id is specified by the client/user (for example, a product model that has a user-specified model ID), PATCH for updating a resource, POST for creating something where the id will be specified by the server (for example, a purchase transaction, where multiple POSTs of the same data should create multiple transactions, with a server-generated unique transaction id) (in case helpful to reference: https://www.w3.org/Protocols/rfc2616/rfc2616-sec9.html and https://stackoverflow.com/questions/630453/put-vs-post-in-rest. I understand that often POST is used for broader purposes but in my view that approach is highly flawed and since this thread is not meant to cover that discussion I won't go into that here).

@MikeRalphson
Copy link
Member

As Mark Nottingham says:

Don’t use RFC2616. Delete it from your hard drives, bookmarks, and burn (or responsibly recycle) any copies that are printed out.

https://tools.ietf.org/html/rfc7231#section-4.3.4 covers PUT and s.4.3.3 POST

@handrews
Copy link
Member

handrews commented Mar 7, 2018

@mewalig this issue is really about describing existing APIs rather than best practices for designing new ones.

@pplr
Copy link
Author

pplr commented Mar 7, 2018

@handrews thank you for your help.

The way you use the not field is unusual.
Understanding "property": {"not": {}} meaning is not straightforward!

We use OpenAPI and JSON schema to help developers using our APIs.
We want to be closer as possible to the code they need to write.
When reading not: {}, it must be "translated" and coded as "forbidden field".

I agree with you that it totally covers my examples.

in new JSON Schema you can make this more clear and encourage tools to optimize by setting a boolean schema of false, but that's no available in OpenAPI yet

It will be possible to write this ?

{
    "allOf": [
        {"$ref": "#/components/schemas/base"},
        {
            "required": ["onlyOnCreate"],
            "properties": {
                "id": false,
                "onlyOnUpdate": false
            }
        }
    ]
}

@pplr Is the duplication due to also using "additionalProperties": false or do you need to redefine properties across the oneOfs for other reasons ?

I don't see any way to avoid duplication when the base resource is polymorphic, even with the "not": {} trick. For example:

{
    "required": ["alwaysHere"],
    "properties": {
        "id": {"type": "integer", "minimum": 0, "readOnly": true}
    },
    "oneOf": [
      {
        "properties": {
          "onlyOnUpdate": {"type": "string"},
          "alwaysHereA": {"type": "string"}
        }
      },
      {
        "properties": {
          "onlyOnCreate": {"type": "boolean"},
          "alwaysHereB": {"type": "string"}
        }
      }
    ]
}

@handrews
Copy link
Member

handrews commented Mar 7, 2018

@pplr

The way you use the not field is unusual.
Understanding "property": {"not": {}} meaning is not straightforward!

This is why in draft-06 we defined boolean schemas: true is equivalent to {}, and false is equivalent to {"not": {}}. This is exactly how true and false behave for additionalProperties and additionalItems, we just made that syntax legal anywhere a schema can appear.

So yes, in response to your "it will be possible to write this?", yes, that's exactly what it will look like.
It's a lot more clear, and is specifically intended to allow tooling to optimize false and true instead of having to guess about not. I'm bringing this up because it's important to not introduce more complex solutions for things that are solved quite simply by updating the JSON Schema support.

In the meantime, you can put {"not": {}} in a definition or as it's own schema file and do something like:

{
    "allOf": [{"$ref": "#/components/schemas/base"}],
    "required": ["id", "onlyOnUpdate"],
    "properties": {
        "onlyOnCreate": {"$ref": "#/components/schemas/forbidden"},
        "sensitiveInformation": {"$ref": "#/components/schemas/forbidden"}
    }
}

which gets the job done.

I don't see any way to avoid duplication when the base resource is polymorphic, even with the "not": {} trick. For example:

The not trick isn't the main technique for dealing with "polymorphism", but I need to understand more about your use case and what, exactly, you mean by "polymorphism" to tell you how to solve it (if it's solvable). In general, mapping OOP concepts directly into JSON Schema validation does not work well (hence awkward constructs like OpenAPI's discriminator). Are you trying to map OOP base/child classes into JSON Schema? If so can you give an example that produces too much duplication? It's hard to see these problems with trivial examples, they're too easy to refactor.

And, of critical importance: Are you trying to use "additionalProperties": false anywhere? That has a huge impact on how difficult this is.

@mewalig
Copy link

mewalig commented Mar 7, 2018

OK so use https://tools.ietf.org/html/rfc7231#section-4.3.4 instead. The same reasoning still applies.

@handrews re "this issue is really about describing existing APIs rather than best practices for designing new ones": I assume you are not suggesting that supporting existing practices means not supporting best practices where it is easy to support both. I can't imagine that OpenAPI would not want to support best practices. My only intention here is to suggest that any documented solution is presented in a manner that makes it clear how to apply the solution to a best-practices case (unless there are cases where the solution can't apply to both, which I can't think of).

@stueynz
Copy link

stueynz commented Mar 22, 2018

Now let's make a suitably realistic non-trivial base with structured objects rather than just a simple list of properties and figure out how we're going to make some nested properties required for certain operations.

#/components/schemas/Person:

{
       "properties": {
        "id": {"type": "integer", "minimum": 0, "readOnly": true},
        "name": {
            "type": "object",
            "properties" : {
                "firstName" : "string",
                "middleName" : {  "type" : "array", "items": "string" },
                "lastName" : "string"
            },
        },
       "demographics": { 
            "type" : "object",
            "properties" : {
                 "gender" : "string",
                 "ethnicity" : { "type" : "array", "items" : "string"}
            }
      }
}

...and then we have POST operation with body

{
   "schema" : {
       "allOf" : [ {"$ref: "#/components/schemas/Person" } ],
      
       // I bet this isn't allowed .... but it's what  I want to say
       "required" : [ "name.firstName", "name.lastName", "demographics.gender" ]
       ] 
   }
}

Is there a way of doing this with the current edition of OpenAPI / JSON Schema?

@handrews
Copy link
Member

@stueynz There is a deepRequired proposal for JSON Schema, although it probably won't be considered until draft-09 (which will quite possibly be out before OpenAPI updates JSON Schema support anyway, as draft-08 should be out in two months or so). https://github.com/json-schema-org/json-schema-spec/issues/203 if you want to see details. I've been skeptical of it recently but I keep going back and forth- it's definitely still in the running.

@mewalig I am trying to stay focused on describing APIs as they are, which includes both the legacy APIs such as @pplr specifically mentioned in the original comment, and APIs designed with best practices in mind. It's not necessary to debate best practices such as the proper use of PUT, POST, etc. in this issue- there are many forums for that.

@fantavlik
Copy link

And, of critical importance: Are you trying to use "additionalProperties": false anywhere? That has a huge impact on how difficult this is.

I'm only a year+ late this discussion 😃 but my team does have schemas that really should have "additionalProperties": false in practice but because of the limitations and lack of deepRequired in OA3 we are forced to omit this.

Thank you @handrews for the elegant "property": {"not": {}} pattern. One limitation of breaking out the schemas like this is that I think it sends a signal to code generation that you should generate three separate models for PUT requests v. POST requests v. responses using composition where some properties are removed and some are marked required.

The use-case we would like to support is to signal to code generation to use a single model for these three+ scenarios where marshaling/unmarshaling handles all of the property omission depending on the http method/read/write context. I think the x-createOnly, etc vendor extensions mentioned above would help us unify into a single schema to achieve this although I worry about that language becoming a bit verbose and also the PUT debate above re: definition of create vs update.

Here is a proposal for extending the readOnly and writeOnly properties with x-readMethods and x-writeMethods to be more specific when needed -- I doubt it has a place in JSON schema since it's tied to http methods but it seems like it could solve our use-case in OpenAPI schemas:

Legend:
put? means the field is optional for PUT
'*' means required for all methods
('*?' could mean optional for all methods?)

SingleModel:
  type: object
  properties:
    immutableProp:
      description: optional on create, invalid for update, required for responses
      type: integer
      x-readMethods: ['*'] # equivalent to [get,put,post,delete,options,head,patch,trace]
      x-writeMethods: [post?,put?]
    alwaysHere:
      description: rendered in all responses and requests
      type: string
      required: true
    requiredForCreate:
      type: string
      x-readMethods: ['*'] # equivalent to [get,put,post,delete,options,head,patch,trace]
      x-writeMethods: [post,put,patch?]
    onlyOnUpdate:
      type: string
      x-readMethods: ['*'] # equivalent to [get,put,post,delete,options,head,patch,trace]
      x-writeMethods: [patch?]
    sensitiveInformation:
      description: optional for create/update, never returned
      type: string
      writeOnly: true # equivalent to x-readMethods: []
    createdOn:
      description: only returned in responses
      type: string
      format: date-time
      readOnly: true # equivalent to x-writeMethods: []
      required: true

@handrews
Copy link
Member

@fantavlik 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.

@codyaray
Copy link

codyaray commented May 6, 2020

Definitely would like to see something like this built in to OpenAPI... sort of like how many tools know to ignore readOnly: true on POST/PUT/PATCH operations, we need something that says "ignore this on update (PUT/PATCH) but allowed it to be set". As suggested by the op, immutable is a good choice for this:

Write once / immutable property: for properties like slug or identifier. Those properties should not be part of update request.

Otherwise, we need separate requestBody schemas for "create" and "update" operations. :(

Similarly, I have a use case for "onlyOnCreateResponse" such as secret attributes that are only returned once, on create. In our use case, we have an API Key management but we only want to return the secret part of the API Key in the POST response.

@handrews
Copy link
Member

handrews commented May 7, 2020

@codyaray the way this can work is to write the minimally restrictive schema (which can be passed to a plain, non-HTTP-aware JSON Schema validator in the next version of OAS because it will have full JSON Schema compatibility), and then add keywords that let a later (an HTTP-aware validation pass) or separate step (code generation) further restrict the outcome.

JSON Schema is a constraint system. You can add constraints, you can't lift them (the "ignore required on readOnly+write is being dropped in the next OAS version because of this).

So write your schema such that it only requires the fields that are always required. And annotate it with which fields are further required in specific contexts. That will work with the larger tooling ecosystem of both OAS and JSON Schema. Attempting to lift requirements will not work.

@codyaray
Copy link

@handrews I used allOf to separate required out of the "main" schema. However, it doesn't really solve the "immutable" case cleanly. Because these schemas are used so frequently for code generation, I don't want to have a schema for Create and a schema for Update... normally, you'd just want to use the same object everywhere, but be able to annotate the object with whether or not each field can be updated.

@mewalig
Copy link

mewalig commented Jun 16, 2020

(update: changed "JSON Schema" to "Open API")

@codyaray I agree with you. How do we move this issue toward a solution? Assuming the general goal here is to describe a CRUD schema, including different requirements under different operations, in a manner that isn't overly repetitive or difficult to maintain, which of the below is the current status?

  1. Open API doesn't do that and there is currently no plan for it to do that
  2. Open API will do that, and someone has an idea of specifically how
  3. Open API will do that but we don't have a specific idea yet of how

if the answer is # 1, then the question is: what (if anything) can make this get into the plan? If nothing, and it will never be supported directly by Open API, then we should think about alternatives which might be some combination of Open API + annotations + custom code generator and/or Open API converter

if # 2, then, how is that going to work? What would the above CRUD example look like?

if # 3, then what is the process for identifying and evaluating different ways to proceed?

@avanbrunt-cb
Copy link

This definitely seems like a hole in the Open API specification. This #425 (comment) laid out the options well and showed how Microsoft solved their particular case.

Write only - e.g. secrets, which can be set, but you wouldn't want to return to the client.
Read only - e.g. server-generated values, such as created/modified timestamps
Read + write once - immutable properties, such as resource geographical location
Read + write many - I guess this is the normal case :)

A possible suggestion for a new specifier could be writeOnce which would indicate that the property becomes readOnly once it has been set. This could possibly include the case where a PUT with a new id creates the resource beyond limiting this to a POST for creation.

@ewhauser
Copy link

Just noting that Google also supports the writeOnce pattern in their APIs via their IMMUTABLE decorator:

https://github.com/googleapis/googleapis/blob/master/google/api/field_behavior.proto#L49

enum FieldBehavior {
  // Conventional default for enums. Do not use this.
  FIELD_BEHAVIOR_UNSPECIFIED = 0;

  // Specifically denotes a field as optional.
  // While all fields in protocol buffers are optional, this may be specified
  // for emphasis if appropriate.
  OPTIONAL = 1;

  // Denotes a field as required.
  // This indicates that the field **must** be provided as part of the request,
  // and failure to do so will cause an error (usually `INVALID_ARGUMENT`).
  REQUIRED = 2;

  // Denotes a field as output only.
  // This indicates that the field is provided in responses, but including the
  // field in a request does nothing (the server *must* ignore it and
  // *must not* throw an error as a result of the field's presence).
  OUTPUT_ONLY = 3;

  // Denotes a field as input only.
  // This indicates that the field is provided in requests, and the
  // corresponding field is not included in output.
  INPUT_ONLY = 4;

  // Denotes a field as immutable.
  // This indicates that the field may be set once in a request to create a
  // resource, but may not be changed thereafter.
  IMMUTABLE = 5;
}

@karlismelderis
Copy link

I'm curious how this will evolve now as OpenAPI 3.1 is trying to align with JSON Schema and they don't have writeOnce option in spec.

That said this is a gap that would be great to fix.
I've been burned quite enough where you should specify properties on Creating resource and never change them again.

Either we take some opinionated approach or try to get this "IMMUTABLE" feature added in JSON Schema, follow POST/PUT/UPDATE logic but something needs to be done.

I don't think we will be able to make everyone happy so let's try to agree on something that makes majority happy or make things heavily "flexible" and specify array of HTTP methods where property should be available.

@handrews
Copy link
Member

I like writeOnce a lot!

@karlismelderis with extensible vocabularies, it's not necessary for a keyword to get adopted into the main JSON Schema spec to be useful. You can declare and require extension vocabularies, such as the one that will be put out by OpenAPI with OAS 3.1 (of course, the implementation needs to support the vocabulary, but I expect OpenAPI's vocabulary to be supported pretty widely).

@YousefHaggy
Copy link

@handrews Do you think writeOnce can make it into 3.2 or 3.3 planning?

@handrews
Copy link
Member

@YousefHaggy we're trying not to add more OAS-custom fields to the Schema Object, and leave that instead to the JSON Schema project and JSON Schema's own extensibility mechanisms. This is still open because we haven't made a definitive decision- I thought we had something tracking that but now I can't find anything.

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

No branches or pull requests