Page MenuHomeSoftware Heritage
Differential D3046

browse: Disambiguate URLs related to origin / snapshot context
ClosedPublic
Actions

Authored by anlambert on Apr 22 2020, 6:07 PM.

Details

Reviewers
ardumont
vlorentz
Group Reviewers
Reviewers
Maniphest Tasks
T2115: Directories named 'directory' cannot be browsed in snapshot context views
T2135: Disambiguate browse URL regexps
Commits
rDWAPPS6945c4efa49b: browse: Disambiguate URLs related to origin / snapshot context browsing
Summary

Still on the road to T2342, I thought removing ambiguities in current browse URL scheme
was a prerequisite to ensure all archive objects can be correctly browsed and referenced
with updated SWHIDs.

So disambiguate and/or deprecate the following browse URLs:

  • GET /browse/origin/(origin_url)/content/(path)/
  • GET /browse/origin/(origin_url)/directory/(path)/
  • GET /browse/origin/(origin_url)/visit/(timestamp)/branches/
  • GET /browse/origin/(origin_url)/visit/(timestamp)/content/(path)/
  • GET /browse/origin/(origin_url)/visit/(timestamp)/directory/(path)/
  • GET /browse/origin/(origin_url)/visit/(timestamp)/log/
  • GET /browse/origin/(origin_url)/visit/(timestamp)/releases/
  • GET /browse/snapshot/(snapshot_id)/content/(path)/
  • GET /browse/snapshot/(snapshot_id)/directory/(path)/

Replace their use by providing timestamp and path as query parameters to these endpoints
for consistency:

  • GET /browse/origin/(origin_url)/content/
  • GET /browse/origin/(origin_url)/directory/
  • GET /browse/origin/(origin_url)/log/
  • GET /browse/origin/(origin_url)/branches/
  • GET /browse/origin/(origin_url)/releases/
  • GET /browse/snapshot/(snapshot_id)/content/
  • GET /browse/snapshot/(snapshot_id)/directory/

Endpoints documentation have also been updated to reflect these changes.

Sorry for the huge diff but I did not have a choice to process all URLs in one batch.

Closes T2115
Closes T2135

Diff Detail

Repository
rDWAPPS Web applications
Branch
disambiguate-browse-origin-snapshot-urls
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 11986
Build 18173: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 18172: arc lint + arc unit

Event Timeline

anlambert created this revision.Apr 22 2020, 6:07 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptApr 22 2020, 6:07 PM
swh-public-ci added a comment.Apr 22 2020, 6:15 PM

Build is green

Patch application report for D3046 (id=10828)

Rebasing onto 6708b108fb...

Current branch diff-target is up to date.
Changes applied before test
commit 79b2177e8c4e8f2875718c251d0bb0a5fab0feaf
Author: Antoine Lambert <antoine.lambert@inria.fr>
Date:   Wed Apr 22 17:28:02 2020 +0200

    browse: Disambiguate URLs related to origin / snapshot context browsing
    
    Disambiguate and deprecate the following browse URLs:
    
      - GET /browse/origin/(origin_url)/content/(path)/
    
      - GET /browse/origin/(origin_url)/directory/(path)/
    
      - GET /browse/origin/(origin_url)/visit/(timestamp)/branches/
    
      - GET /browse/origin/(origin_url)/visit/(timestamp)/content/(path)/
    
      - GET /browse/origin/(origin_url)/visit/(timestamp)/directory/(path)/
    
      - GET /browse/origin/(origin_url)/visit/(timestamp)/log/
    
      - GET /browse/origin/(origin_url)/visit/(timestamp)/releases/
    
      - GET /browse/snapshot/(snapshot_id)/content/(path)/
    
      - GET /browse/snapshot/(snapshot_id)/directory/(path)/
    
    Replace their use by providing timestamp and path as query parameters to these endpoints:
    
      - GET /browse/origin/(origin_url)/content/
    
      - GET /browse/origin/(origin_url)/directory/
    
      - GET /browse/origin/(origin_url)/log/
    
      - GET /browse/origin/(origin_url)/branches/
    
      - GET /browse/origin/(origin_url)/releases/
    
      - GET /browse/snapshot/(snapshot_id)/content/
    
      - GET /browse/snapshot/(snapshot_id)/directory/
    
    Update endpoints documentation to reflect these changes.
    
    Related to T2135
    Closes T2115

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

Harbormaster completed remote builds in B11983: Diff 10828.Apr 22 2020, 6:15 PM
anlambert updated this revision to Diff 10831.Apr 22 2020, 6:17 PM

Update: Doc fix

anlambert retitled this revision from browse: Disambiguate URLs related to origin / snapshot context browsing to browse: Disambiguate URLs related to origin / snapshot context.Apr 22 2020, 6:19 PM
swh-public-ci added a comment.Apr 22 2020, 6:25 PM

Build is green

Patch application report for D3046 (id=10831)

Rebasing onto 6708b108fb...

Current branch diff-target is up to date.
Changes applied before test
commit b54b0d46dfbd11aa2e26b183229d1ebec624b7ce
Author: Antoine Lambert <antoine.lambert@inria.fr>
Date:   Wed Apr 22 17:28:02 2020 +0200

    browse: Disambiguate URLs related to origin / snapshot context browsing
    
    Disambiguate and/or deprecate the following browse URLs:
    
      - GET /browse/origin/(origin_url)/content/(path)/
    
      - GET /browse/origin/(origin_url)/directory/(path)/
    
      - GET /browse/origin/(origin_url)/visit/(timestamp)/branches/
    
      - GET /browse/origin/(origin_url)/visit/(timestamp)/content/(path)/
    
      - GET /browse/origin/(origin_url)/visit/(timestamp)/directory/(path)/
    
      - GET /browse/origin/(origin_url)/visit/(timestamp)/log/
    
      - GET /browse/origin/(origin_url)/visit/(timestamp)/releases/
    
      - GET /browse/snapshot/(snapshot_id)/content/(path)/
    
      - GET /browse/snapshot/(snapshot_id)/directory/(path)/
    
    Replace their use by providing timestamp and path as query parameters to these endpoints
    for consistency:
    
      - GET /browse/origin/(origin_url)/content/
    
      - GET /browse/origin/(origin_url)/directory/
    
      - GET /browse/origin/(origin_url)/log/
    
      - GET /browse/origin/(origin_url)/branches/
    
      - GET /browse/origin/(origin_url)/releases/
    
      - GET /browse/snapshot/(snapshot_id)/content/
    
      - GET /browse/snapshot/(snapshot_id)/directory/
    
    Update endpoints documentation to reflect these changes.
    
    Related to T2135
    Closes T2115

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

Harbormaster completed remote builds in B11986: Diff 10831.Apr 22 2020, 6:25 PM
ardumont added a subscriber: ardumont.Apr 23 2020, 9:43 AM
ardumont added inline comments.
docs/uri-scheme-browse-origin.rst
579

I am not sure we want to keep that dateutil.parser.parse. Its function input
is a plain string (quite large) and its output is sometimes unexpected [1]

We already discussed in other modules that we should converge on using iso8601
dates (well use that module at least). So might be, it'd be best to use more
conventional timestamp formats, iso8601 sounds like one sensible choice for at
least one format.

I imagine we have that behavior already so that might be another task entirely.
I just mention it here since we are reworking endpoints.

[1] https://forge.softwareheritage.org/D2463#change-xBZ2jUSgNbl8

vlorentz added a subscriber: vlorentz.Apr 23 2020, 11:14 AM

Cool!

Wouldn't this be a good time to also move the URL to a query parameter? (Because of the double-slash issue that can happen on clients)

ardumont added a comment.Apr 23 2020, 11:43 AM

Wouldn't this be a good time to also move the URL to a query parameter? (Because of the double-slash issue that can happen on clients)

I refrain from asking this as this would be quite a change :D
Maybe that could be done as an extra step.

ardumont accepted this revision.Apr 23 2020, 11:54 AM

Looks good.

(Remains my question about keeping dateutil.parser.parse_date though)

This revision is now accepted and ready to land.Apr 23 2020, 11:54 AM
vlorentz accepted this revision.Apr 23 2020, 11:54 AM

yes, indeed

