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

content_type not handled properly if it is a function #343

Closed
bomb-on opened this issue Nov 13, 2015 · 9 comments · Fixed by #350
Closed

content_type not handled properly if it is a function #343

bomb-on opened this issue Nov 13, 2015 · 9 comments · Fixed by #350
Labels

Comments

@bomb-on
Copy link

bomb-on commented Nov 13, 2015

I have defined view decorator in my resource:

@view(renderer='json', content_type='application/json')

and when I call it without Content-Type properly defined, I get a 415 error.

However, if I want to call a function instead of defining a string for content_type

@view(renderer='json', content_type=content_type)

and in some other file i have defined that function:

def content_type(request):
    # interact with request
    return 'application/json'

it won't produce error 415, but it will return a 200 response.

Why is this happening? For some reason I'm sure it was working few months ago when I defined it in this way...

@leplatrem
Copy link
Contributor

Why is this happening? For some reason I'm sure it was working few months ago when I defined it in this way...

I may be wrong, but I see nothing in the code base that would support providing content_type as a callable. That could be a relevant contribution IMO if you need it!

@bomb-on
Copy link
Author

bomb-on commented Nov 16, 2015

Hm, to be honest, I didn't go trough the code base, but I thought that this part of documentation is saying it is possible. Or is it only a service's feature?

@leplatrem
Copy link
Contributor

OH! You are right, the documentation explicitly says it should work! That's a bug then! (either in code or documentation)

@amotl
Copy link
Contributor

amotl commented Feb 26, 2016

This should have worked, right. Have been there the other day (see #91) so will have a look if nobody is already into it.

@amotl
Copy link
Contributor

amotl commented Feb 28, 2016

Hey there, i had a look and found the following things:

  • The documentation [1] is correct by stating

    The callable should return a list of accepted content types

    Here, @bomb-on tried to return a single internet media type as scalar, hence the misbehavior of Cornice

  • However, when specifying the list of accepted content types as plain value, Cornice allows lists and scalars

To improve the functionality:

[1] https://cornice.readthedocs.org/en/latest/validation.html#content-type-validation

Maybe consider relabeling this from "bug" to "enhancement".

@bomb-on
Copy link
Author

bomb-on commented Mar 3, 2016

Thanks for looking into this! I see you discussed it and changed the code base a bit, but just to check, does this mean that content_type can now accept functions?

@amotl
Copy link
Contributor

amotl commented Mar 3, 2016

@bomb-on: content_type always accepted functions, but the machinery was improved now to be able to cope with return values made of scalar strings. Before, it only accepted lists of media type strings but was poor on reporting that properly, so your use case failed somehow silently:

def content_type(request):
    # interact with request
    return 'application/json'

This should work now as we're using to_list(...) to always convert the callback return value to a list before propagating it internally.

@bomb-on
Copy link
Author

bomb-on commented Mar 3, 2016

@amotl ah, I understand now... Ok, thank you one more time guys for all the effort and keep up with good work ;)

@almet
Copy link
Contributor

almet commented Mar 3, 2016

thanks for the kind words :)

amotl added a commit to zerotired/cornice that referenced this issue Mar 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants