Page MenuHomeSoftware Heritage

return a 400 error when accessing endpoints without the arguments
Needs ReviewPublic

Authored by Hakimb on Fri, Apr 2, 2:57 PM.

Details

Summary

When accessing existing endpoints and forgetting the arguments, you get a 404 error. It's more correct to have a 400 error instead because the request is badly formulated.

Related to T1968

Diff Detail

Repository
rDGRPH Graph service
Branch
ES_missing_args_endpoints
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 20472
Build 31774: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 31773: arc lint + arc unit

Event Timeline

Build was aborted

Patch application report for D5411 (id=19360)

Rebasing onto 8d30918cd7...

First, rewinding head to replay your work on top of it...
Applying: Related to T1968 : accessing an endpoint without argument returns an error 400
Changes applied before test
commit 07c1ddcd4c170dfe0e4eb63f31d6701f21d08b70
Author: Hakim Baaloudj <hbaaloud@po461-pro.paris.inria.fr>
Date:   Fri Apr 2 14:28:24 2021 +0200

    Related to T1968 : accessing an endpoint without argument returns an error 400

Link to build: https://jenkins.softwareheritage.org/job/DGRPH/job/tests-on-diff/70/
See console output for more information: https://jenkins.softwareheritage.org/job/DGRPH/job/tests-on-diff/70/console

Harbormaster returned this revision to the author for changes because remote builds failed.Fri, Apr 2, 3:17 PM
Harbormaster failed remote builds in B20415: Diff 19360!

Build is green

Patch application report for D5411 (id=19360)

Rebasing onto f055c4eaf0...

First, rewinding head to replay your work on top of it...
Applying: Related to T1968 : accessing an endpoint without argument returns an error 400
Changes applied before test
commit 7d488edc23b78afadd7fb59e477cdcb7a0beb8aa
Author: Hakim Baaloudj <hbaaloud@po461-pro.paris.inria.fr>
Date:   Fri Apr 2 14:28:24 2021 +0200

    Related to T1968 : accessing an endpoint without argument returns an error 400

See https://jenkins.softwareheritage.org/job/DGRPH/job/tests-on-diff/71/ for more details.

Hakimb requested review of this revision.Fri, Apr 2, 5:26 PM

Hi, thanks for the diff.

Could you update your commit message and diff description to follow https://docs.softwareheritage.org/devel/contributing/git-style-guide.html

I'm not very familiar with swh-graph, but it may be more readable to group similar endpoints, like this:

app.get("/leaves/", ctx -> {
    ctx.status(400);
    ctx.json("src (SWHID) argument missing");
});
app.before("/leaves/*", ctx -> {
    checkQueryStrings(ctx, "direction|edges");
});

app.get("/neighbors/", ctx -> {
    ctx.status(400);
    ctx.json("src (SWHID) argument missing");
});
app.before("/neighbors/*", ctx -> {
    checkQueryStrings(ctx, "direction|edges");
});

etc. what do you think?

(I'll leave the rest of the review to seirl or zack)

Hakimb retitled this revision from Related to T1968 : accessing an endpoint without argument returns an error 400 to return a 400 error when accessing endpoints without the arguments.Tue, Apr 6, 11:31 AM
Hakimb edited the summary of this revision. (Show Details)

@vlorentz

I thought we should group the code by endpoint type, all the "get" endpoints without arguments between them (we can consider them as default cases), as well as the "before" endpoints and the "get" endpoints with detailed arguments that were already coded. I may be wrong

zack requested changes to this revision.Tue, Apr 6, 11:59 AM
zack added a subscriber: zack.

also, can you add tests verifying that calling the API without an argument does in fact return 400 error?

(maybe not for all cases if it's too repetitive, but at least one or two to ensure the code does what it is supposed to do would be nice)

java/src/main/java/org/softwareheritage/graph/server/App.java
82–110 ↗(On Diff #19360)

thanks, this looks good! but I suggest leaving out the parentheses. While the intent of helping users is great, this is duplication of information w.r.t. the doc, which will mean we *will* forget to update one of the other when the types of the arguments change in the future :)

just say something "src argument missing"

This revision now requires changes to proceed.Tue, Apr 6, 11:59 AM

There's a problem with this diff, it's on an old java-only backend that isn't the one we use when we run swh graph rpc-serve. The one that is currently used is in python, at swh/graph/server/app.py

complete the task on the right server and add tests

my first modifications were on an obsolete version of the server, written in java, now they are on the recent server in python.

Build is green

Patch application report for D5411 (id=19415)

Could not rebase; Attempt merge onto 15c2da0f08...

Updating 15c2da0..5eda10d
Fast-forward
 .../org/softwareheritage/graph/server/App.java     | 32 ++++++++++++++++++++++
 swh/graph/client.py                                | 29 ++++++++++++++++++++
 swh/graph/server/app.py                            | 14 ++++++++++
 swh/graph/tests/test_api_client.py                 | 15 ++++++++++
 4 files changed, 90 insertions(+)
Changes applied before test
commit 5eda10d8b0d5ace4fa164d3d20be3df779259938
Author: Hakim Baaloudj <hbaaloud@po461-pro.paris.inria.fr>
Date:   Tue Apr 6 21:18:56 2021 +0200

    complete the task on the right server and add tests

commit f7dbddf07878c87c46cb67b56cd8e495fe2d5ced
Author: Hakim Baaloudj <hbaaloud@po461-pro.paris.inria.fr>
Date:   Fri Apr 2 14:28:24 2021 +0200

    Related to T1968 : accessing an endpoint without argument returns an error 400

See https://jenkins.softwareheritage.org/job/DGRPH/job/tests-on-diff/73/ for more details.

I thought we should group the code by endpoint type, all the "get" endpoints without arguments between them (we can consider them as default cases), as well as the "before" endpoints and the "get" endpoints with detailed arguments that were already coded. I may be wrong

When someone wants to create a new endpoint, they look at how an existing one (eg. leaves) are implemented and copy-paste the boilerplate. If the implementation of that is scattered in multiple places (that don't depend on each other), they may miss one of them.

Thanks, the new implementation is better. I don't think we should have methods on the client just to generate errors. You can make the test access the server directly