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

http: improve 404 Not Found response message #11818

Merged
merged 1 commit into from
Dec 13, 2021

Conversation

jkirschner-hashicorp
Copy link
Contributor

When a URL path is not found, return a non-empty message with the 404 status code to help the user understand what went wrong. If the URL path was not prefixed with '/v1/', suggest that may be the cause of the problem (which is a common mistake).

Examples

Path not prefixed with '/v1/`

curl --request GET --verbose localhost:8500/agent/services
Note: Unnecessary use of -X or --request, GET is already inferred.
*   Trying 127.0.0.1:8500...
* TCP_NODELAY set
* Connected to localhost (127.0.0.1) port 8500 (#0)
> GET /agent/services HTTP/1.1
> Host: localhost:8500
> User-Agent: curl/7.68.0
> Accept: */*
>
* Mark bundle as not supporting multiuse
< HTTP/1.1 404 Not Found
< Date: Mon, 13 Dec 2021 16:32:45 GMT
< Content-Length: 87
< Content-Type: text/plain; charset=utf-8
<
* Connection #0 to host localhost left intact
Invalid URL path: if attempting to use the HTTP API, ensure the path starts with '/v1/'

Path prefixed with '/v1/`

# endpoint is misspelled
$ curl --request GET localhost:8500/v1/agent/srvices
Invalid URL path: not a recognized HTTP API endpoint

@jkirschner-hashicorp jkirschner-hashicorp added theme/api Relating to the HTTP API interface theme/operator-usability Replaces UX. Anything related to making things easier for the practitioner backport/1.11 labels Dec 13, 2021
@jkirschner-hashicorp jkirschner-hashicorp requested a review from a team December 13, 2021 16:36
@github-actions github-actions bot removed the theme/api Relating to the HTTP API interface label Dec 13, 2021
@jkirschner-hashicorp jkirschner-hashicorp changed the title api: improve 404 Not Found response message http: improve 404 Not Found response message Dec 13, 2021
@vercel vercel bot temporarily deployed to Preview – consul December 13, 2021 16:43 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging December 13, 2021 16:43 Inactive
@jkirschner-hashicorp jkirschner-hashicorp added the theme/api Relating to the HTTP API interface label Dec 13, 2021
Copy link
Contributor

@dnephin dnephin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I think this should help make it easier to notice the problem.

When we start introducing different versions I think we'll want to update our API docs to include the /v1 prefix in the docs, which should also help.

agent/http.go Outdated
Comment on lines 592 to 594
fmt.Fprint(resp, "Invalid URL path: not a recognized HTTP API endpoint")
} else {
fmt.Fprint(resp, "Invalid URL path: if attempting to use the HTTP API, ensure the path starts with '/v1/'")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: maybe a trailing newline on these?

Suggested change
fmt.Fprint(resp, "Invalid URL path: not a recognized HTTP API endpoint")
} else {
fmt.Fprint(resp, "Invalid URL path: if attempting to use the HTTP API, ensure the path starts with '/v1/'")
fmt.Fprintln(resp, "Invalid URL path: not a recognized HTTP API endpoint")
} else {
fmt.Fprintln(resp, "Invalid URL path: if attempting to use the HTTP API, ensure the path starts with '/v1/'")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, I'll make this change then merge

@jkirschner-hashicorp
Copy link
Contributor Author

we'll want to update our API docs to include the /v1 prefix in the docs, which should also help.

@dnephin: I made this mistake relatively often when starting out for that reason... the docs omit the /v1 prefix, so unless you consistently remember it's needed, you'll run into this and be confused. (And I still see it happen with people who aren't new.)

When a URL path is not found, return a non-empty message with the 404 status
code to help the user understand what went wrong. If the URL path was not
prefixed with '/v1/', suggest that may be the cause of the problem (which is a
common mistake).
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging December 13, 2021 19:04 Inactive
@vercel vercel bot temporarily deployed to Preview – consul December 13, 2021 19:04 Inactive
@jkirschner-hashicorp jkirschner-hashicorp merged commit f81dd81 into main Dec 13, 2021
@jkirschner-hashicorp jkirschner-hashicorp deleted the improve-url-not-found-response branch December 13, 2021 21:08
@hc-github-team-consul-core
Copy link
Contributor

🍒 If backport labels were added before merging, cherry-picking will start automatically.

To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/527069.

@hc-github-team-consul-core
Copy link
Contributor

🍒✅ Cherry pick of commit f81dd81 onto release/1.11.x succeeded!

hc-github-team-consul-core pushed a commit that referenced this pull request Dec 13, 2021
http: improve 404 Not Found response message
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/api Relating to the HTTP API interface theme/operator-usability Replaces UX. Anything related to making things easier for the practitioner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants