dwyl/mvp

Chore: API `404` Response should return `JSON` not `HTML`

Closed this issue · 7 comments

nelsonic commented

While reviewing #263 I attempted to make a cURL request (with Content-Type: application/json header)
to an API endpoint that doesn't exist: http://localhost:4000/api/items/new

e.g:

curl -X POST http://localhost:4000/api/items/new -H 'Content-Type: application/json' -d '{"text":"My Awesome item text"}'

I get an HTML response:

...
        function on (el, event, handler) {
            if (el.addEventListener) {
                el.addEventListener(event, handler)
            } else {
                el.attachEvent('on' + event, function () {
                    handler.call(el)
                })
            }
        }
    }())</script>
</body>
</html>

This is an extremely unhelpful response. The <html> response is too long so my Terminal truncates it so I can't even debug what page was returned. 🤷‍♂️

p.s. I would expect POST /api/items/new to allow me to create a new item ...
But just ran mix phx.routes and it's POST /api/items (no /new in the path) ...

Todo

  • Check all incoming /api requests and if they have Content-Type: application/json Header,
    Should return a JSON response when there's a 404 (route not found error).
nelsonic commented

Same goes for malformed API Requests:

curl -X POST http://localhost:4000/api/items/1/timers -H 'Content-Type: application/json' -d '{"start":""2022-10-28T00:00:00""}'

Note: this wasn't a deliberate error, I just copy-pasted the date from somewhere else
and ended up with ""2022-10-28T00:00:00"" instead of "2022-10-28T00:00:00" ...
but it surfaced the same issue, HTML response for a JSON request ...

Get an epic HTML page that I have to scroll through to find the error:

        <div class="code-explorer">
            <textarea class="hidden-contents" role="copy-contents"># Plug.Parsers.ParseError at POST /api/items/1/timers

Exception:

    ** (Plug.Parsers.ParseError) malformed request, a Jason.DecodeError exception was raised with message "unexpected byte at position 11: 0x32 (\"2\")"
        (plug 1.14.0) lib/plug/parsers/json.ex:90: Plug.Parsers.JSON.decode/2
        (plug 1.14.0) lib/plug/parsers.ex:346: Plug.Parsers.reduce/8
        (app 1.0.0) lib/app_web/endpoint.ex:1: AppWeb.Endpoint.plug_builder_call/2
        (app 1.0.0) lib/plug/debugger.ex:136: AppWeb.Endpoint."call (overridable 3)"/2
        (app 1.0.0) lib/app_web/endpoint.ex:1: AppWeb.Endpoint.call/2
        (phoenix 1.6.15) lib/phoenix/endpoint/cowboy2_handler.ex:54: Phoenix.Endpoint.Cowboy2Handler.init/4
        (cowboy 2.9.0) /Users/n/code/mvp/deps/cowboy/src/cowboy_handler.erl:37: :cowboy_handler.execute/2
        (cowboy 2.9.0) /Users/n/code/mvp/deps/cowboy/src/cowboy_stream_h.erl:306: :cowboy_stream_h.execute/3
        (cowboy 2.9.0) /Users/n/code/mvp/deps/cowboy/src/cowboy_stream_h.erl:295: :cowboy_stream_h.request_process/3
        (stdlib 4.1.1) proc_lib.erl:240: :proc_lib.init_p_do_apply/3
LuchoTurtle commented

You're right, currently routes that are not found return 404 but with an HTML. This is handled by default from Phoenix, I'll see how to change this so it returns a simple JSON object.

Regarding POST /api/items/new (and any /update endpoints), the reason I'm using nouns came from the research made in https://github.com/dwyl/learn-api-design#provide-sensible-resource-names and https://www.restapitutorial.com/lessons/restfulresourcenaming.html, quoting the latter.

Just to clarify, the following resource URL is redundant:

PUT http://api.example.com/customers/12345/update

With both PUT and 'update' in the request, we're offering to confuse our service consumers! Is 'update' the resource? So, we've spent some time beating the horse at this point. I'm certain you understand...

I'm sure you're aware of these, so if you want to deliberately use /new and /update (and have /timers/:id instead of /items/:item_id/timers/:id, which was created this way for completeness and compliancy with RESTful principles), I can make the change 👍

  • 👍1
nelsonic commented

This is what I was trying to get around with content so that all routes go through the same pipeline but the json ones can be handled appropriately.

As for the /new and /update the idea was just to have it explicit what the action is in the path.
But we don't need to update it now. Leave it. 👌

LuchoTurtle commented

Turning :debug_error: to true doesn't seem to be working :/

https://hexdocs.pm/phoenix/custom_error_pages.html

The json scope doesn't seem to be using the Content-type with json (it's using html instead 😞 )

All errors (NoRouteException, for example) go through lib/app_web/views/error_view. Here we define how we want things to be rendered. Here's the default.

    Phoenix.Controller.status_message_from_template(template)
  end

template is usually "404.html" or "404.json", depending on the content-type header. It seems to be returning 404.html, regardless if I ask for a json file 😔

LuchoTurtle commented

Ok, I got it fixed but it was a combination of Hoppscotch not passing the request headers I thought they were being passed and how Phoenix behaves when it receives them.

According to phoenixframework/phoenix#1879 (and after testing this myself), Phoenix only correctly routes requests according to content type if the Accept header is passed, not Content-type. Which makes sense, because the Accept header always indicates what kind of response from the server a client can accept. Content-type is about the content of the current request or response, depending on which kind of HTTP message it is applied.

So, if I run this:

curl -X GET http://localhost:4000/api/items/1/invalid_route -H 'Content-Type: application/json'

The ErrorView will run this as a .html template.
However, if I run

curl -X GET http://localhost:4000/api/items/1/invalid_route -H 'Accept: application/json'

The ErrorView will correctly detect this as a .json template.

Hoppscotch was only passing the Content-Type, instead of Accept, which is why I didn't realise what was really going on for a while.

I've created a PR for this (#270) where JSON is returned even if Content-header is the only request header defined.

Additionally, inside config.ex, I had to turn :debug_errors: true to false. This is what causes to render an HTML regardless when running locally (to help debugging, it shows the following fancy page).

image

Going to see how to fix the issue with the dates in the same PR, as well.

  • 👍1
LuchoTurtle commented

Malformed requests like the one in

curl -X POST http://localhost:4000/api/items/1/timers -H 'Content-Type: application/json' -d '{"start":""2022-10-28T00:00:00""}'

are now fixed.

After the previous commit, it was working if Accept header was passed.
Now it works when either Accept or Content-type are passed on.

I broke a few tests, I'll resolve them.

  • 🎉1
  • 🚀1
nelsonic commented

Thanks for looking at this. 👍