From b32d45689cb7f1666f838063c57370c9c3f4399f Mon Sep 17 00:00:00 2001 From: Alberto Cerqua <106998309+imalbi@users.noreply.github.com> Date: Tue, 19 May 2026 10:52:32 +0200 Subject: [PATCH 1/2] fix: avoid stray comma when required is set but properties is empty 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> --- index.js | 13 ++++++++++--- test/additionalProperties.test.js | 26 ++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 3 deletions(-) diff --git a/index.js b/index.js index 96ca8ff0..e736e2cd 100644 --- a/index.js +++ b/index.js @@ -386,9 +386,16 @@ function buildInnerObject (context, location, objVar) { const localUid = context.uid++ let addComma = '' - if (requiredProperties.length > 0) { - // If we have required properties, we know that at least one property will be serialized. - // We can avoid the runtime check for the comma. + if (requiredProperties.length > 0 && propertiesKeys.length > 0) { + // If we have required properties AND declared properties, we know that + // at least one declared property will be serialized first, so we can + // avoid the runtime check for the comma. + // + // When `required` is set but `properties` is empty (e.g. a record-style + // schema with `additionalProperties` only), we fall through to the + // generic branch below which uses a runtime `addComma` flag — otherwise + // the additionalProperties serializer would write a separator with no + // preceding property, producing `{ ,"k":v,... }`. // The first property is required, so we don't need a comma. // For the subsequent properties, we can blindly add a comma. diff --git a/test/additionalProperties.test.js b/test/additionalProperties.test.js index a42e176b..06a01ac0 100644 --- a/test/additionalProperties.test.js +++ b/test/additionalProperties.test.js @@ -330,3 +330,29 @@ test('function and symbol references are not serialized as undefined', (t) => { const obj = { str: 'x', test: 'test', meth: () => 'x', sym: Symbol('x') } t.assert.equal(stringify(obj), '{"str":"x","test":"test"}') }) + +test('required + additionalProperties without declared properties produces valid JSON', (t) => { + // Regression: when `required` is non-empty but `properties` is absent + // (e.g. a record-style schema with only `additionalProperties`), the + // serializer used to emit a stray leading comma: + // {"obj":{,"a":1,"b":2}} + // because the "skip first comma" optimization assumed a declared + // property would anchor it. With no `properties`, the additionalProperties + // branch would write the separator before its first entry. + t.plan(2) + const stringify = build({ + type: 'object', + properties: { + obj: { + type: 'object', + propertyNames: { type: 'string', enum: ['a', 'b'] }, + additionalProperties: { type: 'number' }, + required: ['a', 'b'] + } + } + }) + + const out = stringify({ obj: { a: 1, b: 2 } }) + t.assert.equal(out, '{"obj":{"a":1,"b":2}}') + t.assert.deepStrictEqual(JSON.parse(out), { obj: { a: 1, b: 2 } }) +}) From 2d1143ffa81050aa9695b814fba18fcf1e647e3c Mon Sep 17 00:00:00 2001 From: Alberto Cerqua <106998309+imalbi@users.noreply.github.com> Date: Tue, 19 May 2026 12:11:46 +0200 Subject: [PATCH 2/2] fix: tighten optimization guard to also cover required-not-in-properties 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> --- index.js | 26 ++++++++++++++++---------- test/additionalProperties.test.js | 21 +++++++++++++++++++++ test/patternProperties.test.js | 24 ++++++++++++++++++++++++ 3 files changed, 61 insertions(+), 10 deletions(-) diff --git a/index.js b/index.js index e736e2cd..3789dd15 100644 --- a/index.js +++ b/index.js @@ -386,16 +386,22 @@ function buildInnerObject (context, location, objVar) { const localUid = context.uid++ let addComma = '' - if (requiredProperties.length > 0 && propertiesKeys.length > 0) { - // If we have required properties AND declared properties, we know that - // at least one declared property will be serialized first, so we can - // avoid the runtime check for the comma. - // - // When `required` is set but `properties` is empty (e.g. a record-style - // schema with `additionalProperties` only), we fall through to the - // generic branch below which uses a runtime `addComma` flag — otherwise - // the additionalProperties serializer would write a separator with no - // preceding property, producing `{ ,"k":v,... }`. + // The skip-leading-comma optimization is only safe when at least one + // declared property is also required: that guarantees the first emitted + // entry has no preceding separator. `propertiesKeys` is sorted with + // required keys first, so `requiredProperties.includes(propertiesKeys[0])` + // is the cheapest way to express the condition. + // + // Falls through to the generic runtime-`addComma` branch when: + // * `requiredProperties` is empty (no anchor needed), or + // * `propertiesKeys` is empty (record-style schema with only + // `additionalProperties`), or + // * `required` only names keys that are not in `properties` (so no + // declared property is guaranteed to emit first). + // Otherwise the additionalProperties / patternProperties serializer + // would write a separator with no preceding property, producing + // `{ ,"k":v,... }`. + if (propertiesKeys.length > 0 && requiredProperties.includes(propertiesKeys[0])) { // The first property is required, so we don't need a comma. // For the subsequent properties, we can blindly add a comma. diff --git a/test/additionalProperties.test.js b/test/additionalProperties.test.js index 06a01ac0..4a44a8c3 100644 --- a/test/additionalProperties.test.js +++ b/test/additionalProperties.test.js @@ -356,3 +356,24 @@ test('required + additionalProperties without declared properties produces valid t.assert.equal(out, '{"obj":{"a":1,"b":2}}') t.assert.deepStrictEqual(JSON.parse(out), { obj: { a: 1, b: 2 } }) }) + +test('required key not in properties + additionalProperties produces valid JSON', (t) => { + // Regression: declared `properties` is non-empty but `required` only lists + // keys that are not in `properties`. After sorting, propertiesKeys[0] is + // not required, so the "first declared property anchors the comma" premise + // does not hold. The serializer used to emit '{,"str":"x"}' for input + // missing the (non-required) declared `num`. + t.plan(2) + const stringify = build({ + type: 'object', + properties: { + num: { type: 'number' } + }, + additionalProperties: true, + required: ['str'] + }) + + const out = stringify({ str: 'x' }) + t.assert.equal(out, '{"str":"x"}') + t.assert.deepStrictEqual(JSON.parse(out), { str: 'x' }) +}) diff --git a/test/patternProperties.test.js b/test/patternProperties.test.js index d80a7f1b..13633d4f 100644 --- a/test/patternProperties.test.js +++ b/test/patternProperties.test.js @@ -166,3 +166,27 @@ test('patternProperties - fail on invalid regex, handled by ajv', (t) => { } }), new Error('schema is invalid: data/patternProperties must match format "regex"')) }) + +test('required key not in properties + patternProperties produces valid JSON', (t) => { + // Regression: symmetric case to the additionalProperties variant. `required` + // names a key not in `properties`, so after sorting propertiesKeys[0] is not + // required and the "first declared property anchors the comma" premise does + // not hold. The patternProperties branch shares the same code path, so the + // same stray-leading-comma bug would surface here without the tightened + // guard in `index.js`. + t.plan(2) + const stringify = build({ + type: 'object', + properties: { + num: { type: 'number' } + }, + patternProperties: { + '^s_': { type: 'string' } + }, + required: ['s_x'] + }) + + const out = stringify({ s_x: 'x' }) + t.assert.equal(out, '{"s_x":"x"}') + t.assert.deepStrictEqual(JSON.parse(out), { s_x: 'x' }) +})