Skip to content

fix: registerAjvFormats() never registered any format - #295

Merged
ilteoood merged 5 commits into
fastify:mainfrom
Puppo:plan-c8-coverage-100
Oct 11, 2026
Merged

ilteoood merged 5 commits into
fastify:mainfrom
Puppo:plan-c8-coverage-100

Conversation

@Puppo

@Puppo Puppo commented Sep 1, 2026 •

Copy link
Copy Markdown
Member

registerAjvFormats() previously iterated the ajv-formats plugin object's properties instead of its format definitions, so it registered nothing. Existing validation tests passed because TypeBox already provides built-in formats.

This fixes the helper to register 22 string formats using ajv-formats' full validation mode. It excludes int32, int64, float, and double, whose numeric validators do not belong in TypeBox's string-format registry. The existing defensive default-export fallback is preserved for CJS, ESM, and bundler interoperability, with a coverage exclusion limited to that fallback. Validator normalization handles the real format definitions without the speculative error branch or its mock-only test. The password and binary formats share one always-true validator across registrations.

Calling the helper replaces existing validators with the same names. Observable changes include rejecting http://localhost:3000 as a url and user@localhost as an email, while accepting a space separator in date-time and the urn:uuid: prefix in uuid. The README now documents these changes and corrects the helper import and the description of TypeBox's built-in formats.

The complete runtime suite is shared between separate CJS and ESM entrypoints. c8 includes both builds and unexecuted JavaScript files, and requires 100% source coverage. Build source maps allow coverage to measure the original TypeScript rather than compiler-generated interoperability helpers. Unrelated formatting and dependency-order changes have been reverted.

Validation

  • CI passed: all 12 combinations of Node 20, 22, 24, and 26 on Linux, macOS, and Windows, plus lint, license, and dependency checks.
  • npm run lint
  • npm test: both builds, package checks, 38 runtime tests, 20 type assertions, and 100% statements, branches, functions, and lines.
  • Regression tests verify the exact 22 registered names, repeat registration, custom-format preservation, real validator definition shapes, and positive/negative validation cases in both builds.
  • Deliberately disabling registration or inverting the boolean-format branch causes the registration test to fail in both builds; generated artifacts were restored afterward.

Proposed release note

Fix registerAjvFormats() so it registers AJV string-format validators. Calling the helper now overrides matching TypeBox defaults and can change accepted inputs, including URL, email, date-time, and UUID validation. Numeric formats are excluded. Recommend a minor release because the previously ineffective helper now changes runtime validation behavior.

@Puppo
Puppo force-pushed the plan-c8-coverage-100 branch 2 times, most recently from d60d57a to 0c16fa3 Compare September 3, 2026 05:23
Comment thread index.ts
@Puppo
Puppo force-pushed the plan-c8-coverage-100 branch from 2bc8113 to 0c16fa3 Compare September 3, 2026 05:35
@Puppo
Puppo marked this pull request as ready for review September 3, 2026 05:37
@Puppo
Puppo force-pushed the plan-c8-coverage-100 branch from 0c16fa3 to 0adb0d7 Compare September 23, 2026 10:26
@gurgunday
gurgunday requested review from a team and Tony133 September 23, 2026 20:53

@Tony133 Tony133 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While checking this I noticed that on main registerAjvFormats() doesn't actually register anything: rawFormats is the plugin function, so Object.entries() only gives get and default and Format.Set is never reached. The tests pass because TypeBox has its own built-ins. So this PR is really a fix, and it changes behavior: ajv-formats now overrides 17 of TypeBox's formats and adds 9. For example url starts rejecting http://localhost:3000 and email rejects user@localhost, while date-time starts accepting a space separator and uuid the urn:uuid: prefix.

Keep in mind this package ships both CJS and ESM builds, and the ajv-formats interop is the part that behaves differently between the two, so that's where I'd be most careful.

A few things before merging:

  • rename to something like fix: registerAjvFormats() never registered any format and mention the behavior change in the description, it should show up in the changelog
  • remove int32, int64, float, double: they're number formats, TypeBox applies them to strings, so int32/int64 fail on everything and float/double pass everything
  • have the CJS test actually call registerAjvFormats(), see inline
  • revert the formatting-only changes, see inline

Also worth noting: 100% wasn't reachable on main without touching the code, Format.Set was dead. So the fix was necessary, but the ESM test migration, the formatting and the extra branches in getFormatValidator (which then need the mock test) weren't.

I'd release this as a minor.

Comment thread index.ts Outdated
Comment thread index.ts Outdated
Comment thread test/index.mjs Outdated
Comment thread index.ts Outdated
Comment thread index.ts Outdated
Comment thread index.ts Outdated
Comment thread package.json Outdated
@Puppo Puppo changed the title feat: add 100% c8 coverage fix: registerAjvFormats() never registered any format Oct 10, 2026
@Puppo
Puppo requested review from Eomm and Tony133 October 10, 2026 11:40
@Puppo

Puppo commented Oct 10, 2026

Copy link
Copy Markdown
Member Author

While checking this I noticed that on main registerAjvFormats() doesn't actually register anything: rawFormats is the plugin function, so Object.entries() only gives get and default and Format.Set is never reached. The tests pass because TypeBox has its own built-ins. So this PR is really a fix, and it changes behavior: ajv-formats now overrides 17 of TypeBox's formats and adds 9. For example url starts rejecting http://localhost:3000 and email rejects user@localhost, while date-time starts accepting a space separator and uuid the urn:uuid: prefix.

Keep in mind this package ships both CJS and ESM builds, and the ajv-formats interop is the part that behaves differently between the two, so that's where I'd be most careful.

A few things before merging:

  • rename to something like fix: registerAjvFormats() never registered any format and mention the behavior change in the description, it should show up in the changelog
  • remove int32, int64, float, double: they're number formats, TypeBox applies them to strings, so int32/int64 fail on everything and float/double pass everything
  • have the CJS test actually call registerAjvFormats(), see inline
  • revert the formatting-only changes, see inline

Also worth noting: 100% wasn't reachable on main without touching the code, Format.Set was dead. So the fix was necessary, but the ESM test migration, the formatting and the extra branches in getFormatValidator (which then need the mock test) weren't.

I'd release this as a minor.

Hey @Tony133 let me know what do you think now 🙏

@Puppo
Puppo requested a review from ilteoood October 10, 2026 11:41
Comment thread index.ts Outdated
@ilteoood
ilteoood self-requested a review October 11, 2026 06:31

@Tony133 Tony133 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Once merged, let's release this as a minor 🚀

@gurgunday gurgunday left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@ilteoood
ilteoood merged commit 374aeb2 into fastify:main Oct 11, 2026
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants