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

404 behavior of /live/<user_ids> #2400

Open
vicb opened this issue Feb 13, 2021 · 3 comments
Open

404 behavior of /live/<user_ids> #2400

vicb opened this issue Feb 13, 2021 · 3 comments

Comments

@vicb
Copy link
Contributor

vicb commented Feb 13, 2021

The endpoint /live/<user_ids> returns a 404 if any of the id in the list is not found.

That is /live/1,2,3,4,10 will return a 404 if 1, 2, 3, 4 exist but only 10 doesn't. The error would be {"message": "Sorry, 1 of the requested records ([1, 2, 3, 4, 10]) do not exist in our database."}

What do you think of changing this behavior ?

My preferred solution would be to never return a 404 but add information to the output JSON:

{
   "flights":[
      ...
   ],
   "pilots":[
      {
         "id":1,
         "firstName":"...",
         ...
      },
   ]
  "errors": {
    "pilotsNotFound" : [10]
  },
}

Other possible options:

  • returns a 404 if no pilot is found,
  • at least update the error message to include the id that are not found.

I could work on the implementation + test if we agree on what to do here.

Thanks.

@vicb vicb changed the title 404 behavior of 404 behavior of /live/<user_ids> Feb 13, 2021
@vicb
Copy link
Contributor Author

vicb commented Mar 11, 2021

Hey @Turbo87, do you have an opinion about this request.

The problem on the client side is that if one of the id is incorrect (typo / wrong id) then no position are returned for the correct ids and there is error allowing to figure out the wrong id.

I could work on improving the API if we can agree on the best solution.

Thanks.

@Turbo87
Copy link
Member

Turbo87 commented Mar 12, 2021

I could see this being useful, but this should be considered a breaking change for the API. We would need to adjust the SkyLines frontend to handled that case first too. Since the frontend deploys automatically after merging it would probably be best to implement that in two PRs (one for frontend, one for backend), and then merge the frontend PR first, which should support both API variants.

@vicb
Copy link
Contributor Author

vicb commented Mar 12, 2021

Thanks for your answer.

Could you point me to where this endpoint is used in the frontend ? (I am not familiar with that part of the code).

An other option is to add a query parameter or http header to enable the new behavior. That way there would be no breaking change. It might be preferable so that we do not break any external app querying the endpoint.

I you think that last proposal makes sense, please let me know if you prefer a parameter or header. I kind of like the header but no other endpoints use that so may be a parameter would be more consistent ? (i.e. ?lenient=true)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants