Details

Reviewers

vlorentz
zack

Group Reviewers

Reviewers

Commits

rDWAPPS773ee1a21b9c: /known/ api endpoint
rDWAPPS42f4b086a393: lookup missing hashes in the storage
rDWAPPS7765946353da: group persistent identifiers by their type

Summary

New endpoint to check if a list of PIDs exist in the archive (T2176)

Get information about the Software Heritage persistent identifiers:

input: a JSON list of SWH PIDs
output: a dictionary with: pid as key and a boolean value['found'] to check if the

corrisponding pid exists or not

Diff Detail

Repository

rDWAPPS Web applications

Branch

known-api-endpoint

Lint

No Linters Available

Unit

No Unit Test Coverage

Build Status

Buildable 10288
Build 15279: Cypress tests for swh-web diffs	Jenkins
Build 15278: tox-on-jenkins	Jenkins
Build 15277: arc lint + arc unit

Event Timeline

DanSeraf created this revision.Jan 24 2020, 3:36 PM

Herald added a reviewer: Reviewers. · View Herald TranscriptJan 24 2020, 3:36 PM

DanSeraf edited the summary of this revision. (Show Details)Jan 24 2020, 3:38 PM

Build is green
See https://jenkins.softwareheritage.org/job/DWAPPS/job/tox/898/ for more details.

vlorentz requested changes to this revision.Jan 24 2020, 4:08 PM

vlorentz added a subscriber: vlorentz.

vlorentz added inline comments.

