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

writeOnly and readOnly naming #1546

Closed
toobulkeh opened this issue Apr 16, 2018 · 18 comments
Closed

writeOnly and readOnly naming #1546

toobulkeh opened this issue Apr 16, 2018 · 18 comments
Labels

Comments

@toobulkeh
Copy link

toobulkeh commented Apr 16, 2018

It might be beneficial to have aliases for writeOnly and readOnly, as their use cases are often considered in the context of their reverse, per #425 (comment).

writeOnly is used for input fields (like passwords) on GET requests.

readOnly is used for server-generated output fields (like IDs) on POST requests.

May I suggest:

writeHidden to be aliased to readOnly

and

readHidden to be aliased to writeOnly

@handrews
Copy link
Member

@toobulkeh I think you have those flipped? A server-generated field that a client can't change would be readOnly, and a field that can be set but is never present in a GET response would be writeOnly.

It's not clear to me that aliasing fields is a good idea- it adds more complexity for tools to support. And really any code that uses an OpenAPI document would have to check for both. What is the benefit here?

@toobulkeh
Copy link
Author

toobulkeh commented Apr 16, 2018

Sorry about that, I've fixed my examples -- that's exactly why I'm requesting this!

In general, aliasing is a good thing for many object-oriented reasons, but in this particular case this aligns the verb write/read with the section of the spec it exists (eg writeHidden is located on POST responses).

Readability and programmer happiness are greater than some complexity of tooling/support (coming from the Ruby/Rails world).

Either way, thanks for the thought!

@silkentrance
Copy link

silkentrance commented Aug 4, 2018

Here is the relevant part of the spec

readOnly boolean Relevant only for Schema "properties" definitions. Declares the property as "read only". This means that it MAY be sent as part of a response but SHOULD NOT be sent as part of the request. If the property is marked as readOnly being true and is in the required list, the required will take effect on the response only. A property MUST NOT be marked as both readOnly and writeOnly being true. Default value is false.
writeOnly boolean Relevant only for Schema "properties" definitions. Declares the property as "write only". Therefore, it MAY be sent as part of a request but SHOULD NOT be sent as part of the response. If the property is marked as writeOnly being trueand is in the required list, the required will take effect on the request only. A property MUST NOT be marked as both readOnly and writeOnly being true. Default value is false.

On the server side, or even on the client side, the readOnly properties can be written once during deserialisation, whereas the writeOnly properties can written to but are no longer available when reading the same entity back from the server.

Whatever the use case is. Personally, I do not see it. Over specified is what comes to my mind...

Why over specify the whole shebang even further by introducing aliases for already highly redundant parts of the specification?

To be frank, this readOnly and writeOnly stuff is BS. One can easily just specify a request object and a response object. With the request object not including the properties that the client should not be able to modify and the response object containing all relevant data. SOC (separation of concern) and proper data modelling is what comes to my mind. Here we have the opposite of that, more like a swiss army knife.

And why should a client not be able to read back information that it originally provided, even if it is a password? This special casing goes over my head.

@toobulkeh
Copy link
Author

And why should a client not be able to read back information that it originally provided, even if it is a password? This special casing goes over my head.

Security 101 is that the password should never be known, even to the server. So if the server doesn't have a plaintext version of the password, then how can it send it back?

Back to the larger point:

The point of this request, and these variables in the first place, is ease of use. Why require a user type request objects and response objects for every single thing? Provide the user with a some helpful tools to make their jobs easier.

Maybe I'm asking this in the wrong place, and a tool could translate this to the "spec" - but that's just me not understanding the organization behind OAI and the toolchains.

@handrews
Copy link
Member

handrews commented Aug 6, 2018

readOnly comes from JSON Schema, where it has existed for quite a few years. It will not be changed there. We adopted writeOnly from OpenAPI as it made sense, and I see no reason to change it. There are always disagreements on what naming is ideal, but we have rarely if ever gotten complaints on readOnly (writeOnly is still new, but has also not attracted complaints for JSON Schema).

JSON Schema is slightly different in that it leans towards ignoring fields when they are supplied, but it generally defers behavior to the application, so OpenAPI specifying more implementation details is still compatible with JSON Schema.

@MikeRalphson
Copy link
Member

@silkentrance

To be frank, this readOnly and writeOnly stuff is BS

Please try and keep comments civil and respectful.

@tedepstein
Copy link
Contributor

tedepstein commented Aug 9, 2018

whereas the writeOnly properties can written to but are no longer available when reading the same entity back from the server.
...
Whatever the use case is. Personally, I do not see it. Over specified is what comes to my mind...
...
And why should a client not be able to read back information that it originally provided, even if it is a password? This special casing goes over my head.

@silkentrance , I personally don't have a strong opinion about writeOnly, and I don't think anyone here is proposing to remove it. If it's not useful to you, feel free to ignore it. But if you do want the background, there's discussion about it in #425, and I'm sure in related issues and PRs.

One can easily just specify a request object and a response object. With the request object not including the properties that the client should not be able to modify and the response object containing all relevant data. SOC (separation of concern) and proper data modelling is what comes to my mind. Here we have the opposite of that, more like a swiss army knife.

I don't agree. The readOnly and writeOnly annotations don't change the data model; they annotate the data model with hints about how they should be handled in client-server interactions.

Creating an entirely separate schema for the request vs. the response is a high cost to pay. IMO it doesn't improve the data model; it degrades the data model by making it more verbose, and more redundant (or less DRY).

If SOC is the goal, I think we're much better off with annotations, which are at most a minor, maybe even negligible intrusion of API concerns into the data model; rather than forcing a complete reorganization of the data model to fit the API.

You can use allOf composition, but that still adds syntactic complexity, making the data model more fragmented, less readable. Also, the semantics of allOf are really not ideally suited to this kind of data model composition. While most tools and libraries, in most respects, will process allOf as a schema that merges the properties of the included subschemas, that isn't what allOf really means. And it interacts badly with additionalProperties, which is why the JSON Schema introduced unevaluatedProperties in a later release (not supported in OAS3).

Even if readOnly and writeOnly aren't strictly necessary, I think it's fair game for OpenAPI to support them as syntactic sugar to address common idioms in API design, making API descriptions more concise and more readable.

That said, I don't like the idea of introducing aliases for these. What's intuitive for the person writing the API description might not be intuitive for the person reading it. I think it's better to keep the language well-bounded and agree on the vocabulary within that boundary.

@tedepstein
Copy link
Contributor

@silkentrance , to put this in context, RAPID-ML takes a much more radical approach to SOC, using realizations to adapt a domain model to the API context.

This makes sense for RAPID-ML, because the domain model consists of data structures that are specified at the conceptual level, not intended to be used as physical schemas. Realizations form a part of the API specification language, making it easy to use a subset of properties, add constraints, and handle references as embedded objects and/or hyperlinks.

So when I say that readOnly is valuable, I'm saying that in the context of OpenAPI as we know it today. Maybe someday it will make sense to propose something like RAPID-ML realizations as a new feature of OpenAPI and/or JSON Schema. Until then, I think a lot of OpenAPI users would have a hard time living without readOnly.

@silkentrance
Copy link

silkentrance commented Aug 9, 2018

@tedepstein I mostly agree with you here, however, the DRY principle can easily be achieved by defining separate request and response object schemas, e.g.

definitions:
  Request:
    type: object
    properties:
      payload:
        $ref: ./#definitions/RequestPayload
   Response:
    type: object
    properties:
      payload:
        $ref: ./#definitions/ResponsePayload
  ReqResPayloadBase:
    type: object
    properties:
      timestamp:
        type: string
        format: datetime
      username:
        type: string
   RequestPayload:
      allOf:
        - $ref: ./#definitions/ReqResPayloadBase
        - type: object
           properties:
             password:
                type: string
    ResponsePayload:
      allOf:
        - $ref: ./#definitions/ReqResPayloadBase
        - type: object
           properties:
             status:
                type: string
                enum: ['SUCCESS', 'INVALID_PASSWORD', 'UNKNOWN_USER']

Given the above approach, which is in no way perfect and just OTOH, one would not need the readonly or writeonly hints.

But maybe this is just my way of thinking.

And, considering the approach using readonly / writeonly, one would instead do the following

definitions:
  ReqRes:
    type: object
    properties:
      payload:
        $ref: ./#definitions/ReqResPayload
  ReqResPayload:
    type: object
    properties:
      status:
         readonly: true
         type: string
         enum: ['SUCCESS', 'INVALID_PASSWORD', 'UNKNOWN_USER']
      password:
         type: string
         writeonly: true  
      timestamp:
        type: string
        format: datetime
      username:
        type: string

And I do not see any benefit in that, especially when it comes to code generation and determining the context in which each instance of the ReqResPayload objects is being used in.

Regardless, and since both readonly and writeonly are purely optional, I do not have any problems with these.

@handrews
Copy link
Member

handrews commented Aug 9, 2018

@silkentrance but that is not a universal way to structure APIs. For example, since HTTP semantics imply that doing a GET and immediately PUT-ing the exact response back should be a no-op (assuming no intervening changes and no side effects like a last-request-received field, which sometimes show up despite technically not being idempotent).

For those of us who support that read-modify-write cycle with GET and PUT, readOnly and writeOnly are essential to set expectations. Attempting to change a read-only field will either be ignored or cause an error (I like to use the Prefer header to control this behavior), and clients should never treat a missing write-only field as an error even if it is required when sent back in a PUT.

There are many uses of these fields that help implement proper use of HTTP without adding complexity, and your distaste for them does not invalidate everyone else's use cases. Many of us see a lot of benefit.

@silkentrance
Copy link

Also, regarding SOC, IMO the second approach is less clear about that while the first approach makes it absolutely clear.

@silkentrance
Copy link

@handrews Still it poses a lot of additional effort (and time, considering larger object trees) on behalf of the backend, figuring out in which context GET/POST/PUT a given object is being used in.

@tedepstein
Copy link
Contributor

@silkentrance , I don't agree at all with most of what you're concluding here. But I also think we have accidentally hijacked this issue, which is really supposed to be able the naming of readOnly and writeOnly.

If you really think those features are harmful, and shouldn't be included in OpenAPI, you can open a separate issue to propose changes. If you think they're harmful, but don't think it's necessary or realistic to remove them from OAS, maybe a blog post, stack overflow article, or some other forum would be better.

@silkentrance
Copy link

@tedepstein Yes, but, again, since the attributes are optional, I do not mind having them. As for the issue at hand, I think that I made it perfectly clear that we do not need yet another indirection.

@silkentrance
Copy link

@tedepstein and given the OP's nick toobulkeh it seems that he was also making fun of whoever he wanted to make fun of.

@toobulkeh
Copy link
Author

What does my handle have to do with anything?

For those that are still interested in discussing this issue, I would still appreciate this helper/alias.

@MikeRalphson
Copy link
Member

Having aliases / multiple ways of achieve the same thing is basically not the way the OAS is written. It would be a burden, cognitively (i.e on support) and on tooling.

@MikeRalphson
Copy link
Member

The direction the OAS is taking is to further the alignment of the schema object with JSON Schema, not more divergence.

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

No branches or pull requests

5 participants