Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

REST API improvements #657

Closed
4 tasks
kishansagathiya opened this issue Feb 5, 2019 · 4 comments · Fixed by #879
Closed
4 tasks

REST API improvements #657

kishansagathiya opened this issue Feb 5, 2019 · 4 comments · Fixed by #879
Labels
exp/intermediate Prior experience is likely helpful help wanted Seeking public contribution on this issue kind/bug A bug in existing code (including security flaws) P1 High: Likely tackled by core team if no one steps up status/ready Ready to be worked

Comments

@kishansagathiya
Copy link
Contributor

Inspired from discussions that happened in #634

  • Support trailing slashes in URLs. That is, it should not redirect, but behave just the same way as URLs without a trailing slash(same response, same http code, no multiple requests or redirect)
  • Errors returned by our APIs are in JSON format. Requests that doesn't reach our handlers and are responded by Gorilla are not in JSON format. Make sure that those responses are also in JSON format. So, when we fail to match a route instead of text message error not found 404, it should return a JSON response.
  • Effects on tests. We have makePostWithContentType and other similar test helper functions, which should work for given content type judging them by their names, but there logic assumes that we are always using Content-Type as application/json. processResp and checkHeaders assumes that response should have. I don't see a point in having WithContentType functions when we know all responses should return json responses.
  • Add tests for various APIs, make sure that trailing slash support is tested and returns JSON response. Test that APIs are not returning text error not found 404, but a JSON response.

Should be done after #634 is merged. #634 won't be perfect, but I think changes in this issue should make it so.

Thoughts?

@kishansagathiya kishansagathiya added exp/novice Someone with a little familiarity can pick up status/blocked Unable to be worked further until needs are met P2 Medium: Good to have, but can wait until someone steps up labels Feb 5, 2019
@hsanjuan
Copy link
Collaborator

hsanjuan commented Feb 5, 2019

Inspired from discussions that happened in #634

* [ ]  Support trailing slashes in URLs. That is, it should not redirect, but behave just the same way as URLs without a trailing slash(same response, same http code, no multiple requests or redirect)

I wouldn't do this. Trailing slashes (and double //) are the wrong path. We can assume the user means to use a path without trailing slashes and //, but this is just an assumption (probably wrong in most cases as this is an API and such urls are just the result of bugs in clients). When gorilla enables the possibility of doing 308 redirects we can think of it as it would come for free, but right now things a better without any extra handling.

* [ ]  Errors returned by our APIs are in JSON format. Requests that doesn't reach our handlers and are responded by Gorilla are not in JSON format. Make sure that those responses are also in JSON format. So, when we fail to match a route instead of text message `error not found 404`, it should return a JSON response.

Yes, we can do this.

* [ ]  Effects on tests. We have `makePostWithContentType` and other similar test helper functions, which should work for given content type judging them by their names, but there logic assumes that we are always using Content-Type as `application/json`. `processResp` and `checkHeaders` assumes that response should have. I don't see a point in having `WithContentType` functions when we know all responses should return json responses.

ContentType different than json is used when using /add to make a multipart post.

* [ ]  Add tests for various APIs, make sure that trailing slash support is tested and returns JSON response. Test that APIs are not returning text `error not found 404`, but a JSON response.

Only a single test to detect that a catchall route returns 404 with the json message.

@kishansagathiya
Copy link
Contributor Author

Currently makePost doesn't check http status code, in case the request succeeds. We should modify it to do so.

@hsanjuan
Copy link
Collaborator

hsanjuan commented Feb 5, 2019

It should probably return it, so it can be checked per test, when needed.

@kishansagathiya kishansagathiya removed the status/blocked Unable to be worked further until needs are met label Apr 9, 2019
@hsanjuan hsanjuan changed the title Handle trailing slash and make rest API uniform and more appropriate REST API improvements Jul 23, 2019
@hsanjuan hsanjuan added kind/bug A bug in existing code (including security flaws) exp/intermediate Prior experience is likely helpful P1 High: Likely tackled by core team if no one steps up status/ready Ready to be worked help wanted Seeking public contribution on this issue and removed exp/novice Someone with a little familiarity can pick up P2 Medium: Good to have, but can wait until someone steps up labels Jul 23, 2019
@hsanjuan
Copy link
Collaborator

The main actionable here is to make sure all errors are JSON (and test).

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
exp/intermediate Prior experience is likely helpful help wanted Seeking public contribution on this issue kind/bug A bug in existing code (including security flaws) P1 High: Likely tackled by core team if no one steps up status/ready Ready to be worked
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants