Named validation errors #502

TRowbotham · 2020-05-08T19:26:09Z

This addresses #406.

By my count, there are 54 different places where a validation error can occur with the majority of them being unique. This adds a table with descriptive error codes for each unique validation error, which also has an associated description of the error.

Would there by value in adding a column indicating whether the validation error can cause the parser to fail?
I am considering adding example input that would trigger each validation error. Is this something you think would have value?
The unexpected-windows-drive-letter-host needs an error description still.
I wasn't sure how best to link the validation errors in each spot, so I just ended up doing <a><unique error name></a> <a>validation error</a> everywhere. I had considered a prefix or suffix of validation-error to every code, but felt it was overly verbose, though I'm not opposed to it if someone feels its more appropriate.
"Error Message" should probably be renamed to "Error Description".
Some of the error descriptions could probably use a little more detail, but I wanted to get this initial draft posted.

Preview | Diff

domenic · 2020-05-08T20:05:36Z

This looks amazing! Giving my opinions on some of your questions, but you should probably wait for @annevk before acting on them:

Would there by value in adding a column indicating whether the validation error can cause the parser to fail?

IMO yes. I'm thinking a "Fatal" column which is either empty or one of the unicode checkmarks.

I am considering adding example input that would trigger each validation error. Is this something you think would have value?

IMO yes. Doing so in a way that makes things non-overwhelming might be a bit tricky. E.g. I think another column would probably not work well. Maybe a second row (making each error code rowspan="2") with class="example", similar to https://html.spec.whatwg.org/#usage-summary-2 ? Or maybe just putting in <p class="example"> wouldn't be so bad.

Relatedly, although I imagine you are following the pattern from HTML, maybe the descriptions could be tightened up by removing "This error occurs when".

I wasn't sure how best to link the validation errors in each spot,

I like what you did. It provides a convenient link to the concept of validation error, which is important as people can be confused by parse error vs. validation error.

annevk

This is great, thanks for putting this together!

Yes to both your questions from me as well. I gave some initial feedback on some of the naming choices.

url.bs

TRowbotham · 2020-05-10T00:11:11Z

Thanks for the feedback. I'll work on adding a new "Fatal" column as domenic suggested and rewording the descriptions to try not to repeat the "This error occurs when...".

I agree that an additional column probably isn't the best approach for the example input. The HTML parse errors table appears to put notes and examples right under the error description text, so I'll experiment with doing that and hopefully things don't get too cluttered.

domenic · 2020-05-11T15:01:01Z

The HTML parse errors table appears to put notes and examples right under the error description text, so I'll experiment with doing that and hopefully things don't get too cluttered.

One thing that will declutter things a bit is adding <p> around each description, which will introduce some nicer margins.

annevk · 2020-05-11T15:16:39Z

@TRowbotham if this needs rebasing at some point due to other ongoing changes I'm happy to do that work for you.

TRowbotham · 2020-05-12T22:10:30Z

