Skip to content
This repository has been archived by the owner on Mar 28, 2019. It is now read-only.

Service should only accept application/json content-types. #667

Merged
merged 10 commits into from
Feb 26, 2016

Conversation

Natim
Copy link
Contributor

@Natim Natim commented Feb 25, 2016

Refs: Kinto/kinto#461

  • The check should only happens with POST, PUT and PATCH verbs.

@Natim Natim force-pushed the should_reject_bad_content_type branch from af5c697 to 486dbe1 Compare February 25, 2016 15:13
@Natim
Copy link
Contributor Author

Natim commented Feb 25, 2016

@ayusharma Can you validate that this branch fixes your issue?

There are tests here to validate the integration with Kinto.

@Natim
Copy link
Contributor Author

Natim commented Feb 25, 2016

r? @almet

@almet
Copy link
Contributor

almet commented Feb 25, 2016

C'est bien.

@Natim
Copy link
Contributor Author

Natim commented Feb 25, 2016

Few questions:

  • Do you think I should add a test for every verb on records and collections endpoints (get/option/head/put/patch/post/delete)?
  • Is it ok to have the errno INVALID_PARAMETER for the 406 and 415 http codes Here

@ayusharma
Copy link
Contributor

Functionalities: Woking.

Test Case: 1 Fail for python2.7 and Errors for Python 3

Take a look at error trace : http://pastebin.com/f4YfNeZP

@@ -28,12 +28,27 @@ class ViewSet(object):

readonly_methods = ('GET', 'OPTIONS', 'HEAD')

content_types = ["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.

Since this is not used at the class level, I would suggest not to define it here but in a constant (repeating is even acceptable IMO).

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One may want to accept xml for his service, that's the reason why I added this configuration here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh you mean that it won't actually work if we update the content_type property.

@leplatrem
Copy link
Contributor

It looks good to me!

Do not hesitate to define new errors if no one matches (invalid parameter seems ok to me but I understand it's not very precise)

Theorically we should test very http method on both collection/record enpoints (motto no code should be here if no test fails when it's removed). But it's acceptable to leave it like you did.

If you could go a bit further with:

  • Update changelog (easier release)
  • Update resources docs to mention the possible status for invalid accept/content-type on put/post/patch

would be great :)

@Natim
Copy link
Contributor Author

Natim commented Feb 26, 2016

Do not hesitate to define new errors if no one matches (invalid parameter seems ok to me but I understand it's not very precise)

It seems to me that the http code is sufficient in that case. Don't you?

@leplatrem
Copy link
Contributor

r+

Natim added a commit that referenced this pull request Feb 26, 2016
…nt_type

Service should only accept application/json content-types.
@Natim Natim merged commit 4cd44dc into master Feb 26, 2016
@Natim Natim deleted the should_reject_bad_content_type branch February 26, 2016 11:00
@Natim Natim removed the in progress label Feb 26, 2016
@amotl
Copy link

amotl commented Feb 26, 2016

Didn't read the context here yet, but you referenced Cornices/cornice#343 here. Will have a look into that.

@Natim
Copy link
Contributor Author

Natim commented Feb 26, 2016

At first I though that I could use function based content-type to return application/json in case it was post/put/patch and None in case it was the other methods.

Finally I did it differently in Cliquet.

@amotl Do you think you could add a failing test in cornice?

@leplatrem leplatrem added this to the 3.0.0 milestone Feb 29, 2016
@leplatrem leplatrem added this to the 3.0.0 milestone Feb 29, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants