Page MenuHomeSoftware Heritage

existing graph endpoints should not return 404 upon missing arguments
Closed, ResolvedPublic

Description

e.g., GET /neighbors/ → 404

as the endpoint does exist, but an argument is missing (the node PID in this case), it should return something like 400 (Bad Request) instead

Event Timeline

zack triaged this task as Low priority.Aug 25 2019, 2:50 PM
zack created this task.
zack renamed this task from existing graph endpoints should not return 404 upon for missing arguments to existing graph endpoints should not return 404 upon missing arguments.Nov 13 2019, 3:32 PM
This comment was removed by Kaustuv942.

Hello @zack I want to complete this task.

zack added a subscriber: Kaustuv942.

Dear @Kaustuv942, sure, patches welcome. We do not use task claiming for non regular contributors though, just submit a patch when you have one.

@zack We talked about this on IRC with @vlorentz, I think this issue is invalid. We chose to have the source and destination nodes as part of the URI in the API. Semantically, it makes sense that accessing the path without these path fragments would return a 404: it's not a missing argument but an invalid path. If we had a ?src= and a &dst= arguments instead, then having a 400 error would make sense, but in our case the semantics are really weird.

If you look at the proposed fix in D5411, the fact that we have to double the number of endpoints just to change an error code shows that we're doing something really wrong. I think we should either keep the 404 error, or decide to use parameters instead of URL fragments for the SWHIDs. Does that sound reasonable to you?

@seirl, @vlorentz: I see your point, and I agree. We should never have used /nested/paths for this API.
Maybe we should just reconsider this and, one @Hakimb is ready with a new traversal language proposal, we can map it to a better REST API that uses query parameters, and deal properly with 4xx return codes.

Right, I suppose we can close the task then?

zack claimed this task.

Sure! My apologies @Hakimb, but it's thank to your work that we have realized what was the right fate for this task.

zack changed the task status from Invalid to Resolved.Wed, Dec 1, 5:00 PM
zack moved this task from Backlog to Deployed on the Graph service board.