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

Request data extractors to allow posting other formats than JSON #192

Merged
merged 1 commit into from
Oct 28, 2013

Conversation

ghost
Copy link

@ghost ghost commented Oct 22, 2013

Ref. #186

Creating a PR so it can be easily reviewed. It's not quite ready yet because I haven't updated the docs.

Also I'm not happy with the name 'request_data_extractor', but couldn't think of anything better so far.

@ghost ghost mentioned this pull request Oct 22, 2013
@ghost
Copy link
Author

ghost commented Oct 23, 2013

Argh, test failures with Py3. Will look into it.

@ghost
Copy link
Author

ghost commented Oct 24, 2013

And here is the doc update :) What do you think?

BTW I had to comment-out the mozilla theme in docs/source/conf.py otherwise the docs wouldn't built.

@@ -478,6 +478,10 @@ def wrapper(request):
if is_string(view):
view_ = getattr(ob, view.lower())

# set data extractor
if 'request_data_extractor' in args:
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if it's not there? Shouldn't we fallback to a default value here?

Copy link
Author

Choose a reason for hiding this comment

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

This is for the view config parameter. If it's not there we fallback to the global dictionary keyed by content-type, which is populated with json and form-urlencoded decoders when when the app start.

@almet
Copy link
Contributor

almet commented Oct 24, 2013

I added a few comments here and there, thanks for the work.

About the sphinx theme, I'm fixing this right now :-)

@almet
Copy link
Contributor

almet commented Oct 24, 2013

Ready to be merged? Can you squash these commits into one and add an item in the changelog?

 * add built-in support for www-form-urlencoded data (ie.
   traditional HTML form submission)
 * allow specifying custom deserializers for other input types through
   app configuration or view configuration

Includes work by Alexandre Bourget aka @abourget
@ghost
Copy link
Author

ghost commented Oct 25, 2013

There you go.

@almet
Copy link
Contributor

almet commented Oct 28, 2013

great, thanks!

almet added a commit that referenced this pull request Oct 28, 2013
Request data extractors to allow posting other formats than JSON
@almet almet merged commit 9bd011b into Cornices:master Oct 28, 2013
requested content type asked by the client sending an ``Accept`` header.
Otherwise it will croak with a ``406 Not Acceptable``.


Copy link
Contributor

Choose a reason for hiding this comment

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

Hi Alex, Hi Alexis,

i added/rephrased this section in the documentation with PR #91 resolving issue #57. I believe this is still valid, since it's about "Content-Type" header validation, which happens before request body deserializing and is technically also completely independent from it.

Especially the constraints on the "Accept" http request header have nothing to do with the ingress request body, but rather with the egress response body.

Maybe we should rephrase the introductional sentence again, so instead of having:

Content validation

There are two flavors of content validations cornice can apply to services:

this could make it more clear:

Content-Type- and Accept-header validation

There are two flavors of constraints cornice can apply to http request headers:

What do you think?

Cheers,
Andreas.

@amotl
Copy link
Contributor

amotl commented Nov 3, 2013

Hi again, apparently this is still in master, i was mislead by the diff. Sorry for interrupting!

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.

3 participants