fix: avoid stray comma when required is set but properties is empty#841
Open
imalbi wants to merge 2 commits into
Open
fix: avoid stray comma when required is set but properties is empty#841imalbi wants to merge 2 commits into
imalbi wants to merge 2 commits into
Conversation
The skip-leading-comma optimization assumes a declared property will
anchor the comma logic. With an empty properties map (e.g. a record-only
schema produced by z.toJSONSchema for z.record(enum, V)), the loop never
runs and the additionalProperties branch ends up writing a separator
before its first entry, producing '{ ,"k":v,... }' which fails JSON.parse.
Narrows the optimization condition to require both arrays to be non-empty;
otherwise falls back to the existing runtime addComma path.
Signed-off-by: Alberto Cerqua <106998309+imalbi@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes an invalid-JSON edge case in the object serializer where a schema can emit a leading comma when required is set but there are no declared properties, affecting record-style schemas using additionalProperties.
Changes:
- Tighten the “skip first comma” optimization in
buildInnerObjectto avoid unconditional comma insertion whenpropertiesis empty. - Add a regression test covering
required+additionalPropertieswith no declaredpropertieson the nested object.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
index.js |
Adjusts comma-optimization guard in object serialization to prevent leading commas in an edge case. |
test/additionalProperties.test.js |
Adds a regression test ensuring the affected schema shape produces valid JSON. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Raised by auto-review on the PR: the previous guard
(`requiredProperties.length > 0 && propertiesKeys.length > 0`) is still
unsafe when `required` only names keys that are not declared in
`properties`. After sorting, `propertiesKeys[0]` is then not required,
so the "first declared property anchors the comma" premise does not
hold and the additionalProperties branch can still emit a stray
leading comma.
Tighten the condition to `propertiesKeys.length > 0 &&
requiredProperties.includes(propertiesKeys[0])`. The sort already
moves required-and-declared keys to the front, so this check captures
exactly the case where the optimization is safe. Falls through to the
runtime addComma path otherwise.
Regression test added for the new shape: `properties: { num }`,
`required: ['str']`, `additionalProperties: true`. Full suite green
(475/475). Re-ran benchmark 3 vs 3 against the previous fix, deltas
within inter-run noise, so by transitivity still performance-neutral
vs main.
Signed-off-by: Alberto Cerqua <106998309+imalbi@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A response schema with
requiredset but nopropertiesmap (e.g. whatz.toJSONSchemaproduces forz.record(enum, V)) serializes with a stray leading comma:HTTP 200 from Fastify, but
JSON.parsefails client-side.Root cause
The optimization at
index.js:389skips the runtime comma check whenrequiredProperties.length > 0, on the assumption that the first declared property will anchor the comma logic. Whenpropertiesis empty (or absent), the loop body never runs andaddCommais set to unconditional immediately after, so theadditionalPropertiesbranch atindex.js:484emits a separator before its first entry.Fix
Tighten the condition to
propertiesKeys.length > 0 && requiredProperties.includes(propertiesKeys[0]). The sort just above (index.js:367-373) moves required-and-declared keys to the front, so this captures exactly when the "first declared property anchors the comma" premise actually holds. It falls through to the existing runtimeaddCommaflag path in three cases that the old condition handled incorrectly:propertiesis empty (record-style schema, onlyadditionalProperties)requiredonly names keys that are not inpropertiesrequiredPropertiesis empty — already handled beforeNo behaviour change for any schema where at least one declared property is required.
Tests
Three regression tests covering the canonical bug shapes:
test/additionalProperties.test.js:required + additionalPropertieswith nopropertiesmap (the original z.record-style trigger).test/additionalProperties.test.js:requiredkey not inproperties, withadditionalProperties: true.test/patternProperties.test.js: same shape withpatternProperties(symmetric branch atindex.js:484).Each test fails on the respective baseline (
mainfor #1, the first proposed fix for #2 and #3) and passes after this patch. Full suite: 476/476 green.Benchmark
Ran
npm run benchmark3 times onmainand 3 times on the fix branch. All scenario ranges overlap; absolute mean deltas ≤ 1.1%, within natural inter-run variance (±0.7% to ±2.0%). The modified branch is not exercised by any standard scenario, so no runtime perf change is expected by construction.After tightening the guard further (to also handle
requiredkeys not inproperties), re-ran 3 vs 3 between the first fix and the tightened version. Absolute mean deltas ≤ 1.7%, still within inter-run variance. By transitivity, the tightened fix is also performance-neutral vsmain.Related
requiredsupport entirely. This fix is a smaller targeted patch in the same area while that broader change is on hold.Checklist
npm run test && npm run benchmark --if-presentand the Code of conduct