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_types_provided should be allowed to return the empty list when no Accept header is specified #1640

Open
asabil opened this issue Mar 18, 2024 · 6 comments

Comments

@asabil
Copy link
Contributor

asabil commented Mar 18, 2024

The implementation today unconditionally returns a 406 if the content_types_provided callback for cowboy_rest:

{[], Req2, State2} ->
    not_acceptable(Req2, State2);

I think that it should take into account the situation where the client doesn't specify any Accept header, in which case it should skip the content negotiation.

@essen
Copy link
Member

essen commented Mar 19, 2024

If you don't provide content then doing a GET request will always fail, so I'm not sure what you are trying to say. What is the expected response?

Cowboy does handle the case where accept isn't defined when content is provided. https://github.com/ninenines/cowboy/blob/master/src/cowboy_rest.erl#L476-L486

Content negotiation is also useful for other methods. For example if you return a body following a POST request, it can tell you which content type to use.

The only cases where we don't care about content negotiation is when the resource never returns a body. In that case simply do not implement content_types_provided. Returning [] is for explicitly rejecting the request, for example if you apply additional heuristics on top of what Cowboy is doing.

@asabil
Copy link
Contributor Author

asabil commented Mar 29, 2024

Imagine an endpoint where:

allowed_methods(Req, State) ->
    {[<<"OPTIONS">>, <<"HEAD">>, <<"GET">>, <<"POST">>, <<"DELETE">>], Req, State}.

and where only the GET method should send a content, whereas POST and DELETE should return 204, ideally we should be able to define content_types_provided as:

content_types_provided(#{method := <<"GET">>} = Req, State) ->
    {[
        {<<"application/json">>, to_json}
    ], Req, State};
content_types_provided(Req, State) ->
    {[], Req, State}.

However, this doesn't work, because the current cowboy_rest logic is tied to the fact that content_types_provided and doesn't handle the case where content_types_provided but returns the empty list.

@essen
Copy link
Member

essen commented Mar 29, 2024

You don't need to go that far, you can just do:

content_types_provided(Req, State) ->
    {[
        {<<"application/json">>, to_json}
    ], Req, State}.

@asabil
Copy link
Contributor Author

asabil commented Apr 30, 2024

I still think that implementing content_types_provided/2 and returning the [] (1) should be equivalent to not implementing it (2).

The current implementation handles (1) as reject all requests with 406, whereas (2) only returns 406 if the client actually sends an Accept header.

I suggest changing the implementation such as (1) and (2) are equivalent. Effectively handling the case where the client doesn't send any Accept header.

@essen
Copy link
Member

essen commented Apr 30, 2024

They can't be equivalent because when the callback is not implemented it is equivalent to returning [{<<"text/html">>, to_html}].

If you need to have the equivalent you need to return this and not just [].

@essen
Copy link
Member

essen commented Apr 30, 2024

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

No branches or pull requests

2 participants