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

Move request.body_as_json from middleware to decorator #1650

Merged
merged 1 commit into from Feb 17, 2015

Conversation

@asmacdo
Copy link
Contributor

commented Feb 17, 2015

Since the middleware runs on every endpoint, an empty or invalid body was defaulting to {}. When a body is required, an invalid or absent body should be raising a 400 and giving a helpful error. Instead it was running with a default value. This change moves this logic from the middleware to 2 decorators. json_body_required raises a 400 if the body does not contain valid JSON, json_body_allow_empty allows only an empty body or valid JSON. This does technically change the API because webpy was raising a 500 rather than a 400 in at least some of these cases, which I think represents a bug fix rather than a backwards incompatible change, particularly because it is listed this way in our docs.

Additionally, this creates a problem in the tests because the decorator cannot be mocked. The solution is to pass valid JSON through the decorator rather than setting the request.body_as_json directly.

We will need to do some hand testing before merge to ensure that the JSON body is actually required for each endpoint. If not, the decorator can be switched to json_body_allow_empty. See the DeleteOrphansActionView to see how this should work.

If a request body is not passed, the value of request.body_as_json will be None. If this should be an empty list or dict, this assignment should take place in the View.

Additionally this will solve https://bugzilla.redhat.com/show_bug.cgi?id=1191417

@pulpbot

This comment has been minimized.

Copy link
Member

commented Feb 17, 2015

Refer to this link for build results (access rights to CI server needed):
https://pulp-jenkins.rhev-ci-vms.eng.rdu2.redhat.com//job/unittest-pulp-pr/205/
Test PASSed.

@asmacdo asmacdo force-pushed the asmacdo:move-body-as-json branch from 818a0e3 to cee9286 Feb 17, 2015
@pulpbot

This comment has been minimized.

Copy link
Member

commented Feb 17, 2015

Refer to this link for build results (access rights to CI server needed):
https://pulp-jenkins.rhev-ci-vms.eng.rdu2.redhat.com//job/unittest-pulp-pr/206/
Test PASSed.

@asmacdo asmacdo self-assigned this Feb 17, 2015
@asmacdo

This comment has been minimized.

Copy link
Contributor Author

commented Feb 17, 2015

Webpy returns the following with invalid or empty bodies. The docs usually say 400 if one or more of the parameters is invalid, and I think that invalid JSON should fall under that category and should not raise a 500. The docs generally don't say anything about an empty body, so the idea is that if webpy returns a 400 or 500 for an empty body, django should return 400. If webpy returns a 200 on an empty body, django should do the same.

Summary of changes:

tldr If webpy returns django will return
Invalid JSON 500 400
Invalid JSON 404, 400 400
Invalid JSON 200 or 202 400
No body 500 400
No body 404, 400 400
No body 200, 202 200, 202

PUT /v2/consumer_groups/<consumer_group_id>/
Invalid: 500 -- Bug - docs say 400
Empty: 200

POST /v2/consumer_groups/<consumer_group_id>/actions/associate/
Invalid: 200
Empty: 200

POST /v2/consumer_groups/<consumer_group_id>/actions/unassociate/
Invalid: 200
Empty: 200

POST /v2/consumer_groups/<consumer_group_id>/actions/content/<action>/
Install
Invalid: 202
Empty: 202

Uninstall
Invalid: 202
Empty: 202

Update
Invalid: 202
Empty: 202

POST /v2/consumer_groups/<consumer_group_id>/bindings/
Invalid: 400
Empty: 400

POST /v2/content/actions/delete_orphans/
Invalid: 202
Empty: 202

PUT /v2/content/units/<type_id>/<unit_id>/pulp_user_metadata/
Never released.

POST /v2/events/
Invalid: 500 -- Bug, docs say 400
Empty: 400

PUT /v2/events/<event_listener_id>/
Invalid: 500 -- Bug, docs say 400
Empty: 200

POST /v2/permissions/actions/grant_to_user/
Invalid: 500 -- Bug, docs say 404, should be changed to 400
Empty: 400 -- appropriate but not in docs.

POST /v2/permissions/actions/revoke_from_user/
Invalid: 500 -- Bug, docs say 404, should be changed to 400
Empty: 400 -- appropriate but not in docs.

POST /v2/permissions/actions/grant_to_role/
Invalid: 500 -- Bug, docs say 404, should be changed to 400
Empty: 400 -- appropriate but not in docs.

POST /v2/permissions/actions/revoke_from_role/
Invalid: 500 -- Bug, docs say 404, should be changed to 400
Empty: 400 -- appropriate but not in docs.

POST /v2/roles/
Invalid: 500 -- Bug, docs say 400
Empty: 400

PUT /v2/roles/<role_id>/
Invalid: 500 -- Bug, docs say 404, should be changed to 400
Empty: 500 -- should be 400 also

POST /v2/roles/<role_id>/users/
Invalid: 500 -- Bug, docs say 400
Empty: 400 -- appropriate but not in docs.

@@ -96,3 +96,4 @@
" directory: %(path)s"), ['path'])
PLP1008 = Error("PLP1008", _("The importer type %(importer_type_id)s does not exist"),
['importer_type_id'])
PLP1009 = Error("PLP1009", _("Request data is not valid JSON"), [])

This comment has been minimized.

Copy link
@bmbouter

bmbouter Feb 17, 2015

Member

data is kind of an ambiguous term in HTTP land.

s/Request data is not valid JSON/The request body does not contain valid JSON/

import json


class ParseBodyMiddleware(object):

This comment has been minimized.

Copy link
@bmbouter
@@ -22,7 +22,6 @@
MIDDLEWARE_CLASSES = (
'django.middleware.http.ConditionalGetMiddleware',
'pulp.server.webservices.middleware.exception.DjangoExceptionHandlerMiddleware',
'pulp.server.webservices.middleware.parse_body.ParseBodyMiddleware',

This comment has been minimized.

Copy link
@bmbouter
@@ -10,7 +10,8 @@
from pulp.server.managers.consumer.group.cud import bind, unbind
from pulp.server.webservices.controllers.decorators import auth_required
from pulp.server.webservices.views.util import (generate_json_response,
generate_json_response_with_pulp_encoder)
generate_json_response_with_pulp_encoder,
must_have_json_body)

This comment has been minimized.

Copy link
@bmbouter

bmbouter Feb 17, 2015

Member

consider renaming this to json_body_required

@@ -56,6 +57,7 @@ def delete(self, request, consumer_group_id):
result = manager.delete_consumer_group(consumer_group_id)
return generate_json_response(result)

@must_have_json_body

This comment has been minimized.

Copy link
@bmbouter

bmbouter Feb 17, 2015

Member

Please reorder to make the auth_required statements always the top-most decorator. If a request isn't authorized I don't want to tell the user anything until they come back with proper authorization. The way this is written will have auth_required will have validation occur before authentication+authorization. This should be fixed throughout the PR.

This comment has been minimized.

Copy link
@asmacdo

asmacdo Feb 17, 2015

Author Contributor

good catch!

