Page MenuHomeSoftware Heritage

Deprecate /browse/origin/log/ URLs
Needs RevisionPublic

Authored by jayeshv on Fri, Oct 8, 12:33 PM.

Details

Summary

Many URLs defined in the origin view (urlconf) are redundant.
This fix redirects /origin/logs?origin_url=<url> and
/origin/<url>/log to /snapshot/log/

  • Generic decorator to deprecate a route
  • Generic logic to forward url args and query params.

Related to T3608

Diff Detail

Repository
rDWAPPS Web applications
Branch
directory_route
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 24364
Build 38025: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 38024: arc lint + arc unit

Event Timeline

Build is green

Patch application report for D6441 (id=23393)

Rebasing onto 264c0bc84b...

Current branch diff-target is up to date.
Changes applied before test
commit 1d1903e0dc6485b5b3ffa146ef8d2e281bb81453
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Fri Oct 8 12:29:46 2021 +0200

    Deprecate most of the /browse/origin/.* URLs
    
    Many URLs defined in the origin view (urlconf) are redundant.
    This fix redirects /origin/logs?origin_url=<url> and
    /origin/<url>/log to /snapshot-log/
    - Generic decorator to deprecate a route
    - Generic logic to forward url args and query params.
    
    Related to T3608

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

Build is green

Patch application report for D6441 (id=23395)

Rebasing onto 264c0bc84b...

Current branch diff-target is up to date.
Changes applied before test
commit 3c2da2ad33cf614c5e014a61457a4fed56554bd6
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Fri Oct 8 12:39:33 2021 +0200

    Deprecate most of the /browse/origin/.* URLs
    
    Many URLs defined in the origin view (urlconf) are redundant.
    This fix redirects /origin/logs?origin_url=<url> and
    /origin/<url>/log to /snapshot-log/
    - Generic decorator to deprecate a route
    - Generic logic to forward url args and query params.
    
    Related to T3608

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

WIP: Many URLs defined in the origin view (urlconf) are redundant.
This fix redirects /origin/branches?origin_url=<url> and
/origin/<url>/branches to /snapshot-branches/

Build has FAILED

Patch application report for D6441 (id=23417)

Rebasing onto 264c0bc84b...

Current branch diff-target is up to date.
Changes applied before test
commit 3bbb4320b179ab47b453e26a36f3b290c5ee57f4
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Fri Oct 8 16:19:35 2021 +0200

    test-changes

commit f5b3cd19b787479ad090eba7f6db824783ed0978
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Fri Oct 8 16:01:40 2021 +0200

    branch routes

commit 3c2da2ad33cf614c5e014a61457a4fed56554bd6
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Fri Oct 8 12:39:33 2021 +0200

    Deprecate most of the /browse/origin/.* URLs
    
    Many URLs defined in the origin view (urlconf) are redundant.
    This fix redirects /origin/logs?origin_url=<url> and
    /origin/<url>/log to /snapshot-log/
    - Generic decorator to deprecate a route
    - Generic logic to forward url args and query params.
    
    Related to T3608

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

Build is green

Patch application report for D6441 (id=23418)

Rebasing onto 264c0bc84b...

Current branch diff-target is up to date.
Changes applied before test
commit 3c2da2ad33cf614c5e014a61457a4fed56554bd6
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Fri Oct 8 12:39:33 2021 +0200

    Deprecate most of the /browse/origin/.* URLs
    
    Many URLs defined in the origin view (urlconf) are redundant.
    This fix redirects /origin/logs?origin_url=<url> and
    /origin/<url>/log to /snapshot-log/
    - Generic decorator to deprecate a route
    - Generic logic to forward url args and query params.
    
    Related to T3608

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

anlambert added a subscriber: anlambert.

I added some inline comments to handle.

Also we should remove the use of /browse/origin/log/ URLs in codebase, grep for "browse-origin-log" to find them.

swh/web/browse/views/origin.py
24–45

Could you remove the decorator wrapping and simply call redirect_to_new_route in views implementation ?
It feels weird to have functions without any instructions in them.

swh/web/browse/views/snapshot.py
129

Please use r"snapshot/log/ instead.

140

if snasphot_id is None

141–157

origin_url and timestamp query parameters must be passed to browse_snapshot_log to
ensure same behavior as before.

swh/web/tests/browse/views/test_origin.py
867–868

Please move the redirection check in a separate test.

This revision now requires changes to proceed.Mon, Oct 11, 1:22 PM
swh/web/browse/views/origin.py
24–45

Ok, do you think adding an extra line in the doc string in each deprecated function would be enough?. It would be like "This route is deprecated, use <alternate name> instead" ?

swh/web/browse/views/origin.py
24–45

Yes this should be enough, you can reference the correct view to use in sphinx using :http:get:<url>, see how its is done for the vault.

swh/web/browse/views/origin.py
24–45

Ok, Thanks. In fact, I have that doc string added in the coming diffs (for branches and releases). So, I will move that change to this diff and keep the decorator

Build is green

Patch application report for D6441 (id=23441)

Rebasing onto dc489d1df2...

First, rewinding head to replay your work on top of it...
Applying: Deprecate /origin/log route
Changes applied before test
commit b169d9dffbe99d0585d1bd2c965ba5ddc33c86d9
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Mon Oct 11 14:56:52 2021 +0200

    Deprecate /origin/log route
    
    This fix deprecates and redirects /origin/log?origin_url=<url> and
    /origin/<url>/log to /snapshot/log/
    - Generic decorator to deprecate a route
    - Generic logic to forward url args and query params.
    
    Related to T3608

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

swh/web/tests/browse/views/test_origin.py
874

This should be assert "/snapshot/{browse_context}/" in resp.url
I will fix this with branch/release diff

It seems that the browse-snapshot-log view is missing tests so we should create the file swh/web/tests/browse/views/test_snapshot.py
and implement tests here.

Endpoint documentation must also be updated:

  • /browse/origin/directory/ must be marked as deprecated in docs/uri-scheme-browse-origin.rst
  • /browse/snapshot/log/ documentation must be added in docs/uri-scheme-browse-snapshot.rst indicating the origin_url query parameter is mandatory and that a redirection will be performed to browse the log from the latest snapshot of the origin.
swh/web/browse/views/snapshot.py
26

If origin_url is not provided, get_snapshot_context will fail so you should raise a BadInputExc in that case to get a 400 error page.

142

Could you do a redirection to the same view with the snapshot argument instead ?
This way, the latest snapshot id will be displayed in browser URL bar.

swh/web/tests/browse/views/test_origin.py
869–874

This test is not complete and can be improved, use this instead:

@given(origin())
@pytest.mark.parametrize("browse_context", ["log"])
def test_origin_view_redirects(client, browse_context, origin):
    query_params = {"origin_url": origin["url"]}
    url = reverse(f"browse-origin-{browse_context}", query_params=query_params)

    resp = check_html_get_response(client, url, status_code=301)
    assert resp["location"] == reverse(
        f"browse-snapshot-{browse_context}", query_params=query_params
    )
This revision now requires changes to proceed.Mon, Oct 11, 3:47 PM
anlambert retitled this revision from Deprecate most of the /browse/origin/.* URLs to Deprecate /browse/origin/log/ URLs.Mon, Oct 11, 3:48 PM

Review changes

  • checking for origin_url in request params
  • better tests
  • redirect to /snapshot/log/<id>

Build is green

Patch application report for D6441 (id=23447)

Rebasing onto d7c16735a4...

Current branch diff-target is up to date.
Changes applied before test
commit 47f58ef7fcc396761e6d08e784351356f51a5f22
Author: Jayesh Velayudhan <jayesh@softwareheritage.org>
Date:   Mon Oct 11 16:10:53 2021 +0200

    Deprecate /origin/log route
    
    This fix deprecates and redirects /origin/log?origin_url=<url> and
    /origin/<url>/log to /snapshot/log/
    - Generic decorator to deprecate a route
    - Generic logic to forward url args and query params.
    
    Related to T3608

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

Still some work to do before I can accept the diff:

  • add missing tests
  • remove the use of the browse-origin-log view in codebase (grep for it)
swh/web/browse/views/origin.py
25–31

I think it will be better to add a view_redirect parameter to the decorator instead of storing the deprecation_mapping dict in it.
It will make the code more readable and the decorator more generic.

swh/web/browse/views/snapshot.py
23

request.GET is already a dictionary so that instruction is not needed.pt

28

We need a test for this.

134–157

That view is not covered by tests, we must add some in a new tests/browse/views/test_snapshot.py file.

This revision now requires changes to proceed.Tue, Oct 12, 10:50 AM