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
Details
Details
- Reviewers
seirl zack - Group Reviewers
Reviewers - Maniphest Tasks
- T1968: existing graph endpoints should not return 404 upon missing arguments
- Required Signatures
L3 Software Heritage Contributor License Agreement, version 1.0
Diff Detail
Diff Detail
- Repository
- rDGRPH Compressed graph representation
- Branch
- ES_missing_args_endpoints
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 20415 Build 31683: Phabricator diff pipeline on jenkins Jenkins console · Jenkins Build 31682: arc lint + arc unit
Event Timeline
Comment Actions
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 400Link 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
Comment Actions
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 400See https://jenkins.softwareheritage.org/job/DGRPH/job/tests-on-diff/71/ for more details.
Comment Actions
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)
Comment Actions
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
Comment Actions
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 | 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" | |
Comment Actions
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
Comment Actions
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.
Comment Actions
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 400See https://jenkins.softwareheritage.org/job/DGRPH/job/tests-on-diff/73/ for more details.
Comment Actions
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.
Comment Actions
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