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

render_errors doesn't work with 2 options #1879

Closed
noma4i opened this issue Aug 23, 2016 · 19 comments
Closed

render_errors doesn't work with 2 options #1879

noma4i opened this issue Aug 23, 2016 · 19 comments

Comments

@noma4i
Copy link
Contributor

noma4i commented Aug 23, 2016

Environment

  • Elixir version 1.3.2
  • Phoenix version (mix deps): 1.2
  • Operating system: macos

Expected behavior

render_errors: [accepts: ~w(html json)]

Errors both works for html or json specified ways.

Actual behavior

html - ok
json - Phoenix.Template.HTML.encode_to_iodata!(%{errors: %{message: "Not Found"}})

BUT! If you define only HTML or JSON at a time in accepts it will work correct.

@chrismccord
Copy link
Member

Can you provide more information on what isn't working for you with json? We need to see the full stack trace and details about the request as I'm not sure what issues you are hitting from the description. Thanks!

@noma4i
Copy link
Contributor Author

noma4i commented Aug 23, 2016

It's easy.

  • Define in config render_errors: [accepts: ~w(html)]
    Update ErrorView to render 404.html. - check it, looks nice.
  • Define in config render_errors: [accepts: ~w(json)]
    Update ErrorView to render 404.json as %{error: "message"}. - check it, looks even better.
  • Define in config render_errors: [accepts: ~w(html json)]
    Update ErrorView to support 404.html - works as always.
    Do the same for 404.json. = error.

Stacktrace:
[error] #PID<0.533.0> running MD.Endpoint terminated Server: localhost:4001 (http) Request: GET /api/users123 ** (exit) an exception was raised: ** (FunctionClauseError) no function clause matching in Phoenix.Template.HTML.encode_to_iodata!/1 (phoenix) lib/phoenix/template/html.ex:12: Phoenix.Template.HTML.encode_to_iodata!(%{errors: %{message: "Not Found"}}) (phoenix) lib/phoenix/controller.ex:641: Phoenix.Controller.do_render/4 (plug) lib/plug/adapters/cowboy/handler.ex:15: Plug.Adapters.Cowboy.Handler.upgrade/4 (cowboy) src/cowboy_protocol.erl:442: :cowboy_protocol.execute/4

@chrismccord
Copy link
Member

It appears you are not sending the proper accept headers. When you specify both [accepts: ~w(html json)], the way we choose which one to render is based on the accept header sent by the client. Can you ensure you are sending the application/json accept header, or using the _format=json query param? A quick check would be to hit /api/users123?_format=json

@noma4i
Copy link
Contributor Author

noma4i commented Aug 23, 2016

@chrismccord Fair enough. Explicit header will force phoenix to work render json. In this case Im lost with ideas here: https://api.github.com/users123 - you can open in browser w/o any hassle with headers.

Looks like I will need to add more logic around ErrorView/gut conn to determine content-type.

@noma4i
Copy link
Contributor Author

noma4i commented Aug 23, 2016

Anyway it's not an error in phoenix.
P.s: StackOverflow was checked twice before posting here..

@noma4i noma4i closed this as completed Aug 23, 2016
@noma4i
Copy link
Contributor Author

noma4i commented Aug 23, 2016

I ended up with decision that error while trying to render json is a bug. Phoenix can guess that I need to render json as an output w/o any format only when render_errors: [accepts: ~w(json)]

While html is rendered all the time not matter are there format=json or any headers.

@noma4i noma4i reopened this Aug 23, 2016
@josevalim
Copy link
Member

Can you please post your error view and how are you are sending requests to render such view? I see no reason why Phoenix would try to encode a map as HTML.

@josevalim
Copy link
Member

Also, you can have the same behaviour as GitHub if you put JSON first in the formats list. If you have more than one format, then you must do content negotiation.

@noma4i
Copy link
Contributor Author

noma4i commented Aug 27, 2016

I was able to resolve that issue. Accept: application/json make sense. Edge cases solved by plug middleware.

Thnx all!

@noma4i noma4i closed this as completed Aug 27, 2016
@noma4i
Copy link
Contributor Author

noma4i commented Aug 28, 2016

As phoenix not able to reject connection via api pipeline went to unknown path people are doing things like that http://stackoverflow.com/questions/38185875/how-to-enforce-json-encoding-for-phoenix-request

