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

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

Open
boydgreenfield opened this issue Sep 13, 2016 · 7 comments
Open

Comments

@boydgreenfield
Copy link
Contributor

boydgreenfield commented Sep 13, 2016

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.

@lyschoening
Copy link
Contributor

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".

@boydgreenfield
Copy link
Contributor Author

@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!)

@lyschoening
Copy link
Contributor

lyschoening commented Sep 27, 2016

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

@boydgreenfield
Copy link
Contributor Author

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

@lyschoening
Copy link
Contributor

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.

@boydgreenfield
Copy link
Contributor Author

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

@lyschoening
Copy link
Contributor

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).

bjornt added a commit to bjornt/potion that referenced this issue Nov 16, 2018
This partly fixes issue biosustain#98, where a field that has io="cr" wouldn't
show up in the "create" schema.
lyschoening pushed a commit that referenced this issue Nov 28, 2018
This partly fixes issue #98, where a field that has io="cr" wouldn't
show up in the "create" schema.
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

2 participants