-
-
Notifications
You must be signed in to change notification settings - Fork 762
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
Create abstract validator classes #1653
Conversation
f7c30ba
to
c6bc75c
Compare
@@ -291,7 +290,7 @@ def _transform_form(self, form_parameters: t.List[dict]) -> dict: | |||
|
|||
default = param.get("default") | |||
if default is not None: | |||
defaults[param["name"]] = default | |||
prop["default"] = default |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also needs to be added for each property separately, so if a body is sent without the property, the default value is added.
encoding=encoding, | ||
strict_validation=strict_validation, | ||
) | ||
self._uri_parser = uri_parser |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not completely happy with this. The __init__
arguments for the validators are fixed in the request_validation
, so it doesn't really make sense that individual validators define additional parameters.
On the other hand, we do support form data specifically, so I can live with it. Open for better solutions though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't have a better idea at the moment :)
We should provide a similar base class for the |
c6bc75c
to
3a2b000
Compare
Pull Request Test Coverage Report for Build 4308721132
💛 - Coveralls |
3a2b000
to
005ed0f
Compare
Added an |
840517e
to
aa47094
Compare
aa47094
to
4e8e57e
Compare
strict_validation: bool, | ||
uri_parser: t.Optional[AbstractURIParser] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just realized now that we are still using a uri_parser
for form data (i.e. request body), which is not ideal.
Can't remember completely, but it's actually to parse arrays and/or parameters that are specified multiple times?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The form data body is the same format as a query string, just sent as body. So I think it's logical to use the uri_parser
here as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, true for the case of application/x-www-form-urlencoded
but for multipart/form-data
the parameters are sent in different parts instead of query string.
But indeed, not necessarily as surprising as I thought (URI <> body) :)
encoding=encoding, | ||
strict_validation=strict_validation, | ||
) | ||
self._uri_parser = uri_parser |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't have a better idea at the moment :)
@@ -443,11 +446,8 @@ def test_nullable_parameter(simple_app): | |||
resp = app_client.put("/v1.0/nullable-parameters", content="null", headers=headers) | |||
assert resp.json() == "it was None" | |||
|
|||
resp = app_client.put("/v1.0/nullable-parameters", content="None", headers=headers) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can we delete this one now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In query parameters it's impossible to represent null
there:
?key=
could be interpreted as an empty string?key
is not clear as discussed in this issue:
Boolean query parameter with allowEmptyValue responds 400 insisting on=true
#1538
So connexion supports "null"
and "None"
as null values, so you can send:
?key=null
?key=None
(I'm actually not a fan of supporting"None"
here, since it's Python specific, but if we still need to support"null"
, the benefit of dropping it is probably too small.)
In json, null can be represented:
{
"key": null
}
or for a null
body (eg. try json.dumps(None)
):
"null"
So we don't need to support "None"
here.
We actually don't need to support "null"
here either, since we should always parse it before checking, in which case it's parsed to None
. This is why I replaced all is_null(body)
checks in the body validators with body is None
.
As promised, an
AbstractRequestBodyValidator
class. This has 2 advantages:Looking at the remaining logic in the subclasses, I'm quite happy with the current abstraction. A lot of the complexity comes from substituting the default body when no body is received.
I'll leave some additional comments in-line.