Page MenuHomeSoftware Heritage
Differential D5484

assets/webapp-utils: Add SWHID validation in search inputs
ClosedPublic
Actions

Authored by anlambert on Apr 12 2021, 3:25 PM.

Details

Reviewers
vlorentz
Group Reviewers
Reviewers
Maniphest Tasks
T3247: Implement SWHID validation in frontend
T3234: Handle gracefully trailing slashes when resolving SWHID in search box
Commits
rDWAPPSf3f1b7c84a73: assets/webapp-utils: Add SWHID validation in search inputs
Summary

Check a SWHID entered in a search input is valid by sending a request
to the resolve endpoint of the Web API.

If the SWHID is not valid, precise parsing error will be extracted from
the Web API response and displayed in the Web UI to indicate the search
form can not be validated.

As the resolve endpoint will be more requested, we should change its rate limit settings
to allow more requests (120 per minute for instance).


Related to T3247

Diff Detail

Repository
rDWAPPS Web applications
Branch
validate-swhid-in-search-inputs
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 20714
Build 32149: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 32148: arc lint + arc unit

Event Timeline

anlambert created this revision.Apr 12 2021, 3:25 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptApr 12 2021, 3:25 PM
swh-public-ci added a comment.Apr 12 2021, 3:33 PM

Build is green

Patch application report for D5484 (id=19612)

Rebasing onto 7131f6cd49...

Current branch diff-target is up to date.
Changes applied before test
commit 4175e28e3d11a00081bec042558808736f2d8016
Author: Antoine Lambert <antoine.lambert@inria.fr>
Date:   Mon Apr 12 15:15:44 2021 +0200

    assets/webapp-utils: Automatically fix malformed SWHID in search inputs
    
    Handle malformed SWHID entered in search input due to an invalid trailing
    slash, for instance when copying the SWHID from its browse URL.
    
    Remove the erroneous trailing slash in that case and update the input.
    
    Related to T3234

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

Harbormaster completed remote builds in B20644: Diff 19612.Apr 12 2021, 3:33 PM
anlambert requested review of this revision.Apr 12 2021, 3:33 PM
vlorentz added a subscriber: vlorentz.Apr 12 2021, 3:39 PM

Are you planning to add a warning in a future diff?

vlorentz added a comment.Apr 12 2021, 3:55 PM

imo the new code in webapp-utils.js should be in its own function, because the $(document).ready anonymous function is starting to be very long

assets/src/bundles/webapp/webapp-utils.js
182

could you document the motivation? (you can copy-paste the example in T3234)

194

Doesn't work if there is = in the value

vlorentz requested changes to this revision.Apr 12 2021, 3:55 PM
This revision now requires changes to proceed.Apr 12 2021, 3:55 PM
anlambert added a comment.Apr 12 2021, 4:34 PM
In D5484#139147, @vlorentz wrote:

Are you planning to add a warning in a future diff?

Nope, I do not see the interest. Such copy paste errors will be pretty rare imho.

But we could add a SWHID validator for the search input, what do you think ?

imo the new code in webapp-utils.js should be in its own function, because the $(document).ready anonymous function is starting to be very long

Ack

assets/src/bundles/webapp/webapp-utils.js
182

I will add the URL of the task in the comment.

194

Based on qualifiers parsing code in swh-model, having a = character in qualifier value will raise a ValidationError.

I guess I can check the size of the split and abort if is not equal to 2.

anlambert updated this revision to Diff 19615.Apr 12 2021, 4:45 PM

Adapt according to @vlorentz comments.

swh-public-ci added a comment.Apr 12 2021, 4:52 PM

Build is green

Patch application report for D5484 (id=19615)

Rebasing onto 7131f6cd49...

Current branch diff-target is up to date.
Changes applied before test
commit 8b9e02c19bc9649cba836af4b668c5227158fa9f
Author: Antoine Lambert <antoine.lambert@inria.fr>
Date:   Mon Apr 12 15:15:44 2021 +0200

    assets/webapp-utils: Automatically fix malformed SWHID in search inputs
    
    Handle malformed SWHID entered in search input due to an invalid trailing
    slash, for instance when copying the SWHID from its browse URL.
    
    Remove the erroneous trailing slash in that case and update the input.
    
    Related to T3234

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

Harbormaster completed remote builds in B20646: Diff 19615.Apr 12 2021, 4:52 PM
vlorentz added inline comments.Apr 12 2021, 4:54 PM
assets/src/bundles/webapp/webapp-utils.js
194

then it's a bug in swh-model. From the SWHID spec:

<context_qualifier> ::=
    <origin_ctxt>
  | <visit_ctxt>
  | <anchor_ctxt>
  | <path_ctxt>
  ;
<origin_ctxt> ::= "origin" "=" <url_escaped> ;
<path_ctxt> ::= "path" "=" <path_absolute_escaped> ;

where

    <url_escaped> is a RFC 3987 IRI

in either case all occurrences of ; (and %, as required by the RFC) have been percent-encoded (as %3B and %25 respectively). Other characters can be percent-encoded, e.g., to improve readability and/or embeddability of SWHID in other contexts.

and from said RFC:

ipath-absolute = "/" [ isegment-nz *( "/" isegment ) ]
isegment       = *ipchar
ipchar         = iunreserved / pct-encoded / sub-delims / ":"
               / "@"
sub-delims     = "!" / "$" / "&" / "'" / "(" / ")"
               / "*" / "+" / "," / ";" / "="
194

forgot this line:

<path_absolute_escaped> is an <ipath-absolute> from RFC 3987, and
anlambert added inline comments.Apr 12 2021, 4:59 PM
assets/src/bundles/webapp/webapp-utils.js
194

Does it mean that we should split the qualifier at the first = character or percent encode it in qualifier value ?

vlorentz added inline comments.Apr 12 2021, 5:00 PM
assets/src/bundles/webapp/webapp-utils.js
194

the former

anlambert added inline comments.Apr 12 2021, 5:02 PM
assets/src/bundles/webapp/webapp-utils.js
194

Ack, will fix the issue in swh-model first and update that diff afterwards.

anlambert added inline comments.Apr 12 2021, 5:50 PM
assets/src/bundles/webapp/webapp-utils.js
194

D5487

anlambert planned changes to this revision.Apr 12 2021, 6:02 PM

Thinking it back, the proper way to avoid sending a malformed SWHID to the resolver is to implement a form validator
as it is already done for the save code now form (try to save a malformed URL and form will not be validated, details
of the validation issue are then displayed below the search input).

This way we will have an error displayed and the resolve request will not be send.
So the idea will be to implement a SWHID validator in Javascript to see if the input
SWHID respects the specification. An extra processing for erroneous trailing slash will
also be added with a proper error message displayed in that case.

anlambert mentioned this in D5485: docs/persistent-identifiers: Add guidelines for fixing invalid SWHIDs..Apr 12 2021, 6:20 PM
vlorentz added a comment.Apr 12 2021, 6:35 PM

Great idea!

anlambert updated this revision to Diff 19648.Apr 13 2021, 5:06 PM

Update: Validate SWHID and report any parsing errors instead of autofixing it.

anlambert retitled this revision from assets/webapp-utils: Automatically fix malformed SWHID in search inputs to assets/webapp-utils: Add SWHID validation in search inputs.Apr 13 2021, 5:07 PM
anlambert edited the summary of this revision. (Show Details)
anlambert edited the summary of this revision. (Show Details)
anlambert edited the summary of this revision. (Show Details)Apr 13 2021, 5:11 PM
swh-public-ci added a comment.Apr 13 2021, 5:13 PM

Build is green

Patch application report for D5484 (id=19648)

Rebasing onto 78a038b139...

First, rewinding head to replay your work on top of it...
Applying: assets/webapp-utils: Add SWHID validation in search inputs
Changes applied before test
commit d42538d7bd363faba738ebb7b6eb37a201b5d196
Author: Antoine Lambert <antoine.lambert@inria.fr>
Date:   Mon Apr 12 15:15:44 2021 +0200

    assets/webapp-utils: Add SWHID validation in search inputs
    
    Check a SWHID entered in a search input is valid by sending a request
    to the resolve endpoint of the Web API.
    
    If the SWHID is not valid, precise parsing error will be extracted from
    the Web API response and displayed in the Web UI to indicate the search
    form can not be validated.
    
    Related to T3234

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

