Page MenuHomeSoftware Heritage
Differential D1623

Add origin_metadata_get API endpoint
ClosedPublic
Actions

Authored by twitu on Jun 21 2019, 8:45 AM.

Active Operations

Details

Reviewers
vlorentz
zack
Group Reviewers
Reviewers
Commits
rDWAPPS9a8e1f98c9fd: Add origin_metadata_get API endpoint
Summary

I am adding an API end point to get origin intrinsic metadata to from origin_type and origin_url from indexer_storage. This involves adding an endpoint in origin.py and function that queries the db in service.py.

I use storage.origin_get to get basic origin information from the type and url. I use this to pass the id of the origin, to origin_metadata_get in indexer storage which returns to intrinsic metadata of the origin.

referencing T1613

Diff Detail

Repository
rDWAPPS Web applications
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

twitu created this revision.Jun 21 2019, 8:45 AM
Herald added a reviewer: Reviewers. · View Herald TranscriptJun 21 2019, 8:45 AM
twitu added a comment.Jun 21 2019, 8:48 AM

in reference to T1613
I am returning entry for only one origin id. Is this expected functionality?

swh-public-ci added a comment.Jun 21 2019, 8:50 AM

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

Harbormaster completed remote builds in B6391: Diff 5406.Jun 21 2019, 8:50 AM
vlorentz added a subscriber: vlorentz.Jun 21 2019, 10:09 AM

We require that all endpoints have unit tests. Could you add some?
You can see in swh/web/tests/api/views/test_origin.py how we do it.

swh/web/api/views/origin.py
437–438

Could you use this as route instead?

@api_route(r'/origin/(?P<origin_type>[a-z]+)/url/(?P<origin_url>.+)//metadata',
           'api-origin-metdata-get')

Both ways are consistent with existing APIs, but we're trying to migrate away from using origin_id. You can look at how the api_origin() endpoint does it.

470–474

'results' is for the results of a search, you can return directly the return value of api_lookup(...).

vlorentz requested changes to this revision.Jun 21 2019, 10:09 AM
This revision now requires changes to proceed.Jun 21 2019, 10:09 AM
twitu updated this revision to Diff 5435.Jun 21 2019, 9:13 PM
  • Fix intrinsic metadata enpoint and test case
twitu updated this revision to Diff 5436.Jun 21 2019, 9:15 PM
  • Remove unnecessary tox.ini dependency
twitu added inline comments.Jun 21 2019, 9:18 PM
swh/web/common/service.py
302

I am still making this call to convert url and type information to id of origin, because intrinsic metadata is only searchable by id

swh/web/tests/api/views/test_origin.py
332

Test case is failing here, citing an Attribute error here. I need multiple context managers because to test the api I need to mock both storage and index-storage. I am not sure why I am unable to mock storage, any suggestions?

swh-public-ci added a comment.Jun 21 2019, 9:19 PM

Build has FAILED

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

Harbormaster failed remote builds in B6422: Diff 5435!Jun 21 2019, 9:19 PM
swh-public-ci added a comment.Jun 21 2019, 9:25 PM

Build has FAILED

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

Harbormaster failed remote builds in B6423: Diff 5436!Jun 21 2019, 9:25 PM
twitu marked 2 inline comments as done.Jun 22 2019, 4:18 AM
twitu marked 2 inline comments as not done.Jun 22 2019, 7:25 AM
twitu updated this revision to Diff 5447.Jun 23 2019, 2:48 PM
  • Change key reference from origin_id to id
swh-public-ci added a comment.Jun 23 2019, 2:55 PM

Build has FAILED

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

Harbormaster failed remote builds in B6435: Diff 5447!Jun 23 2019, 2:55 PM
twitu mentioned this in P448 Pytest error stack trace.Jun 23 2019, 2:57 PM
zack resigned from this revision.Jun 23 2019, 3:32 PM
twitu added inline comments.Jun 24 2019, 5:55 PM
swh/web/common/service.py
308

The test is successful when I change this line to origin_ids = [1]. Otherwise I receive a KeyError for id. This indicates that storage.origin_get is not being mocked and is not returning the correct values. The expected value is a dictionary containing key id. I am unable to figure out why this is not behaving as expected.

twitu edited the summary of this revision. (Show Details)Jun 24 2019, 6:13 PM
twitu edited the summary of this revision. (Show Details)
twitu added inline comments.Jun 24 2019, 7:26 PM
swh/web/common/service.py
308

I added print(origin_info) here to record to log the contents of origin_info.

.tox/py3/lib/python3.5/site-packages/swh/web/tests/api/views/test_origin.py {'url': 'https://github.com/wcoder/highlightjs-line-numbers.js', 'type': 'git'}
{'url': 'repo_with_submodules', 'type': 'git'}
{'url': 'https://github.com/memononen/libtess2', 'type': 'git'}
{'url': 'repo_with_submodules', 'type': 'git'}
{'url': 'repo_with_submodules', 'type': 'git'}
{'url': 'https://github.com/memononen/libtess2', 'type': 'git'}
{'url': 'https://github.com/memononen/libtess2', 'type': 'git'}
{'url': 'https://github.com/wcoder/highlightjs-line-numbers.js', 'type': 'git'}
{'url': 'https://github.com/wcoder/highlightjs-line-numbers.js', 'type': 'git'}
{'url': 'https://github.com/wcoder/highlightjs-line-numbers.js', 'type': 'git'}
{'url': 'repo_with_submodules', 'type': 'git'}
{'url': 'https://github.com/memononen/libtess2', 'type': 'git'}
{'url': 'https://github.com/memononen/libtess2', 'type': 'git'}
{'url': 'https://github.com/wcoder/highlightjs-line-numbers.js', 'type': 'git'}
{'url': 'https://github.com/memononen/libtess2', 'type': 'git'}
{'url': 'repo_with_submodules', 'type': 'git'}
{'url': 'repo_with_submodules', 'type': 'git'}
{'url': 'https://github.com/wcoder/highlightjs-line-numbers.js', 'type': 'git'}
{'url': 'https://github.com/wcoder/highlightjs-line-numbers.js', 'type': 'git'}
{'url': 'https://github.com/memononen/libtess2', 'type': 'git'}
{'url': 'repo_with_submodules', 'type': 'git'}
{'url': 'https://github.com/wcoder/highlightjs-line-numbers.js', 'type': 'git'}
{'url': 'repo_with_submodules', 'type': 'git'}
{'url': 'https://github.com/wcoder/highlightjs-line-numbers.js', 'type': 'git'}
{'url': 'repo_with_submodules', 'type': 'git'}
{'url': 'repo_with_submodules', 'type': 'git'}
{'url': 'https://github.com/wcoder/highlightjs-line-numbers.js', 'type': 'git'}
{'url': 'https://github.com/memononen/libtess2', 'type': 'git'}
{'url': 'https://github.com/memononen/libtess2', 'type': 'git'}
{'url': 'https://github.com/memononen/libtess2', 'type': 'git'}
{'url': 'repo_with_submodules', 'type': 'git'}
{'url': 'https://github.com/wcoder/highlightjs-line-numbers.js', 'type': 'git'}
{'url': 'https://github.com/memononen/libtess2', 'type': 'git'}
{'url': 'https://github.com/wcoder/highlightjs-line-numbers.js', 'type': 'git'}
{'url': 'repo_with_submodules', 'type': 'git'}
{'url': 'https://github.com/memononen/libtess2', 'type': 'git'}
{'url': 'https://github.com/memononen/libtess2', 'type': 'git'}
{'url': 'https://github.com/wcoder/highlightjs-line-numbers.js', 'type': 'git'}
{'url': 'repo_with_submodules', 'type': 'git'}
{'url': 'https://github.com/memononen/libtess2', 'type': 'git'}
{'url': 'repo_with_submodules', 'type': 'git'}
{'url': 'https://github.com/wcoder/highlightjs-line-numbers.js', 'type': 'git'}
{'url': 'repo_with_submodules', 'type': 'git'}
{'url': 'https://github.com/wcoder/highlightjs-line-numbers.js', 'type': 'git'}
{'url': 'https://github.com/memononen/libtess2', 'type': 'git'}
{'url': 'https://github.com/memononen/libtess2', 'type': 'git'}
{'url': 'https://github.com/memononen/libtess2', 'type': 'git'}
<-- removed repeated value for clarity -->

What I got back does not contain the any key for id, though it should be part of the returned values according to API docs.

twitu updated this revision to Diff 5470.EditedJun 25 2019, 5:09 AM

Fix test case. Silly mistakes are costly.