I think I broke this while rebasing :(

TRowbotham · 2020-05-15T00:54:46Z

@annevk, I think this is ready now. It just needs a rebase, which I'll leave up to you so I don't botch it again.

annevk · 2020-05-17T06:58:29Z

@TRowbotham how are the rows ordered?

TRowbotham · 2020-05-17T07:27:55Z

They are grouped by section in the spec. I started at section 4.4 URL Parsing and worked my way backwards through the sections.

annevk · 2020-05-18T08:40:39Z

For port-out-of-range a problem is that URL-port string should define the upper limit. If it defines the upper limit, do we still want a separate enum value? I guess we do, but the description would have to change as it would match invalid-port otherwise.

Spotted in #502.

karwa · 2020-08-29T17:25:32Z

I think the missing-credentials error is incorrect. That error occurs if there is an @ but no host. https://@test.com should not cause the parser to fail, but https://username:password@ should (and does, in the Live URL viewer in Safari 13).

Perhaps it should be replaced by something like 'unexpected credentials without host' (similar to 'unexpected port without host')?

…t components from the stored URL string - Corrected a validation error in the host parser - Renamed 'missing-credentials' validation error; it's really an "unexpected credentials without host" error. See: whatwg/url#502 (comment)

Based on the draft whatwg/url#502

annevk · 2023-01-13T17:33:24Z

The IPv4 parser still has two unclassified validation errors.

Otherwise this has been rebased. I adjusted casing to my liking. I addressed @karwa's observation. I want to give it another pass and address those missing cases, but it's getting late.

annevk · 2023-01-17T14:15:23Z

I want #739 to land first and rebase this on top.

I also want to divide the table into 3 row groups. Tentatively titled "Hosts", "URLs", and "Opaque hosts and URLs". And then put the errors in order of appearance.

annevk · 2023-01-20T18:54:49Z

This is now ready for review. All validation errors should be covered. Please don't hold back on nits!

TRowbotham · 2023-01-22T01:35:15Z

Thank you for driving this to completion!

Overall, I think this looks pretty good. I think the IPv4 and IPv6 validation error names could use a normalization pass. Currently, there are a few with similar names, but slightly different word order such as:

too-many-IPv4-parts
IPv6-too-many-pieces
IPv4-in-IPv6-too-many-parts

url.bs

TRowbotham · 2023-01-24T00:10:27Z

I stood up a small demo implementation, similar to the jsdom version, you can use it to quickly check validation errors if that helps with the review.

The git branch of the url parser the demo is using isn't published at the moment, but I can publish the branch if you would find the source code useful. The error highlighting could probably use some refinement, but it should do the job for reporting the validation error.

Edit: fixed the link

domenic

+1 for better consistency in naming the errors, if possible. Some suggestions, none of which I'm 100% confident in:

All the file: URL specific ones could be prefixed with file-.
All the authority/credentails ones could be prefixed with authority-.
Choose between "unexpected" (e.g. unexpected-C0-control-or-space), "invalid" (e.g. invalid-URL-code-point, invalid-scheme-start), and "forbidden" (e.g. forbidden-host-code-point").

I also wonder about redundancy for some of these. Isn't invalid-URL-code-point a superset of unexpected-C0-control-or-space and unexpected-ASCII-tab-or-newline? Similarly isn't unexpected-at-sign a superset of unexpected-credentials-without-host and missing-solidus-before-authority? Maybe the extra specificity is useful, but I'm not so sure...

url.bs

annevk · 2023-01-24T18:14:55Z

I pushed a large number of changes after a fairly lengthy editing session, but it's still not done unfortunately.

Closes whatwg#406.

domenic

LGTM with nits. Very clean.

url.bs

domenic · 2023-02-15T07:01:27Z

url.bs

+    <p class=example id=example-ipv4-in-ipv6-too-many-pieces>"<code>https://[1:1:1:1:1:1:1:127.0.0.1]</code>"
+   <td>Yes
+  <tr>
+   <td><dfn>IPv4-in-IPv6-invalid-code-point</dfn>


Really seems like this should be split into multiple errors, but I guess that could be refactored later.

I think I ended up grouping some obscure ones into this one as there was no great logical separation. And IPv4-in-IPv6 is already pretty obscure so I'm not sure how detailed we really want to go.

annevk · 2023-02-18T15:36:15Z

@TRowbotham did you want to give this a final look before it's merged?

TRowbotham · 2023-02-20T00:22:07Z

LGTM. Thank you for pushing this over the finish line!

Based on the draft whatwg/url#502

annevk reviewed May 9, 2020

View reviewed changes

url.bs Outdated Show resolved Hide resolved

url.bs Outdated Show resolved Hide resolved

url.bs Outdated Show resolved Hide resolved

url.bs Outdated Show resolved Hide resolved

url.bs Outdated Show resolved Hide resolved

url.bs Outdated Show resolved Hide resolved

annevk mentioned this pull request May 13, 2020

Add named validation errors #514

Closed

annevk force-pushed the named-validation-errors branch from e076612 to b22d189 Compare May 14, 2020 04:12

annevk force-pushed the named-validation-errors branch from 85af057 to 0951632 Compare May 17, 2020 06:14

annevk added a commit that referenced this pull request May 18, 2020

Define upper limit of a URL-port string

e540eac

Spotted in #502.

annevk mentioned this pull request May 18, 2020

Define upper limit of a URL-port string #519

Merged

annevk added a commit that referenced this pull request May 18, 2020

Define upper limit of a URL-port string

c1c2133

Spotted in #502.

Base automatically changed from master to main January 15, 2021 07:41

TRowbotham added a commit to TRowbotham/URL-Parser that referenced this pull request Aug 26, 2022

Add basic validation error logging

0ef0c6a

Based on the draft whatwg/url#502

annevk force-pushed the named-validation-errors branch from d56f794 to 35002f3 Compare January 13, 2023 15:31

annevk added clarification Standard could be clearer topic: validation Pertaining to the rules for URL writing and validity (as opposed to parsing) labels Jan 13, 2023

annevk mentioned this pull request Jan 17, 2023

Defered validation error reporting in IPv4 parser #706

Closed

annevk force-pushed the named-validation-errors branch from f63ed02 to 7954d0a Compare January 20, 2023 18:50

annevk requested a review from domenic January 20, 2023 18:54

TRowbotham commented Jan 22, 2023

View reviewed changes

url.bs Outdated Show resolved Hide resolved

TRowbotham commented Jan 22, 2023

View reviewed changes

url.bs Show resolved Hide resolved

annevk reviewed Jan 23, 2023

View reviewed changes

url.bs Outdated Show resolved Hide resolved

annevk reviewed Jan 23, 2023

View reviewed changes

url.bs Outdated Show resolved Hide resolved

domenic reviewed Jan 24, 2023

View reviewed changes

annevk reviewed Jan 24, 2023

View reviewed changes

url.bs Outdated Show resolved Hide resolved

annevk added the do not merge yet Pull request must not be merged per rationale in comment label Jan 24, 2023

TRowbotham and others added 6 commits February 10, 2023 13:43

Add named validation errors

58c15ef

Closes whatwg#406.

put errors in order of appearance (mostly)

cd5f6cf

Update url.bs

8c54f0f

Update url.bs

7cf109b

Update url.bs

b66df55

Update url.bs

96bbbb2

annevk force-pushed the named-validation-errors branch from 6d42298 to 96bbbb2 Compare February 10, 2023 12:43

Update url.bs

0d32eed

annevk removed the do not merge yet Pull request must not be merged per rationale in comment label Feb 10, 2023

annevk requested a review from domenic February 10, 2023 12:49

domenic approved these changes Feb 15, 2023

View reviewed changes

annevk added 2 commits February 17, 2023 11:50

review

7830662

nits

1fddbe3

annevk merged commit d84610f into whatwg:main Feb 20, 2023

TRowbotham added a commit to TRowbotham/URL-Parser that referenced this pull request Dec 14, 2023

Add basic validation error logging

848e2d4

Based on the draft whatwg/url#502

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Named validation errors #502

Named validation errors #502

TRowbotham commented May 8, 2020 •

edited by pr-preview bot

Loading

domenic commented May 8, 2020 •

edited

Loading

annevk left a comment

TRowbotham commented May 10, 2020

domenic commented May 11, 2020

annevk commented May 11, 2020

TRowbotham commented May 12, 2020

TRowbotham commented May 15, 2020

annevk commented May 17, 2020

TRowbotham commented May 17, 2020

annevk commented May 18, 2020

karwa commented Aug 29, 2020

annevk commented Jan 13, 2023

annevk commented Jan 17, 2023

annevk commented Jan 20, 2023

TRowbotham commented Jan 22, 2023

TRowbotham commented Jan 24, 2023 •

edited

Loading

domenic left a comment

annevk commented Jan 24, 2023

domenic left a comment

domenic Feb 15, 2023

annevk Feb 17, 2023

annevk commented Feb 18, 2023

TRowbotham commented Feb 20, 2023

Named validation errors #502

Named validation errors #502

Conversation

TRowbotham commented May 8, 2020 • edited by pr-preview bot Loading

domenic commented May 8, 2020 • edited Loading

annevk left a comment

Choose a reason for hiding this comment

TRowbotham commented May 10, 2020

domenic commented May 11, 2020

annevk commented May 11, 2020

TRowbotham commented May 12, 2020

TRowbotham commented May 15, 2020

annevk commented May 17, 2020

TRowbotham commented May 17, 2020

annevk commented May 18, 2020

karwa commented Aug 29, 2020

annevk commented Jan 13, 2023

annevk commented Jan 17, 2023

annevk commented Jan 20, 2023

TRowbotham commented Jan 22, 2023

TRowbotham commented Jan 24, 2023 • edited Loading

domenic left a comment

Choose a reason for hiding this comment

annevk commented Jan 24, 2023

domenic left a comment

Choose a reason for hiding this comment

domenic Feb 15, 2023

Choose a reason for hiding this comment

annevk Feb 17, 2023

Choose a reason for hiding this comment

annevk commented Feb 18, 2023

TRowbotham commented Feb 20, 2023

TRowbotham commented May 8, 2020 •

edited by pr-preview bot

Loading

domenic commented May 8, 2020 •

edited

Loading

TRowbotham commented Jan 24, 2023 •

edited

Loading