potion icon indicating copy to clipboard operation
potion copied to clipboard

io= flags other than read-only not respected in schema generation

Open boydgreenfield opened this issue 8 years ago • 7 comments

Currently, the io c and u flags are respected when POSTing to the resource, but not included in the generation of the schema. This breaks auto-documentation and auto-generated client libraries.

Specifically, io="r" works just fine and will include the field in the schema["properties"] with the flag "readOnly": True. But io="cr" will (rightly) drop the readOnly flag and balk if you try to update the field, but there's no indication in the schema that the field isn't allowed in PATCHes. Similarly, io="c" will not put the given field into the create method (which will have a targetSchema = "#") and will hide it from the main resource properties.

boydgreenfield avatar Sep 13 '16 05:09 boydgreenfield

Would this be addressed by dropping the "targetSchema": "#" part and instead having a schema there that includes only the properties that are available for the operation? Unfortunately JSON schema does not have any relevant flags beyond "readOnly".

lyschoening avatar Sep 26 '16 08:09 lyschoening

@lyschoening I believe that would be the required fix yes.

Separately... I saw you mention on Gitter that you might be adding Swagger support in the next version. Is that still something on your roadmap? (I ask because we're using and really enjoying Flask-Potion for our API, but want to make sure we have a handle on roadmap before announcing our v1 API using JSON schema if Swagger support is around the corner and that makes more sense; also happy to help if we can!)

boydgreenfield avatar Sep 26 '16 19:09 boydgreenfield

@boydgreenfield I'd like to support it, but there is one key issue with Swagger. Swagger only supports exactly one possible type per property. In particular, you can't have "nullable" properties. That is a problem when PATCHing items: How would you un-set a property that has already been set?

In our team we have been looking a lot at using Protocol Buffers, which have the same behavior, and I think we might come up with a library that would both support Protocol Buffers and JSON with a Swagger and/or JSON schema and to have Potion use the same code base we would do some magic to support nullable types on top of it. It's not certain that this will happen and it won't be ready any time soon. It may be possible to add Swagger schemas to Potion before that (contributions would be greatly appreciated!) but first the issue with PATCH needs to be figured out.

lyschoening avatar Sep 27 '16 11:09 lyschoening

Ah, I hadn't realized that... and indeed that's something of a rather large limitation.

It sounds like: (1) one could implement support for Swagger and then just have no nullable properties in the Potion resources; or (2) wait for it to perhaps land in OpenAPI v3.

Since (1) isn't that useful, so your wait and see approach seems very reasonable. Oof, that's too bad.

Ps. Link to Swagger discussion for future reference to those coming here

boydgreenfield avatar Sep 27 '16 17:09 boydgreenfield

I mentioned Protocol Buffers having the same issue. For update operations they solved it by adding an (optional) update_mask property, which essentially contains a list of the fields/properties that should be updated. If a field is listed but not provided, it is set to null.

It would be relatively straightforward to change the behavior of PATCH to do this. Then the Swagger schema would just be a matter of updating the schema routes.

lyschoening avatar Sep 27 '16 20:09 lyschoening

@lyschoening Interesting. There's some discussion of an x-nullable property in the Swagger discussion that might provide an opportunity for a similar workaround (haven't looked closely). Unfortunately, that appears to be a "vendor extension", i.e., probably has very patchy support by various Swagger clients.

We're really just interested in getting support for the broadest set of clients and also looking at the new Core API work. That doesn't appear to quite work out of the box with Potion's JSON schema flavor, but may be more readily addressed than Swagger since there is some JSON schema support.

boydgreenfield avatar Sep 27 '16 23:09 boydgreenfield

Core API seems neat. You could of course format your output to be compatible with its client, but since the project has very little adoption it will not give you a much broader audience. Swagger on the other hand has quite extensive backing, so if you want a standard with lots of tooling support, a Swagger-compatible API would be a good choice. The decision not to have nullable fields is sensible from a tooling perspective – strictly typed languages do not always support nullable primitive types anyway.

If you are interested in driving this forward, maybe you could – in a new branch – make the required changes listed above in a way that works with existing Swagger clients? Then I will be glad to figure out how to integrate this into Potion in a way that both JSON Schema and Swagger are supported. (As I said before, Swagger support will be something I will look at anyway, but there are a few other things I would otherwise focus on first).

lyschoening avatar Sep 28 '16 09:09 lyschoening