Harbormaster completed remote builds in B20676: Diff 19648.Apr 13 2021, 5:13 PM
anlambert updated this revision to Diff 19685.Apr 14 2021, 11:43 AM

Rebase

anlambert edited the summary of this revision. (Show Details)Apr 14 2021, 11:43 AM
anlambert added a task: T3247: Implement SWHID validation in frontend.
swh-public-ci added a comment.Apr 14 2021, 11:50 AM

Build is green

Patch application report for D5484 (id=19685)

Rebasing onto 6fa50083a1...

Current branch diff-target is up to date.
Changes applied before test
commit c7fba2eff752aa4897d135c7411c4e919adf54e6
Author: Antoine Lambert <antoine.lambert@inria.fr>
Date:   Mon Apr 12 15:15:44 2021 +0200

    assets/webapp-utils: Add SWHID validation in search inputs
    
    Check a SWHID entered in a search input is valid by sending a request
    to the resolve endpoint of the Web API.
    
    If the SWHID is not valid, precise parsing error will be extracted from
    the Web API response and displayed in the Web UI to indicate the search
    form can not be validated.
    
    Related to T3247

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

Harbormaster completed remote builds in B20714: Diff 19685.Apr 14 2021, 11:50 AM
vlorentz added a comment.Apr 15 2021, 10:13 AM

Nice! Why the green labels/checkboxes/dropdowns?

anlambert added a comment.Apr 16 2021, 11:17 AM
In D5484#140108, @vlorentz wrote:

Nice! Why the green labels/checkboxes/dropdowns?

Because bootstrap apply the validation process on the whole form and all its elements.

This is disturbing at first sight but nevertheless it emphasizes the invalid input field.

I can try to validate only the input element but code will be more hackish.

anlambert updated this revision to Diff 19786.Apr 16 2021, 11:55 AM

Rebase

swh-public-ci added a comment.Apr 16 2021, 12:02 PM

Build is green

Patch application report for D5484 (id=19786)

Rebasing onto b095564e54...

Current branch diff-target is up to date.
Changes applied before test
commit f3f1b7c84a73d7717bcf79f58b62641c7b4d3424
Author: Antoine Lambert <antoine.lambert@inria.fr>
Date:   Mon Apr 12 15:15:44 2021 +0200

    assets/webapp-utils: Add SWHID validation in search inputs
    
    Check a SWHID entered in a search input is valid by sending a request
    to the resolve endpoint of the Web API.
    
    If the SWHID is not valid, precise parsing error will be extracted from
    the Web API response and displayed in the Web UI to indicate the search
    form can not be validated.
    
    Related to T3247

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

Harbormaster completed remote builds in B20799: Diff 19786.Apr 16 2021, 12:02 PM
vlorentz accepted this revision.Apr 19 2021, 1:00 PM
This revision is now accepted and ready to land.Apr 19 2021, 1:00 PM
Closed by commit rDWAPPSf3f1b7c84a73: assets/webapp-utils: Add SWHID validation in search inputs (authored by anlambert). · Explain WhyApr 19 2021, 2:03 PM
This revision was automatically updated to reflect the committed changes.
anlambert added a commit: rDWAPPSf3f1b7c84a73: assets/webapp-utils: Add SWHID validation in search inputs.

Revision Contents
Changeset List

PathSize
assets/
src/
bundles/
browse/
45 lines
webapp/
35 lines
6 lines
cypress/
integration/
46 lines
swh/
web/
templates/
includes/
8 lines
9 lines

Diff 19685

View Options

assets/src/bundles/browse/origin-search.js

Loading...
View Options

assets/src/bundles/webapp/webapp-utils.js

Loading...
View Options

assets/src/bundles/webapp/webapp.css

Loading...
View Options

cypress/integration/origin-search.spec.js

Loading...
View Options

swh/web/templates/includes/origin-search-form.html

Loading...
View Options

swh/web/templates/layout.html

Loading...
About · Developer information · Legal