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

3.2.0-rc1 doesn't handle API not found errors correctly #324

Closed
matthewdarwin opened this issue Oct 14, 2022 · 1 comment · Fixed by #337
Closed

3.2.0-rc1 doesn't handle API not found errors correctly #324

matthewdarwin opened this issue Oct 14, 2022 · 1 comment · Fixed by #337
Assignees
Labels
actionable 👍 lgtm OCI Work exclusive to OCI team

Comments

@matthewdarwin
Copy link

matthewdarwin commented Oct 14, 2022

In all previous versions of nodeos, any 404 not-found errors are returned as JSON documents. In 3.2.0-rc1 a plain text error message is returned, but the content type is still application/json.

Either the content-type needs to be changed, or the content. I think the content should be changed to match previous version to not break backwards compatibility. Tools test URLs to understand if APIs are available or not. (eg account API).

3.2.0-rc1:

$ curl -v http://localhost:8888/
*   Trying 127.0.0.1:8888...
* Connected to localhost (127.0.0.1) port 8888 (#0)
> GET / HTTP/1.1
> Host: localhost:8888
> User-Agent: curl/7.74.0
> Accept: */*
>
* Mark bundle as not supporting multiuse
< HTTP/1.1 404 Not Found
< Content-Type: application/json
< Server: Boost.Beast/248
< Content-Length: 31
<
* Connection #0 to host localhost left intact
The resource '/' was not found.#                                                                                                                                                                               

Older:

$ curl -v http://localhost:8888/
*   Trying 127.0.0.1:8888...
* Connected to localhost (127.0.0.1) port 8888 (#0)
> GET / HTTP/1.1
> Host: localhost:8888
> User-Agent: curl/7.74.0
> Accept: */*
>
* Mark bundle as not supporting multiuse
< HTTP/1.1 404 Not Found
< Connection: close
< Content-Length: 210
< Content-type: application/json
< Server: WebSocket++/0.7.0
<
* Closing connection 0
{"code":404,"message":"Not Found","error":{"code":0,"name":"exception","what":"unspecified","details":[{"message":"Unknown Endpoint","file":"http_plugin.cpp","line_number":597,"method":"handle_http_request"}]}}#
@enf-ci-bot enf-ci-bot moved this to Todo in Team Backlog Oct 14, 2022
@heifner heifner added actionable and removed triage labels Oct 14, 2022
@stephenpdeos stephenpdeos added this to the Leap 3.2.0-rc2 milestone Oct 14, 2022
@heifner heifner self-assigned this Oct 14, 2022
@heifner heifner added the OCI Work exclusive to OCI team label Oct 14, 2022
heifner added a commit that referenced this issue Oct 14, 2022
@stephenpdeos stephenpdeos moved this from Todo to Awaiting Review in Team Backlog Oct 14, 2022
@kj4ezj
Copy link
Contributor

kj4ezj commented Oct 14, 2022

From IM.

I'll add, when endpoints that normally return JSON return non-JSON (e.g. in an error case), it can make the client code much more complicated, or errors incredibly hard to diagnose.

For example, my favorite thing at my last job is when people would come to me for help with an error they didn't understand and I would see "< is not a valid character" or something like that. That pretty much meant an endpoint expected to return JSON returned an HTML page with a 400 or 500 error message. I only recognized it so quickly because I spent so many times taking hours to figure it out. The systems usually don't print or log the HTML response, so you still don't know what went wrong. Sometimes they do.

But it is very common for engineers to spend 1-2 hours trying to figure this out.

If an endpoint is expected to return JSON, it saves a lot of time and money if it just always returns JSON, even in error conditions. Tools like curl that work either way will display the error JSON.

heifner added a commit that referenced this issue Oct 17, 2022
[3.2] Return application/json for all http responses including errors
heifner added a commit that referenced this issue Oct 17, 2022
[3.2 -> main] Return application/json for all http responses including errors
Repository owner moved this from Awaiting Review to Done in Team Backlog Oct 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
actionable 👍 lgtm OCI Work exclusive to OCI team
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants