Bug 1381509 - fix public api swagger errors #360
Conversation
| @@ -109,7 +109,7 @@ definitions: | |||
| product: | |||
| type: string | |||
| read_only: | |||
| type: ['boolean', 'null'] | |||
| type: boolean | |||
There was a problem hiding this comment.
I don't quite understand why adding "null" to most of the properties is necessary. As far as I know, the API always returns them, so why is null valid?
There was a problem hiding this comment.
I comment about this bellow.
auslib/web/public/client.py
Outdated
| elif '/update/5/' in request.url: | ||
| defaults = {'queryVersion': 5} | ||
| elif '/update/6/' in request.url: | ||
| defaults = {'queryVersion': 6} |
There was a problem hiding this comment.
Can you refactor this to avoid all the repetition around queryVersion? I think it's safe to just pull it out of the URL.
There was a problem hiding this comment.
You're right, I put this here and don't thought to avoid the repetition.
|
By debugging the code, I realized that that when we have a null value and response validation is enabled, the validation try to determine if the value is valid, how value is None the validation fails. So when we have a setup like this: "type: ['boolean', 'null']", this says to validator that the value can be null. @aksareen, probably you had this problem about response validation, do you remember if what I writed above is right? I thing that we can disable response validation, because we have control around what is returned. |
| url_params = {} | ||
| update_query_version('/update/x1/a/b/c.xml', url_params) | ||
| self.assertIn(version_key, url_params) | ||
| self.assertEqual(url_params[version_key], 0) |
There was a problem hiding this comment.
Good job with this test coverage.
|
@ex-dev it's slightly more complicated. I encountered this problem in both response and request body validations for multiple APIs. If I remember correctly there were few types of cases: The other is when fields themselves are defined in forms.py as NullableStringField where field can be passed in request body as null or string/bool/int. Hence these DB columns of these fields have Nullable=true in the DB schema , therefore the swagger schema needs to be able to accommodate this too. Third one was case when integer field like data_version or priority or backgroundRate were passed as string in requests but had to be explicitly processed and validated into integers and stored and returned as integers too. Hence type had to be defined to allow both integers and strings for the particular field in request json. Disabling response validation will not fix the issue of fields getting passed as both null and string in the request body either via the UI or clients or a script. |
|
This is it, the public API is more simple, so when I turned off the response validation, nothing broke on tests. The problem is, swagger editors like swagger UI, does not recognises "type" as a list of values (eg. [string, 'null']) and emits a warning. |
|
I think there might be a few things twisted up in this issue. My original concern was that I couldn't get client code generation tools working against the public swagger spec. I'm still not 100% sure if that's related to the response validation and null stuff you're talking about above. When it comes to response validation, I think I understand a bit better after reading swagger-api/swagger-core#459. It sounds like you're saying that if we enable response validation, we MUST allow null values in the models for any columns that may be null - do I have that right? I'd really like to understand this fully before we do anything about it - it's not a huge rush though. |
|
You right, if validation response is enabled, we must allow null values in the models, and we do that, by set type, for instance, to "[string, 'null']", but this value is not supported by swagger specification and raises a warning "'type' should be a string". If we set type to "type: string" and keep the validation enabled, we get a server error "None is not type string", I have a small example reproducing this here: |
|
Okay, so that means that Connexion supports importing specs that are not Swagger-compatible (we use lists of types in the admin side here: https://github.com/mozilla/balrog/blob/master/auslib/web/admin/swagger/api.yaml#L3018), which is really confusing. I think @aksareen told me this before..but I keep forgetting. So, it sounds like we have two options:
Does that sound right to you? |
|
This sounds right for me. When we talk about input parameters, connexion implemented a vendor extension to support nullables(spec-first/connexion#197). I understand that using a list of types is correct, but not by swagger spec. Other possible solution is remove entries that values is null, but probably will cause UI and client problems. BTW, if you want, I can open a PR only to handle the queryVersion problem. Nullable x required is a weird problem, it generate a lot of discussion on open API forums haha. |
|
OK, so I think the major point of confusion is that Connexion supports a superset of what Swagger/OpenAPI does. This means that things that will technically work in Balrog will not work with other OpenAPI tools (such as codegen ones). So, I think this means that the primary item of work here is making the spec OpenAPI 2.0 compatible -- which I think is the current state? I've also filed https://bugzilla.mozilla.org/show_bug.cgi?id=1387064 and https://bugzilla.mozilla.org/show_bug.cgi?id=1387063 as follow-ups. |
|
Yes, the only warning that we will have is "Partial path templating is not allowed.", due to "/update/3/{product}/{version}esrpre/{buildID}/{buildTarget}/{locale}/{channel}/{osVersion}/{distribution}/{distVersion}/update.xml" But this seems a bug with the editor: |
|
I kind of get the logic for disabling response validations. But if request validation is still on then there instances when field value can support both null and any primitive-non-null data type . Example : editing a rule , when we want to set a field as null we explicitly pass in request body as {'field_name' : null} or to another value {'field_name': 'value' } . This case requires two 'types' for a field: null and string. Also, UI is broken so it may send null values for some fields if UI isn't fixed before merging this change. Also some fields actually do support both null and string values , specifically ones defined in forms.py as NullableStringField can expect both string and null values. Also , am surprised no test cases fail especially since type is no longer a list (like this ['null', 'string']). Why didn't no test case fail especially the edit rules/release/permission ones since key's value can be set to both string and null in request.json ? |
|
@aksareen, no edit test case fails, because the public API does not have one. This PR will not broken the admin API because the endpoints are not common yet. For the admin API, probably will not can change the type list for a single type, due to requests as you say above. I did some tests with x-nullable and seems works only with parameters, not definitions. The swagger v3 does not accepts a list of types too, but was add a "nullable" flag, we'll have to wait a little to test this. I think that response validation can be disable for public API, once it have more chances to have external clients that use codegens. What you think, @aksareen, @mozbhearsum ? |
|
@ex-dev : so maybe there aren't any test cases for public API. If the public API invokes the same methods that the admin API does in the web layer i.e. invokes the methods in files having path (admin/views/*.py) then in that case we must have nullable fields. If the db layer is getting directly accessed from different common/public endpoint methods then I think it should be fine since the DB can support nullable but the new public swagger spec will restrict those nullable fields to support only a single field type. It looks good to me. |
|
I think @aksareen is correct that we may still have nulls in the public API endpoints. For example, most of the fields of a Rule can be null. This is 100% valid IMO - I certainly wouldn't want to not include the null fields when returning Rules. It would be very strange to see some Rules with osVersion set, and others without it, for example. I've been reading over the OpenAPI V3 spec a bit (which was released a couple of weeks ago: https://www.openapis.org/blog/2017/07/26/the-oai-announces-the-openapi-specification-3-0-0), and it seems like the "nullable" attribute would solve the main issue we've been discussing here. It also appears to support some additional jsonschema structures like "anyOf", which may solve some other issues we've been having. If I'm right about all of this, it seems like the best thing to do is to convert to 3.0 instead of trying to make this work with 2.0. Converting the public+shared endpoints would be a good way to trial it out, and then we can look at admin later (after admin's OpenAPI 2.0 implementation is complete). |
|
Hmmm, connexion may not support 3.0 yet :(. We might need to wait for spec-first/connexion#420 to convert. |
|
Waiting for connexion's support sounds good for me. We can close this PR, in the next wekeend, I'll open two new PR for this question:
We can continue in this way? |
|
That sounds like a good idea. Thank you both for working through all of this! |
This PR move the flask-defaults logic to url verification and disables the response validation.
The principal goal is keep balrog api more close with swagger spec.