swh/web/api/views/known.py
15 ↗	(On Diff #9215)	This method should probably be in `swh/web/api/views/identifiers.py`.
39 ↗	(On Diff #9215)	You should use the parser from swh.model instead of manually splitting. (and pass the parsed object to group_swh_persistent_identifiers, so it doesn't have to parse it again)
42 ↗	(On Diff #9215)	`response` is unassigned if the method is not POST.
swh/web/common/utils.py
422–424	You should add a small wrapper around `parse_persistent_identifier` to handle the error, that can be shared with `resolve_swh_persistent_id`.
swh/web/tests/common/test_service.py
880–898	You should also add a test for when they are not (all) missing.
swh/web/tests/common/test_utils.py
117–126 ↗	(On Diff #9215)	this should be in `swh/web/tests/api/test_identifiers.py`

This revision now requires changes to proceed.Jan 24 2020, 4:08 PM

DanSeraf added inline comments.Jan 24 2020, 5:28 PM

swh/web/api/views/known.py
42 ↗	(On Diff #9215)	There is no other method available, should i assigned it anyways?

known moved to identifiers view, parse pids before grouping them
test present pids

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DWAPPS/job/tox/899/
See console output for more information: https://jenkins.softwareheritage.org/job/DWAPPS/job/tox/899/console

Harbormaster failed remote builds in B10288: Diff 9227!Jan 24 2020, 6:52 PM

/known/ endpoint tests
test_service: testing missing hashes lookup with present pids

Build is green
See https://jenkins.softwareheritage.org/job/DWAPPS/job/tox/900/ for more details.

You shouldn't remove the handling of ValidationError exceptions; because it raises an error 500 Internal Server Error, which should only be returned if there is a bug in the server code.

Instead you should raise a BadInputExc, like your code initially did.

swh/web/api/views/identifiers.py
87–91	shorter version: persistent_ids = [parse_persistent_identifier(pid) for pid in request.data]
93	You can use a dict comprehension: response = {str(pid): {'found': False} for pid in persistent_ids}
101–105	You can assign the dicts with content directly: `response` was already initialized with `found` set to `False` for all pids, so you only need to do this: for pid in persistent_ids: if pid.object_id not in missing_hashes: response[str(pid)]['found'] = True
swh/web/api/views/known.py
42 ↗	(On Diff #9215)	You should return an error `405 Method Not Allowed`.
swh/web/common/service.py
1175	We're trying to use type annotations in new functions, so it should be: def lookup_missing_hashes(grouped_pids: Dict[str, List[bytes]]) -> Set[str]:

This revision now requires changes to proceed.Jan 27 2020, 12:27 PM

returning 400 instead of 500 for invalid pid, type annotation on lookup_missing_hashes
rebase

Build is green
See https://jenkins.softwareheritage.org/job/DWAPPS/job/tox/901/ for more details.

zack requested changes to this revision.Jan 27 2020, 2:52 PM

zack added a subscriber: zack.

zack added inline comments.

swh/web/api/views/identifiers.py
79	the indentation of the dictionary keys do not look right here
80	I don't like "found" as a key here, as the name of the action is neither "search" nor "find" here. I think it should be: either "known: true/false", or "status: known/unknown" The latter would allow to convey more information in the future, e.g., an object that is present but hidden. AFAICT this would be the first time we convey a non-boolean status of objects in a public API, so we probably do not want to go there; we can reconsider for API 2.0. Hence, my proposal is "known: true/false" for now. (Naming is the most difficult problem in CS (cit.) g)
swh/web/common/service.py
1189–1199	these should be either a series of if/elif/elif/elif, or a dictionary with a single lookup
swh/web/common/utils.py
402–403	the indentation of the dictionary keys do not look right here

This revision now requires changes to proceed.Jan 27 2020, 2:52 PM

minor changes, rebase

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DWAPPS/job/tox/902/
See console output for more information: https://jenkins.softwareheritage.org/job/DWAPPS/job/tox/902/console

Harbormaster failed remote builds in B10297: Diff 9237!Jan 27 2020, 3:10 PM

fix /known/ test expected values

Build is green
See https://jenkins.softwareheritage.org/job/DWAPPS/job/tox/903/ for more details.

returning 400 instead of 500 for invalid pid, type annotation on lookup_missing_hashes

This part is ok now, but cf my earlier comment: "You should add a small wrapper around parse_persistent_identifier to handle the error, that can be shared with resolve_swh_persistent_id."

swh/web/api/views/identifiers.py
80	The latter would allow to convey more information in the future, e.g., an object that is present but hidden. It would imply breaking the API compatibility, so no.

This revision now requires changes to proceed.Jan 27 2020, 3:44 PM

zack added inline comments.Jan 27 2020, 3:59 PM

swh/web/api/views/identifiers.py
80	It would imply breaking the API compatibility, so no. This is not the place to have this conversation, but nack. Because (1) with API version 1 we make no API compatibility promises yet (that will happen starting version 2); and (2) adding fields to an existing dictionary should be possible (and documented) as a way to evolve an API that clients should expect.

In D2582#61501, @vlorentz wrote:

returning 400 instead of 500 for invalid pid, type annotation on lookup_missing_hashes

This part is ok now, but cf my earlier comment: "You should add a small wrapper around parse_persistent_identifier to handle the error, that can be shared with resolve_swh_persistent_id."

Should i handle the error in parse_persistent_identifier while creating the pid?

In D2582#61509, @DanSeraf wrote:

Should i handle the error in parse_persistent_identifier while creating the pid?

No, parse_persistent_identifier is in swh.model, which shouldn't deal with webserver stuff (and BadInputExc is an HTTP error). You should add a function in swh-web that calls parse_persistent_identifier, catches ValidationError, and re-raises a BadInputExc.

utils.py: function to get a parsed persistent identifier

Build is green
See https://jenkins.softwareheritage.org/job/DWAPPS/job/tox/904/ for more details.

Build was aborted

Link to build: https://jenkins.softwareheritage.org/job/DWAPPS/job/cypress-diff/506/
See console output for more information: https://jenkins.softwareheritage.org/job/DWAPPS/job/cypress-diff/506/console

Harbormaster failed remote builds in B10276: Diff 9215!Jan 27 2020, 5:19 PM

Build is green
See https://jenkins.softwareheritage.org/job/DWAPPS/job/cypress-diff/507/ for more details.

Build is green
See https://jenkins.softwareheritage.org/job/DWAPPS/job/cypress-diff/508/ for more details.

Harbormaster completed remote builds in B10289: Diff 9228.Jan 27 2020, 5:43 PM

Build is green
See https://jenkins.softwareheritage.org/job/DWAPPS/job/cypress-diff/509/ for more details.

Harbormaster completed remote builds in B10296: Diff 9236.Jan 27 2020, 5:54 PM

Build is green
See https://jenkins.softwareheritage.org/job/DWAPPS/job/cypress-diff/510/ for more details.

Build is green
See https://jenkins.softwareheritage.org/job/DWAPPS/job/cypress-diff/511/ for more details.

Harbormaster completed remote builds in B10300: Diff 9240.Jan 27 2020, 6:16 PM

Build is green
See https://jenkins.softwareheritage.org/job/DWAPPS/job/cypress-diff/512/ for more details.

Harbormaster completed remote builds in B10302: Diff 9242.Jan 27 2020, 6:27 PM

zack accepted this revision as: zack.Jan 27 2020, 7:47 PM

I think you missed this comment: https://forge.softwareheritage.org/D2582?id=9215#inline-17178

In D2582#61590, @vlorentz wrote:

I think you missed this comment: https://forge.softwareheritage.org/D2582?id=9215#inline-17178

The only method accepted by the endpoint is POST so it returns 405 for other methods

Then you don't need if request.method == 'POST':, right?

douardda added a subscriber: douardda.Jan 28 2020, 12:12 PM

douardda added inline comments.

swh/web/api/views/known.py
42 ↗	(On Diff #9215)	I agree this is the job of the framework (since the method has already been declared as POST only) to return the proper 405, but also that the `if request.method == 'POST':` should not be there at all.