ardumont added a comment.Apr 23 2020, 11:56 AM

oh and also

/or deprecate the following browse URLs:

for how long do we deprecate? What's our timeframe policy to remove those, if we have any?

anlambert added a comment.Apr 23 2020, 11:59 AM

Wouldn't this be a good time to also move the URL to a query parameter? (Because of the double-slash issue that can happen on clients)

From my point of view, having the browsed object identifier as URL argument is better for consistency with the rest of the browse URL scheme.

Regarding the double-slash issue, I think we already mitigated it in rDWAPPS56fb869644ad45e0648f4ad45c07619d468019b4

anlambert added a comment.Apr 23 2020, 12:07 PM

I am not sure we want to keep that dateutil.parser.parse. Its function input
is a plain string (quite large) and its output is sometimes unexpected [1]

We already discussed in other modules that we should converge on using iso8601
dates (well use that module at least). So might be, it'd be best to use more
conventional timestamp formats, iso8601 sounds like one sensible choice for at
least one format.

I imagine we have that behavior already so that might be another task entirely.
I just mention it here since we are reworking endpoints.

Well, hopefully we already use ISO dates in origin visit URLs (see https://archive.softwareheritage.org/browse/origin/https://github.com/python/cpython/visit/2020-01-18T09:31:26Z/directory/ for instance).

I agree that restricting the date format is a good idea for consistency. I propose to only support Unix timestamps and ISO dates.

anlambert added a comment.Apr 23 2020, 12:11 PM

for how long do we deprecate? What's our timeframe policy to remove those, if we have any?

I do not really know, at least a couple of months in order to check if these endpoints are still requested in production.

ardumont added a subscriber: douardda.Apr 23 2020, 12:12 PM

I agree that restricting the date format is a good idea for consistency. I propose to only support Unix timestamps and ISO dates.

Awesome, i know at least @douardda will be happy to hear it ;)
Maybe make make that happen in a new diff when you have the time (as you wish ;)

Cheers mate ;)

anlambert added a comment.Apr 23 2020, 12:23 PM
In D3046#74009, @ardumont wrote:

I agree that restricting the date format is a good idea for consistency. I propose to only support Unix timestamps and ISO dates.

Awesome, i know at least @douardda will be happy to hear it ;)
Maybe make make that happen in a new diff when you have the time (as you wish ;)

Cheers mate ;)

Of course, this is out of scope to integrate these changes in that diff.

I created a new task on the subject (T2375)

anlambert updated this revision to Diff 10842.Apr 23 2020, 12:43 PM

Update commit message

anlambert edited the summary of this revision. (Show Details)Apr 23 2020, 12:44 PM
swh-public-ci added a comment.Apr 23 2020, 12:51 PM

Build is green

Patch application report for D3046 (id=10842)

Rebasing onto 6708b108fb...

Current branch diff-target is up to date.
Changes applied before test
commit 6945c4efa49b05877a3be5e6bb72af988ebbe5bd
Author: Antoine Lambert <antoine.lambert@inria.fr>
Date:   Wed Apr 22 17:28:02 2020 +0200

    browse: Disambiguate URLs related to origin / snapshot context browsing
    
    Disambiguate and/or deprecate the following browse URLs:
    
      - GET /browse/origin/(origin_url)/content/(path)/
    
      - GET /browse/origin/(origin_url)/directory/(path)/
    
      - GET /browse/origin/(origin_url)/visit/(timestamp)/branches/
    
      - GET /browse/origin/(origin_url)/visit/(timestamp)/content/(path)/
    
      - GET /browse/origin/(origin_url)/visit/(timestamp)/directory/(path)/
    
      - GET /browse/origin/(origin_url)/visit/(timestamp)/log/
    
      - GET /browse/origin/(origin_url)/visit/(timestamp)/releases/
    
      - GET /browse/snapshot/(snapshot_id)/content/(path)/
    
      - GET /browse/snapshot/(snapshot_id)/directory/(path)/
    
    Replace their use by providing timestamp and path as query parameters to these endpoints
    for consistency:
    
      - GET /browse/origin/(origin_url)/content/
    
      - GET /browse/origin/(origin_url)/directory/
    
      - GET /browse/origin/(origin_url)/log/
    
      - GET /browse/origin/(origin_url)/branches/
    
      - GET /browse/origin/(origin_url)/releases/
    
      - GET /browse/snapshot/(snapshot_id)/content/
    
      - GET /browse/snapshot/(snapshot_id)/directory/
    
    Update endpoints documentation to reflect these changes.
    
    Closes T2115
    Closes T2135

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

Harbormaster completed remote builds in B11997: Diff 10842.Apr 23 2020, 12:51 PM
vlorentz added a comment.Apr 23 2020, 12:56 PM
In D3046#74006, @anlambert wrote:

Wouldn't this be a good time to also move the URL to a query parameter? (Because of the double-slash issue that can happen on clients)

From my point of view, having the browsed object identifier as URL argument is better for consistency with the rest of the browse URL scheme.

Except those with a path, starting with this diff...

Regarding the double-slash issue, I think we already mitigated it in rDWAPPS56fb869644ad45e0648f4ad45c07619d468019b4

That only fixes trailing slashes, not all double slashes (eg. http:// in the origin url)

anlambert added a comment.Apr 23 2020, 1:10 PM
In D3046#74031, @vlorentz wrote:
In D3046#74006, @anlambert wrote:

Wouldn't this be a good time to also move the URL to a query parameter? (Because of the double-slash issue that can happen on clients)

From my point of view, having the browsed object identifier as URL argument is better for consistency with the rest of the browse URL scheme.

Except those with a path, starting with this diff...

Regarding the double-slash issue, I think we already mitigated it in rDWAPPS56fb869644ad45e0648f4ad45c07619d468019b4

That only fixes trailing slashes, not all double slashes (eg. http:// in the origin url)

All right, will turn origin_url as a query parameter then before landing that diff.

vlorentz added a comment.Apr 23 2020, 2:05 PM

You can do it in another diff, that will be less code to review

anlambert added a comment.Apr 23 2020, 2:18 PM
In D3046#74055, @vlorentz wrote:

You can do it in another diff, that will be less code to review

Yes, you're right. Landing this then.

Closed by commit rDWAPPS6945c4efa49b: browse: Disambiguate URLs related to origin / snapshot context browsing (authored by anlambert). · Explain WhyApr 23 2020, 2:42 PM
This revision was automatically updated to reflect the committed changes.
anlambert added a commit: rDWAPPS6945c4efa49b: browse: Disambiguate URLs related to origin / snapshot context browsing.

Revision Contents
Changeset List

PathSizeCoverage (All)Coverage (Touched)
cypress/
integration/
2 lines--
4 lines--
2 lines--
4 lines--
4 lines--
4 lines--
docs/
163 lines--
89 lines--
11 lines--
swh/
web/
browse/
52 lines80%
Loading...
8 lines84%
Loading...
views/
133 lines80%
Loading...
2 lines63%
Loading...
55 lines52%
Loading...
templates/
browse/
8 lines--
tests/
browse/
38 lines99%
Loading...
views/
81 lines98%
Loading...

Diff 10831

View Options

cypress/integration/code-highlighting.spec.js

Loading...
View Options

cypress/integration/content-display.spec.js

Loading...
View Options

cypress/integration/directory.spec.js

Loading...
View Options

cypress/integration/language-select.spec.js

Loading...
View Options

cypress/integration/origin-visits.spec.js

Loading...
View Options

cypress/integration/persistent-identifiers.spec.js

Loading...
View Options

docs/uri-scheme-browse-origin.rst

Loading...
View Options

docs/uri-scheme-browse-snapshot.rst

Loading...
View Options

docs/uri-scheme-browse.rst

Loading...
View Options

swh/web/browse/snapshot_context.py

Loading...
View Options

swh/web/browse/utils.py

Loading...
View Options

swh/web/browse/views/origin.py

Loading...
View Options

swh/web/browse/views/revision.py

Loading...
View Options

swh/web/browse/views/snapshot.py

Loading...
View Options

swh/web/templates/browse/help.html

Loading...
View Options

swh/web/tests/browse/test_snapshot_context.py

Loading...
View Options

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

Loading...
About · Developer information · Legal