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

Fix issue with posting with no content #120

Merged
merged 4 commits into from
May 18, 2017

Conversation

jas32096
Copy link
Contributor

@jas32096 jas32096 commented May 16, 2017

Check if there is no content before aborting the request.

This fixes issues with posts to endpoints that have no parameters i.e.

@routes.ItemRoute.POST('/mark_read')
    def mark_read(self, move_task) -> fields.DateTimeString():
        move_task.mark_read()
        return move_task.seen_at

Without this fix, you'd have one of two issues:

>> http POST :5000/v2/move_tasks/667/mark_read 'Authorization: Bearer '(cat mtoken) 'Content-type: application/json'
HTTP/1.0 400 BAD REQUEST
Content-Length: 111
Content-Type: application/json
Date: Tue, 16 May 2017 19:57:03 GMT
Server: Werkzeug/0.12.1 Python/3.6.1

{
    "message": "Failed to decode JSON object: Expecting value: line 1 column 1 (char 0)",
    "status": 400
}

>> http POST :5000/v2/move_tasks/667/mark_read 'Authorization: Bearer '(cat mtoken)
HTTP/1.0 415 UNSUPPORTED MEDIA TYPE
Content-Length: 60
Content-Type: application/json
Date: Tue, 16 May 2017 19:57:09 GMT
Server: Werkzeug/0.12.1 Python/3.6.1

{
    "message": "Unsupported Media Type",
    "status": 415
}

@coveralls
Copy link

coveralls commented May 16, 2017

Coverage Status

Coverage remained the same at 88.901% when pulling 6201adf on jas32096:master into 88b9352 on biosustain:master.

@Alain1405
Copy link
Contributor

Related to #94

# TODO change to request.get_json(silent=False) to catch invalid JSON
data = request.get_json(silent=True)

if request.method in ('POST', 'PATCH', 'PUT', 'DELETE'):
if data and request.mimetype != 'application/json':
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe data and request.mimetype != 'application/json' is always False. request.get_json() will always return None if the mimetype is not 'application/json' so RequestMustBeJSON never gets raised.

The check that should be made instead is if data is required, e.g. self.fields is not empty, require a JSON request otherwise allow an empty request.

if request.method in ('POST', 'PATCH', 'PUT', 'DELETE'):		
    if self.fields and request.mimetype != 'application/json':		
        raise RequestMustBeJSON()

This could be moved back into the top of the function.

A unit test should be added to make sure.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 88.901% when pulling c779367 on jas32096:master into 88b9352 on biosustain:master.

1 similar comment
@coveralls
Copy link

coveralls commented May 17, 2017

Coverage Status

Coverage remained the same at 88.901% when pulling c779367 on jas32096:master into 88b9352 on biosustain:master.

@coveralls
Copy link

coveralls commented May 17, 2017

Coverage Status

Coverage decreased (-0.3%) to 88.569% when pulling 4cdcfbc on jas32096:master into 88b9352 on biosustain:master.

@jas32096
Copy link
Contributor Author

apparently my test doesn't work in python 2

@coveralls
Copy link

coveralls commented May 17, 2017

Coverage Status

Coverage increased (+0.1%) to 89.012% when pulling 1644d36 on jas32096:master into 88b9352 on biosustain:master.

@lyschoening
Copy link
Contributor

Is this not something that can be tested with the regular test client?

@jas32096
Copy link
Contributor Author

@lyschoening Not without creating an entire test app, which seems frivolous. The Flask test client is better suited for Flask apps, then extensions.

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

Successfully merging this pull request may close these issues.

4 participants