flake8
passed.
JSHint
passed.
Fix @webapi_request_fields to properly check inherited fields.
Review Request #8981 — Created May 31, 2017 and submitted
The decorator for defining fields for webapi requests,
@webapi_request_fields, was written to combine properly with
@augment_method_from, allowing fields to be inherited. This can be seen in
the documentation for various resources, where the new fields are properly
combined with old ones. Unfortunately, the code that actually checks the fields
at runtime was using the wrong variable, causing it to only look at fields
defined in the most top-level method. We didn't notice this until now because
most places that use this also passallow_unknown=True. This change fixes up
the decorator to check the combined fields lists.There's one complication added by the fact that if this decorator is applied
multiple times, the "inner" decorator doesn't know about the outer fields. I've
solved this by adding an assumption that if a field is present in kwargs, that
we've already parsed and validated its contents.
- Reproduced a regression in Review Board 2.5.10 which was caused by the
addition of a fields decorator in a subclass that shadowed parameters from
the superclass. After this change, I was once again able to use the
max-resultsandstartparameters that had been hidden. - Ran unit tests.
| Description | From | Last Updated |
|---|---|---|
|
Looks good to send to the customer, but we need a unit test before this goes in. |
 chipx86
|
|
|
Correct me if I'm wrong, but this means that request.GET or request.POST won't have the fields that are caught by … |
chipx86
|
|
|
E501 line too long (82 > 79 characters) |
 reviewbot
|
|
|
Might be nicer just to wrap somewhere within the parens. |
chipx86
|
|
|
Instead of quoting, use %r for formatting so that if its a string, quotes will be included, otherwise it will … |
 brennie
|
|
|
Docstring? |
brennie
|
- Description:
-
The decorator for defining fields for webapi requests,
@webapi_request_fields, was written to combine properly with@augment_method_from, allowing fields to be inherited. This can be seen inthe documentation for various resources, where the new fields are properly combined with old ones. Unfortunately, the code that actually checks the fields at runtime was using the wrong variable, causing it to only look at fields defined in the most top-level method. We didn't notice this until now because most places that use this also pass allow_unknown=True. This change fixes upthe decorator to check the combined fields lists. + + Unfortunately, it's not quite so simple as just checking the fields lists
+ defined on _validate, because the inner decorator will also run, with only+ the knowledge of the inner fields. This change makes it so each + @webapi_request_fieldsdecorator will loop through the fields that it knows+ about, validating the values and then removing them from the request's + QueryDict. The inner decorators will then see a filtered list. - Testing Done:
-
~ Reproduced a regression in Review Board 2.5.10 which was caused by the addition
~ of a fields decorator in a subclass that shadowed parameters from the ~ - Reproduced a regression in Review Board 2.5.10 which was caused by the
addition of a fields decorator in a subclass that shadowed parameters from
the superclass. After this change, I was once again able to use the
max-resultsandstartparameters that had been hidden.
~ - Ran unit tests.
- superclass. After this change, I was once again able to use the max-results- and startparameters that had been hidden. - Reproduced a regression in Review Board 2.5.10 which was caused by the
- Commit:
-
78f64065f700441dddb0fde4b673d074ecfbec9c3965ff4d9ed8da549ec812e74828c279d24cf2ca
- Commit:
-
3965ff4d9ed8da549ec812e74828c279d24cf2ca72d23197064448b3d11a25fdaf0c76c29ae4b248
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
-
-
Correct me if I'm wrong, but this means that
request.GETorrequest.POSTwon't have the fields that are caught by the validator. So, while the function will be called with, say,key='foo',request.GET['key']won't exist.If so, then this is going to cause some new regressions. I wonder if, rather than manipulating
request.GET/request.POST, we can make use ofparsed_request_fields. The decorated view (or the decorators in-between) will get this as a keyword argument. The decorator could simply skip any keys that it finds in there. It could then (and, either way, should) update the provided version ofparsed_request_fieldswith any new keys it handles after it builds its local version of the map and inserts it intokwargs. That will ensure that the view can, once called, see every single field parsed by every decorator.
- Change Summary:
-
Simpler solution that doesn't modify the QueryDict.
- Description:
-
The decorator for defining fields for webapi requests,
@webapi_request_fields, was written to combine properly with@augment_method_from, allowing fields to be inherited. This can be seen inthe documentation for various resources, where the new fields are properly combined with old ones. Unfortunately, the code that actually checks the fields at runtime was using the wrong variable, causing it to only look at fields defined in the most top-level method. We didn't notice this until now because most places that use this also pass allow_unknown=True. This change fixes upthe decorator to check the combined fields lists. ~ Unfortunately, it's not quite so simple as just checking the fields lists
~ defined on _validate, because the inner decorator will also run, with only~ the knowledge of the inner fields. This change makes it so each ~ @webapi_request_fieldsdecorator will loop through the fields that it knows~ There's one complication added by the fact that if this decorator is applied
~ multiple times, the "inner" decorator doesn't know about the outer fields. I've ~ solved this by adding an assumption that if a field is present in kwargs, that ~ we've already parsed and validated its contents. - about, validating the values and then removing them from the request's - QueryDict. The inner decorators will then see a filtered list. - Commit:
-
72d23197064448b3d11a25fdaf0c76c29ae4b248007d8b6c304679129accf8c272955b7aa615ac9d
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.