swh-public-ci added a comment.Jun 25 2019, 5:16 AM

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

Harbormaster completed remote builds in B6460: Diff 5470.Jun 25 2019, 5:16 AM
vlorentz added a comment.Jun 25 2019, 10:28 AM

Cool, thanks! Two last nitpicks:

  1. you can drop the mocking of swh.web.common.service.storage and add the origin in it directly
  2. Could you check that origin_intrinsic_metadata_get is called with the right arguments? You can use this function: https://docs.python.org/3/library/unittest.mock.html#unittest.mock.Mock.assert_called_once_with
vlorentz requested changes to this revision.Jun 25 2019, 10:29 AM
This revision now requires changes to proceed.Jun 25 2019, 10:29 AM
twitu updated this revision to Diff 5491.Jun 25 2019, 5:18 PM

Add assert_called_once_with to test case

vlorentz accepted this revision.Jun 25 2019, 5:21 PM
This revision is now accepted and ready to land.Jun 25 2019, 5:21 PM
vlorentz requested changes to this revision.Jun 25 2019, 5:22 PM

Sorry, I just noticed, but this needs to be updated:

.. http:get:: /api/1/origin/(origin_type)/url/(origin_url)/intrinsic-metadata
This revision now requires changes to proceed.Jun 25 2019, 5:22 PM
vlorentz accepted this revision.Jun 25 2019, 5:22 PM

Nevermind, I misread it. It's fine

This revision is now accepted and ready to land.Jun 25 2019, 5:22 PM
vlorentz added a comment.EditedJun 25 2019, 5:23 PM

Oh, but there's a double slash and a typo here:

@api_route(r'/origin/(?P<origin_type>[a-z]+)/url/(?P<origin_url>.+)'
           '//intrinsic-metadata', 'api-origin-intrinsic-metdata-get')

it should be replaced by:

@api_route(r'/origin/(?P<origin_type>[a-z]+)/url/(?P<origin_url>.+)'
           '/intrinsic-metadata', 'api-origin-intrinsic-metadata-get')
vlorentz requested changes to this revision.Jun 25 2019, 5:23 PM
This revision now requires changes to proceed.Jun 25 2019, 5:23 PM
swh-public-ci added a comment.Jun 25 2019, 5:24 PM

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

Harbormaster completed remote builds in B6481: Diff 5491.Jun 25 2019, 5:24 PM
twitu updated this revision to Diff 5492.Jun 25 2019, 5:31 PM

Fix typo

vlorentz added a comment.Jun 25 2019, 5:32 PM

The + is not needed at the end off the line

swh-public-ci added a comment.Jun 25 2019, 5:38 PM

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

Harbormaster completed remote builds in B6482: Diff 5492.Jun 25 2019, 5:38 PM
twitu updated this revision to Diff 5493.Jun 25 2019, 5:40 PM

Remove + symbol from end of line

twitu updated this revision to Diff 5494.Jun 25 2019, 5:41 PM

Remove tox dependency

swh-public-ci added a comment.Jun 25 2019, 5:47 PM

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

Harbormaster completed remote builds in B6483: Diff 5493.Jun 25 2019, 5:47 PM
twitu added a comment.Jun 25 2019, 5:47 PM

Ready to land made all the required changes.

vlorentz accepted this revision.Jun 25 2019, 5:48 PM
This revision is now accepted and ready to land.Jun 25 2019, 5:48 PM
swh-public-ci added a comment.Jun 25 2019, 5:53 PM

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

Harbormaster completed remote builds in B6484: Diff 5494.Jun 25 2019, 5:53 PM
twitu updated this revision to Diff 5495.Jun 25 2019, 5:57 PM

Merge with master

swh-public-ci added a comment.Jun 25 2019, 6:04 PM

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

Harbormaster completed remote builds in B6485: Diff 5495.Jun 25 2019, 6:04 PM
Closed by commit rDWAPPS9a8e1f98c9fd: Add origin_metadata_get API endpoint (authored by twitu). · Explain WhyJun 25 2019, 6:10 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
swh/
web/
api/
views/
44 lines
common/
22 lines
tests/
api/
views/
33 lines

Diff 5496

View Options

swh/web/api/views/origin.py

Loading...
View Options

swh/web/common/service.py

Loading...
View Options

swh/web/tests/api/views/test_origin.py

Loading...
About · Developer information · Legal