Page MenuHomeSoftware Heritage

api/origin: Do not attempt to lookup similar origin URLs
ClosedPublic

Authored by anlambert on Jun 14 2022, 3:01 PM.

Details

Summary

Web API should be as dumb as possible and should not attempt to
lookup an origin with and without trailing slash in its URL.

That regression was introduced in rDWAPPS56fb869644ad45e0648f4ad45c07619d468019b4 but that process should
only be performed in a Web UI context.

Related to D7980

Depends on D8056

Diff Detail

Repository
rDWAPPS Web applications
Branch
api-origin-no-url-processing
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 29860
Build 46672: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 46671: arc lint + arc unit

Event Timeline

Build is green

Patch application report for D7988 (id=28777)

Rebasing onto 662ce6eafc...

Current branch diff-target is up to date.
Changes applied before test
commit b92ae62dfc66a937a5f678addd121ef15ab03f56
Author: Antoine Lambert <anlambert@softwareheritage.org>
Date:   Tue Jun 14 13:45:53 2022 +0200

    api/origin: Do not attempt to lookup similar origin URLs
    
    Web API should be as dumb as possible and should not attempt to
    lookup an origin with and without trailing slash in its URL.
    
    That regression was introduced in 56fb869 but that process should
    only be performed in a Web UI context.
    
    Related to D7980

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

Build is green

Patch application report for D7988 (id=28778)

Rebasing onto 662ce6eafc...

Current branch diff-target is up to date.
Changes applied before test
commit 9e82cd6ee59e3817c18a8b3a05af4c2259f1d642
Author: Antoine Lambert <anlambert@softwareheritage.org>
Date:   Tue Jun 14 13:45:53 2022 +0200

    api/origin: Do not attempt to lookup similar origin URLs
    
    Web API should be as dumb as possible and should not attempt to
    lookup an origin with and without trailing slash in its URL.
    
    rDWAPPS56fb869644ad45e0648f4ad45c07619d468019b4 introduced that
    regression but that process  should only be performed in a Web
    UI context.
    
    Related to D7980

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

douardda added inline comments.
swh/web/api/views/origin.py
351–353

I don't get why this (and similar snippets below) is related to this diff's description.

Also, this will "break the API". I am not opposed to this change, but some care should be taken when changing public's API behavior like this. (attempt to check if there are users of the behavior to be modified, document the new behavior, display/return a warning or something, etc)

Also, this will "break the API". I am not opposed to this change, but some care should be taken when changing public's API behavior like this. (attempt to check if there are users of the behavior to be modified, document the new behavior, display/return a warning or something, etc)

I looked at the stats collected by piwik and the origin endpoint does not seem to be queried a lot in the current year so I do not think that change will impact a lot of users.

swh/web/api/views/origin.py
351–353

All API endpoints looking for an origin should have the same behavior, see added test.

But I will move that origin existence check in the archive module instead.

Rebase and centralize similar URLs processing in common/archive.py module.

Build is green

Patch application report for D7988 (id=28822)

Rebasing onto d5f020c02b...

Current branch diff-target is up to date.
Changes applied before test
commit d2cdd09d6f50ff3e18978d40f180eb55a023dda4
Author: Antoine Lambert <anlambert@softwareheritage.org>
Date:   Tue Jun 14 13:45:53 2022 +0200

    api/origin: Do not attempt to lookup similar origin URLs
    
    Web API should be as dumb as possible and should not attempt to
    lookup an origin with and without trailing slash in its URL.
    
    rDWAPPS56fb869644ad45e0648f4ad45c07619d468019b4 introduced that
    regression but that process should only be performed in a Web
    UI context.
    
    Related to D7980

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

This revision is now accepted and ready to land.Jun 30 2022, 11:03 AM

Build has FAILED

Patch application report for D7988 (id=29067)

Rebasing onto 9e89db680b...

Current branch diff-target is up to date.
Changes applied before test
commit 7aec2d43e6171b1d91a9f70bd50da8ef9499f4ce
Author: Antoine Lambert <anlambert@softwareheritage.org>
Date:   Tue Jun 14 13:45:53 2022 +0200

    api/origin: Do not attempt to lookup similar origin URLs
    
    Web API should be as dumb as possible and should not attempt to
    lookup an origin with and without trailing slash in its URL.
    
    rDWAPPS56fb869644ad45e0648f4ad45c07619d468019b4 introduced that
    regression but that process should only be performed in a Web
    UI context.
    
    Related to D7980

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

Build has FAILED

Patch application report for D7988 (id=29071)

Could not rebase; Attempt merge onto 9e89db680b...

Updating 9e89db68..75b37286
Fast-forward
 requirements-test.txt                  |  2 ++
 swh/web/api/views/origin.py            | 10 +++++++--
 swh/web/common/archive.py              | 33 +++++++++++++++++++++++-----
 swh/web/common/origin_visits.py        | 13 ++++++-----
 swh/web/tests/api/views/test_origin.py | 40 +++++++++++++++++++++++++++++-----
 5 files changed, 79 insertions(+), 19 deletions(-)
Changes applied before test
commit 75b37286fe99577d84d142422b729ac3a7315c41
Author: Antoine Lambert <anlambert@softwareheritage.org>
Date:   Tue Jun 14 13:45:53 2022 +0200

    api/origin: Do not attempt to lookup similar origin URLs
    
    Web API should be as dumb as possible and should not attempt to
    lookup an origin with and without trailing slash in its URL.
    
    rDWAPPS56fb869644ad45e0648f4ad45c07619d468019b4 introduced that
    regression but that process should only be performed in a Web
    UI context.
    
    Related to D7980

commit 70c242f2d98c90883e6b7533be61fca6b86bc1cc
Author: Antoine Lambert <anlambert@softwareheritage.org>
Date:   Thu Jun 30 12:24:53 2022 +0200

    requirements-test: Ensure pinned mypy version is used with tox
    
    djangorestframework-stubs 1.7.0 pin mypy to > 0.950 in its requirements,
    thus the mypy pinning to 0.942 in tox.ini is no longer respected and
    mypy 0.961 is used when running tox, leading to new typing errors.
    
    So ensure mypy 0.942 is used until we bump mypy version used by swh.

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

Build is green

Patch application report for D7988 (id=29071)

Could not rebase; Attempt merge onto 9e89db680b...

Updating 9e89db68..75b37286
Fast-forward
 requirements-test.txt                  |  2 ++
 swh/web/api/views/origin.py            | 10 +++++++--
 swh/web/common/archive.py              | 33 +++++++++++++++++++++++-----
 swh/web/common/origin_visits.py        | 13 ++++++-----
 swh/web/tests/api/views/test_origin.py | 40 +++++++++++++++++++++++++++++-----
 5 files changed, 79 insertions(+), 19 deletions(-)
Changes applied before test
commit 75b37286fe99577d84d142422b729ac3a7315c41
Author: Antoine Lambert <anlambert@softwareheritage.org>
Date:   Tue Jun 14 13:45:53 2022 +0200

    api/origin: Do not attempt to lookup similar origin URLs
    
    Web API should be as dumb as possible and should not attempt to
    lookup an origin with and without trailing slash in its URL.
    
    rDWAPPS56fb869644ad45e0648f4ad45c07619d468019b4 introduced that
    regression but that process should only be performed in a Web
    UI context.
    
    Related to D7980

commit 70c242f2d98c90883e6b7533be61fca6b86bc1cc
Author: Antoine Lambert <anlambert@softwareheritage.org>
Date:   Thu Jun 30 12:24:53 2022 +0200

    requirements-test: Ensure pinned mypy version is used with tox
    
    djangorestframework-stubs 1.7.0 pin mypy to > 0.950 in its requirements,
    thus the mypy pinning to 0.942 in tox.ini is no longer respected and
    mypy 0.961 is used when running tox, leading to new typing errors.
    
    So ensure mypy 0.942 is used until we bump mypy version used by swh.

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