@noma4i
Copy link
Contributor Author

noma4i commented Aug 28, 2016

Im reopening this again. No matter that @josevalim said that we should never enforce any content type at this point Phoenix with render_errors json will render json errors with render_errors: [accepts: ~w(json)] while defined ~w(html json) will always try to fallback in html. Override in ErrorView with json output leads to Phoenix.Template.HTML.encode_to_iodata error. When client is pushed to use pipeline :api with explicit declaration of plug :accepts, ["json"] it will be never reached while Elixir.Phoenix.Router.NoRouteError is called.

Client is trying to open not existing page via pipeline :api in browser, Expected behaviour:

  • Client face missing json header error(most obvious) or 404.json

Got:

  • HTML 404.

As I can see that is possible since pipelines from router.ex never kick in when routing errors been faced.

Same problem will appear on using custom api like jsonapi where Content-type: application/vnd.api+json is defined. In this case any 404/500 will go to Phoenix.Template.HTML

@noma4i noma4i reopened this Aug 28, 2016
@josevalim
Copy link
Member

@noma4i I have just started a new application and I did the following to get always JSON responses:

  1. In config/config.exs, set renders errors to only accepts: ["json"]
  2. In your router, remove the plug :accepts, ["json"], because you don't want to do content negotiation, and use plug :put_format, :json to enforce the json format instead.

With those changes, I get json no matter what.

@noma4i
Copy link
Contributor Author

noma4i commented Aug 29, 2016

@josevalim

get always JSON responses

It was never problem here while you set whole app for json only. Issue is about the way 404/500 errors are treated in html/json pipelines.

because you don't want to do content negotiation

Ok. I see the culprit. You talk about plug while https://github.com/phoenixframework/phoenix/blob/master/lib/phoenix/router.ex#L185

  Note that router pipelines are only invoked after a route is found.
  No plug is invoked in case no matches were found.

Is it expected behaviour to skip content negotiation on route errors?

@josevalim
Copy link
Member

Yes, if no route matches we don't know which pipeline to invoke. Then you
need to obey what the browser says or always default one particular format.

On Sunday, August 28, 2016, Alexander Tsirel [email protected]
wrote:

@josevalim https://github.com/josevalim

get always JSON responses

It was never problem here while you set whole app for json only. Issue is
about the way 404/500 errors are treated in html/json pipelines.

because you don't want to do content negotiation

Ok. I see the culprit. You talk about plug while https://github.com/
phoenixframework/phoenix/blob/master/lib/phoenix/router.ex#L185

Note that router pipelines are only invoked after a route is found.
No plug is invoked in case no matches were found.

Is it expected behaviour to skip content negotiation on route errors?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1879 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAAlbmsMGSv4WP5PlR41H90lC-_YKjwGks5qkjP8gaJpZM4Jq1Ce
.

José Valimwww.plataformatec.com.br
http://www.plataformatec.com.br/Founder and Director of R&D

@noma4i
Copy link
Contributor Author

noma4i commented Aug 29, 2016

@josevalim Iv extended tests for html/json ErrorViews + showing consequences of skipped content negotiation #1890

@noma4i
Copy link
Contributor Author

noma4i commented Aug 29, 2016

For now Phoenix has 2 routing pathways. 1 is matching routing 404 errors and the second - content of router.ex. In my case Iv created workaround to make content-type & format negotiation before router matching kicks in. That gives peace in mind that malformed or missing headers/format will never end up with Phoenix.Template.UndefinedError or not appropriate content been served. Like api scope always serves json or headers/format mismatch error.

@yordis
Copy link

yordis commented Mar 15, 2018

In your router, remove the plug :accepts, ["json"], because you don't want to do content negotiation, and use plug :put_format, :json to enforce the json format instead.

@josevalim the :put_format comes from Phoenix or did you expect us to write our function that does that?

I got an error so I am not sure about it.

@noma4i
Copy link
Contributor Author

noma4i commented Mar 15, 2018

@yordis :put_format is coming from Phoenix Controller to modify conn.
https://hexdocs.pm/phoenix/Phoenix.Controller.html#put_format/2

@yordis
Copy link

yordis commented Mar 15, 2018

@noma4i aah, that is at the controller level, I thought this came from the Router one,

thanks a lot for the explanation.

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

4 participants