Add rfc3339Duration validation action - #1520
Conversation
WalkthroughA new 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
library/src/actions/isoDuration/isoDuration.test.ts (1)
121-123: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: add
P1WT1Hto the mixed-week rejection cases.The regex routes weeks through a week-only alternative, so combining
Wwith a time component (e.g.,P1WT1H) is also rejected. Adding it here would document that boundary alongside the existingP1W2D/P1Y1Wcases.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@library/src/actions/isoDuration/isoDuration.test.ts` around lines 121 - 123, The mixed-week rejection test in isoDuration.test.ts only covers calendar/mixed date cases, but the week-only parsing path in the isoDuration action also rejects week durations combined with time components. Update the `for mixed week` case in `action`/`expectActionIssue` to include `P1WT1H` alongside the existing invalid inputs so the test documents this boundary in `isoDuration.test.ts`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@library/src/actions/isoDuration/isoDuration.test.ts`:
- Around line 121-123: The mixed-week rejection test in isoDuration.test.ts only
covers calendar/mixed date cases, but the week-only parsing path in the
isoDuration action also rejects week durations combined with time components.
Update the `for mixed week` case in `action`/`expectActionIssue` to include
`P1WT1H` alongside the existing invalid inputs so the test documents this
boundary in `isoDuration.test.ts`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8e705011-b2c6-4937-9fa2-a7720114077e
📒 Files selected for processing (6)
library/src/actions/index.tslibrary/src/actions/isoDuration/index.tslibrary/src/actions/isoDuration/isoDuration.test-d.tslibrary/src/actions/isoDuration/isoDuration.test.tslibrary/src/actions/isoDuration/isoDuration.tslibrary/src/regex.ts
There was a problem hiding this comment.
1 issue found across 6 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| }); | ||
|
|
||
| test('for fractional values', () => { | ||
| expectActionIssue(action, baseIssue, ['P0.5Y', 'PT0.5S', 'PT1.5H']); |
There was a problem hiding this comment.
There was a problem hiding this comment.
Good catch, thanks. I updated the regex to accept a decimal fraction with a full stop or a comma on a component, so P0.5Y, PT1.5H and P0,5Y are now valid and moved into the passing cases.
|
Several important things: Don't try to invent regular expressions yourself. Don't use AI for generation. You need to use something proven. It's probably impossible to choose a regular expression that covers all the specification's cases. In that case, you need to choose the most practical one and explain why it's better. And describe the limitations in the documentation. By the way, the method's name will also depend on this. You need to leave room for future expansion, should anyone need it. rfc3339Duration? xmlSchemaDuration? For example, there's W3C XML Schema xs:duration https://www.w3.org/TR/xmlschema11-2/#regexs const duration =
/^-?P((([0-9]+Y([0-9]+M)?([0-9]+D)?|([0-9]+M)([0-9]+D)?|([0-9]+D))(T(([0-9]+H)([0-9]+M)?([0-9]+(\.[0-9]+)?S)?|([0-9]+M)([0-9]+(\.[0-9 ]+)?S)?|([0-9]+(\.[0-9]+)?S)))?)|(T(([0-9]+H)([0-9]+M)?([0-9]+(\.[0-9]+)?S)?|([0-9]+M)([0-9]+(\.[0-9]+)?S)?|([0-9]+(\.[0-9]+)?S))))$/;Key differences from the OpenAPI/RFC3339 profile: W3C XSD allows negative durations like -P1D and fractional seconds like PT1.5S, but does not support the week-only P2W form. The W3C form is based on PnYnMnDTnHnMnS, not the full ISO 8601 duration zoo. There's also a library with 100 million downloads per month. const duration = /^P(?!$)((\d+Y)?(\d+M)?(\d+D)?(T(?=\d)(\d+H)?(\d+M)?(\d+S)?)?|(\d+W)?)$/Its purpose: it's not a full ISO 8601 duration, but a fairly narrow and practical profile, close to what's expected in JSON Schema/OpenAPI tools. It supports weeks, but not negative weeks, and it doesn't support fractional values separated by periods or commas. Probably for our case this is not necessary and I shouldn’t have left that comment above. In short, there are a lot of thoughts. A lot of decisions need to be made. What do you think about this? Would you like to see your vision? |
Validates ISO 8601 duration strings such as P1Y2M3DT4H5M6S, PT5H30M and P1W, for OpenAPI format: duration compatibility. Uses the duration regex from ajv-formats, which covers the practical profile used by JSON Schema and OpenAPI tooling. Negative and fractional values are not accepted. Follows the existing iso* action pattern with a new regex, action, and tests.
3dae7e9 to
f53a188
Compare
|
Thanks, this is really helpful. I hadn't come across ajv-formats or the W3C XSD regex before, so those links cleared up a lot. Since #1497 is about OpenAPI So it accepts the week form ( On the name, I kept |
The regex matches the RFC 3339 Appendix A duration profile, so name the action accordingly. This leaves room for other duration profiles later.
|
Thank you! I will try to take a closer look before v1.5. |
|
Hi @NoahStarkenburg — thanks for the work here, and for being responsive to feedback so far. I want to lock in the direction for this PR so we can get it merged into v1.5. A few things needed before we merge: 1. Document why fractional/negative values are rejected Right now the code doesn't explain why the regex rejects fractional (
2. Naming: Looking at our existing 3. Test coverage against the ajv-formats reference behavior Please add a few explicit test cases that document the intentional divergence from "full" ISO 8601 / strict RFC 3339 ABNF, e.g.:
This makes the design decision self-documenting for future readers/reviewers. 4. Website docs This PR is missing the usual API reference additions (properties.ts + index.mdx under Once these are in, I think this is ready to merge. Thanks again for sticking with it through the back-and-forth! |
commit: |
Closes #1497
Adds an
isoDurationaction that validates ISO 8601 / RFC 3339 duration strings, for OpenAPIformat: durationcompatibility.Valid:
P1D,P1W,PT5H30M,P1Y2M3DT4H5M6S. Rejects emptyP/PT, fractional values, out-of-order units, the mixed week form, and lowercase units.It follows the existing
iso*action pattern (likeisoTimeSecond): a new regex inregex.ts, the action, runtime tests, and type tests. The regex is taken from ajv-formats, the duration profile that JSON Schema and OpenAPI tooling expect. Happy to adjust the name or strictness if you prefer.Summary by CodeRabbit
New Features
Tests