@@ -167,7 +170,7 @@ def post(self, request):
:raises: OperationPostponed when an async operation is performed
"""
all_orphans = request.body_as_json
all_orphans = request.body_as_json or {}

This comment has been minimized.

Copy link
@bmbouter

bmbouter Feb 17, 2015

Member

Do you want this to be treated as an expression? Maybe you want this to act like a ternary operator, but this is going to return True/False based on the or statement. Is that what you want?

This comment has been minimized.

Copy link
@asmacdo

asmacdo Feb 17, 2015

Author Contributor

This actually does work as a conditional assignment, and is considered idiomatic python I believe.

In [1]: request = None

In [2]: x = request or {}

In [3]: x
Out[3]: {}

A chain of ors will return the first truthy value, if none are truthy, it will return the last.

Likewise a chain of ands will return the last truthy value, if none are truthy it returns the first.

This works because the truthyness of the value stored in the variable will always match the boolean value of the conditional.

This comment has been minimized.

Copy link
@bmbouter

bmbouter Feb 17, 2015

Member

This makes sense, but it still looks like an expression. It's fine to leave as-is; you've shown it produces the correct result. I learned something. I'm more used to this style: https://docs.python.org/2.6/reference/expressions.html#conditional-expressions

This comment has been minimized.

Copy link
@asmacdo

asmacdo Feb 17, 2015

Author Contributor

I have actually just realized that this conditional will have to be used for every instance where we allow an empty body, so I am going to change this do be the default in the util rather than in each use.

s/request.body_as_json = None/request.body_as_json = {}/

@@ -102,6 +104,7 @@ def delete(self, request, event_listener_id):
return generate_json_response(None)

@auth_required(authorization.UPDATE)
@must_have_json_body

This comment has been minimized.

Copy link
@bmbouter

bmbouter Feb 17, 2015

Member

This is the correct decorator ordering.

mock_request = mock.MagicMock()
mock_request.body = '{"Valid": "JSON"}'

@must_have_json_body

This comment has been minimized.

Copy link
@bmbouter

bmbouter Feb 17, 2015

Member

I never considered using the decorator to decorate a function and then test it all within a test. neat.

@asmacdo asmacdo force-pushed the asmacdo:move-body-as-json branch from cee9286 to 0aa8776 Feb 17, 2015
@pulpbot

This comment has been minimized.

Copy link
Member

commented Feb 17, 2015

Refer to this link for build results (access rights to CI server needed):
https://pulp-jenkins.rhev-ci-vms.eng.rdu2.redhat.com//job/unittest-pulp-pr/211/
Test PASSed.

@asmacdo asmacdo assigned bmbouter and unassigned asmacdo Feb 17, 2015
@asmacdo

This comment has been minimized.

Copy link
Contributor Author

commented Feb 17, 2015

@bmbouter, ready for rereview.

@@ -10,7 +10,9 @@
from pulp.server.managers.consumer.group.cud import bind, unbind
from pulp.server.webservices.controllers.decorators import auth_required
from pulp.server.webservices.views.util import (generate_json_response,
generate_json_response_with_pulp_encoder)
generate_json_response_with_pulp_encoder,
json_body_allow_empty,

This comment has been minimized.

Copy link
@bmbouter

bmbouter Feb 17, 2015

Member

Maybe s/json_body_allow_empty/json_body_required_allow_empty/ At face value it seems that it just allows empty with no requirement enforced.

return func(*args, **kwargs)
return wrapper

json_body_allow_empty = functools.partial(json_body_required, allow_empty=True)

This comment has been minimized.

Copy link
@bmbouter

bmbouter Feb 17, 2015

Member

great use of partial

try:
request.body_as_json = json.loads(request.body)
except ValueError:
raise PulpCodedValidationException(error_code=error_codes.PLP1009)

This comment has been minimized.

Copy link
@bmbouter

bmbouter Feb 17, 2015

Member

It's strange to me that the coded validation exceptions aren't raised as: raise PulpException1009. I realize you did it consistent with how its already done, but the pre-existing behavior seems strange. No change required, just a comment.

@bmbouter

This comment has been minimized.

Copy link
Member

commented Feb 17, 2015

@asmacdo Do you think tests should be written that pass in invalid json and empty json and assert that the API does the right thing? I see the decorator code itself is tested which is good, but we don't assert that the right decorators are set on the right methods. Some invalid and empty tests could do that since they flow through the decorator. What do you think?

@bmbouter

This comment has been minimized.

Copy link
Member

commented Feb 17, 2015

After some IRC conversation, we can introduce those other tests some time in the future. We have never done those before so it would be a new thing versus the porting to Django that we are focused on. LGTM

@bmbouter bmbouter added the LGTM label Feb 17, 2015
@asmacdo asmacdo force-pushed the asmacdo:move-body-as-json branch from 0aa8776 to e4624f5 Feb 17, 2015
@pulpbot

This comment has been minimized.

Copy link
Member

commented Feb 17, 2015

Refer to this link for build results (access rights to CI server needed):
https://pulp-jenkins.rhev-ci-vms.eng.rdu2.redhat.com//job/unittest-pulp-pr/212/
Test PASSed.

asmacdo added a commit that referenced this pull request Feb 17, 2015
Move request.body_as_json from middleware to decorator
@asmacdo asmacdo merged commit 25ae34a into pulp:master Feb 17, 2015
1 check passed
1 check passed
default Merged build finished.
Details
@asmacdo asmacdo deleted the asmacdo:move-body-as-json branch Apr 27, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.