skylines icon indicating copy to clipboard operation
skylines copied to clipboard

404 behavior of /live/<user_ids>

Open vicb opened this issue 3 years ago • 3 comments

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 avatar Feb 13 '21 01:02 vicb

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.

vicb avatar Mar 11 '21 19:03 vicb

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.

Turbo87 avatar Mar 12 '21 08:03 Turbo87

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)

vicb avatar Mar 12 '21 15:03 vicb