Remove ajv-formats-draft2019 - #1411
Conversation
…or idn-email format (leveraging ajv-formats) Signed-off-by: Pasi Eronen <pe@iki.fi>
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Documentation build overview
|
jkowalleck
left a comment
jkowalleck
left a comment
There was a problem hiding this comment.
Thank you so much for taking care of this topic.
see my remarks below
| ajv.addFormat('iri-reference', true) | ||
|
|
||
| // add idn-email format (was previously provided by ajv-formats-draft2019) | ||
| const emailValidator = ajv.compile({type: 'string', format: 'email'}) |
There was a problem hiding this comment.
since using our own implementations, we critically need tests for this!
please add test cases:
- positive cases - like "this is expected to be treated as a valid 'idn-email'"
- negative cases - like "this is expected to be treated as an invalid 'idn-email'"
- edge cases - like UTF8 characters, quoted emails (
"foo@bar"@baz) and waht not in the
There was a problem hiding this comment.
For anything containing only ASCII characters, we just reuse the implementation from ajv-formats (whose definition of what exactly is a valid email might not be identical to somebody else's definition - I don't think we want to write detailed tests for that).
But I agree we need something here to show the code works. I took the test cases I found in https://github.com/json-schema-org/JSON-Schema-Test-Suite/blob/e82bfdfa59c63cc74175d35fac81bb95e61db24b/tests/draft2019-09/optional/format/email.json and https://github.com/json-schema-org/JSON-Schema-Test-Suite/blob/e82bfdfa59c63cc74175d35fac81bb95e61db24b/tests/draft2019-09/optional/format/idn-email.json,
would those be enough?
There was a problem hiding this comment.
could you also add edge cases?
Things i can imagine:
- empty email address (empty string) - shall be a negative
- to be continued
here are other cases that might be woven in:
see also the test cases of the already used library for reference:
There was a problem hiding this comment.
Sure! Especially the empty string is a good idea, added couple more at the same time (amended the same commit).
I also noticed that not even ajv-formats and ajv-formats-draft2019 agree on every input. For example, "John Doe"@example.com is valid according to the latter but not the former. That said, I don't expect this to be a problem (and we probably don't want too many tests for rare edge cases, since ajv-formats itself may also change over time).
Signed-off-by: Pasi Eronen <pe@iki.fi>
3779054 to
6f127bd
Compare
CAOShurong
left a comment
CAOShurong
left a comment
There was a problem hiding this comment.
The dependency removal and the existing test suite pass on exact head 1fece39dd1b9bd367a82daaa0c28ccc794e39454, but this head changes an accepted idn-email case that falls directly within the unresolved edge-case discussion.
Using the real CycloneDX 1.7 validator, I reproduced this boundary on the default-branch base c3d8535 and the PR head:
"John Doe"@example.com: accepted by the current default branch, rejected by this PR;用户@例子.广告: accepted by both;johndoe: rejected by both.
The dependency being replaced also explicitly tests "John Doe"@example.com as valid (ajv-formats-draft2019/index.test.js), and the discrepancy is already acknowledged in the current unresolved thread. This is therefore a concrete compatibility change, not a hypothetical difference between validators.
Please either preserve the existing quoted-local-part behavior and add it as a regression test, or explicitly document and justify the compatibility break before merging. Validation on this exact head included 18 focused JSON-validator tests, the full 4,180-test Node suite, TypeScript, ESLint, and Knip; all passed, so the request is limited to the uncovered compatibility boundary above.
Review assisted by OpenAI Codex; I independently checked the exact head and reproduced the three validator cases above.
3 participants
Description
Replace unmaintained dependency ajv-formats-draft2019 with our own implementation.
Continues work started by grease-work-23 in #1226. See also discussion in #1223 and
CycloneDX/cyclonedx-webpack-plugin#1348
The only piece we still used from ajv-formats-draft2019 is "idn-email". I tried to keep the code simple, and re-use ajv-formats (which we anyway depend on) as much as possible.
It might be possible to implement "iri-reference" in similar way (convert to URI, and use "uri-reference" from ajv-fomats), but I'm not sure how useful that would be: almost any string is a valid "uri-reference", so it wouldn't probably catch any problems anyway.
Resolves or fixes issue: #1226
supersedes #1226
AI Tool Disclosure
Affirmation