fix: update internal schema merging #665

ivan-tymoshenko · 2023-11-25T14:04:51Z

This PR adds a proper schema merging for different cases:

root schema + oneOf/anyOf subschema
root schema + all allOf subschemas
root schema + then schema; root schema + else schema

gurgunday · 2023-11-25T14:38:15Z

index.js

-      }
-      Object.assign(mergedSchema.properties, allOfSchema.properties)
-    }
+  for (let location of locations) {


Won't a simple for loop be faster than iterators?

It was the case last time I checked

This kind of change doesn't create any perf difference unless it places in a very very high-loaded bottleneck (not this library). This library could be optimized by reducing the amount of time it clones the schemas.

Feel free to run benchmarks and prove me wrong.

I see, no problem I'll take your word 😄

index.js

mcollina

lgtm

gurgunday · 2023-11-25T14:52:09Z

index.js

+  }
+  for (const subSchema of Object.values(schema)) {
+    if (
+      typeof subSchema === 'object' &&


Not familiar so just asking, we are sure there can't be a null value right?

Suggested change

typeof subSchema === 'object' &&

subSchema != null &&

It shouldn't be, but this is a good catch. Will add a check.

ivan-tymoshenko · 2023-12-11T20:29:28Z

Need a proper review for this one. This is more difficult than it might seem.

mcollina · 2023-12-12T10:21:32Z

package.json

    "ajv": "^8.10.0",
    "ajv-formats": "^2.1.1",
    "fast-deep-equal": "^3.1.3",
    "fast-uri": "^2.1.0",
    "rfdc": "^1.2.0",
-    "json-schema-ref-resolver": "^1.0.1"
+    "json-schema-ref-resolver": "^1.0.1",
+    "json-schema-merge-allof": "^0.8.1"


The use of https://www.npmjs.com/package/json-schema-merge-allof does not inspire me a lot of faith as it seems an unmaintained module. Is there an alternative? Can we fork or embed its code in here?

Unfortunatelly this is the best solution I could find. We can write our own solution, but I don't have time for this right now. Anyway it should be better that our currect merge logic.

ivan-tymoshenko · 2024-01-16T12:51:46Z

Waiting for https://github.com/fastify/merge-json-schemas release.

Uzlopak · 2024-01-17T18:38:50Z

test/allof.test.js


  const schema = {
    allOf: [
      { type: 'string' },
      { type: 'number' }
    ]
  }
-  t.throws(() => build(schema), new Error('allOf schemas have different type values'))
+  try {
+    build(schema)


Suggested change

build(schema)

build(schema)

t.fail('Should have thrown')

Uzlopak · 2024-01-17T18:39:09Z

test/allof.test.js


  const schema = {
    allOf: [
      { format: 'date' },
      { format: 'time' }
    ]
  }
-  t.throws(() => build(schema), new Error('allOf schemas have different format values'))
+  try {
+    build(schema)


Suggested change

build(schema)

build(schema)

t.fail('Should have thrown')

Uzlopak · 2024-01-17T18:42:18Z

index.js

-    }
+  if (
+    schema.$id !== undefined &&
+    schema.$id.charAt(0) !== '#'


What if schema.$id is an empty string?

It shouldn't be an empty string. If user explicitly add a schema $id field with empty string value, it's kind of his problem. Anyway, this code will prefix all nested schemas' $refs with an empty string then.

Uzlopak · 2024-01-17T18:44:08Z

index.js

-    mergeAllOfSchema(context, location, schema, clone(schema))
-    schema = location.schema
+  code += `
+    else throw new TypeError(\`The value of '${schemaRef}' does not match schema definition.\`)


Suggested change

else throw new TypeError(\`The value of '${schemaRef}' does not match schema definition.\`)

else throw new TypeError(\`The value of '${schemaRef}' does not match the schema definition.\`)

?

I didn't add this error. If you want to update the error message for this error, you can create another PR.

Uzlopak · 2024-01-17T18:45:31Z

test/if-then-else.test.js

 })
-const noElseGreetingOutput = JSON.stringify({
-  kind: 'greeting',
-  foo: 'FOO',
-  bar: 42,
-  hi: 'HI',
-  hello: 45,
-  a: 'A',
-  b: 35
-})
+const noElseGreetingOutput = JSON.stringify({})


Why did you change this?

Because the test was wrong.

Here is a schema:

fast-json-stringify/test/if-then-else.test.js

Line 167 in 69a34a2

const noElseSchema = {

Here is an input data:

fast-json-stringify/test/if-then-else.test.js

Line 209 in 69a34a2

const greetingInput = {

As you can see the if condition doesn't fullfil as greetingInput.kind equal greeting and not a foobar, so it should use else schema instead of then. And as else schema is missing the result is an empty object.

Uzlopak · 2024-01-17T18:46:35Z

test/allof.test.js

 })

-test('allOf: throw Error if nullable mismatch /1', (t) => {
+test('recursive nested allOfs', (t) => {


Why did you change these two allOf tests?

I didn't change the test here. I removed allOf: throw Error if nullable mismatch /1 test and added a couple of new tests. I dropped the allOf: throw Error if nullable mismatch /1 because now the merging algoritm is more accurate and can resolve the situation when two schemas has different nullable keyword values.

If one schema has nullable = true and another one has nullable = false the merged schema will have nullable = false. Now fast-json-stringify just throws an error in this case.

Uzlopak · 2024-01-17T18:53:42Z

Tbh. This is a quite complex PR. I dont think that I could approve it with the full confidence that I understood everything. I read over it, and tbh. I did not make a deep dive. Just a raw review, if everything makes still kind of sense.

Also I dont want to stall to merge this, just because I am not confident enough to approve it.

For my approval I just need the confidence, that we didnt break the api contract and if we break it, it should be an informed decision.As we have a good code coverage in this project, it would enough for me, if no old tests were chaged, and if they were changed, that they dont reflect a significant behavioural change.

I would like to know why some of the tests got changed/removed in the allof.test.js, as it seems the only place where the behaviour changed significantly. If those questions are answered, I am happy to approve it.

mcollina · 2024-01-18T11:19:46Z

@Uzlopak let me know if you have any further concerns, otherwise I'd like to ship this soon.

Uzlopak

Thank you for the explaination.
LGTM

Pezmc · 2024-01-30T16:30:02Z

@Uzlopak @mcollina I'm currently hitting this bug (oneOf results in a trailing comma) and can confirm that this PR resolves it. The latest release of fast-json-strinify was 5.10.0 three weeks ago and does not include this fix, how can I help cut a new release?

Signed-off-by: Matteo Collina <hello@matteocollina.com>

This reverts commit d95ef87.

This reverts commit 9dd8cb6.

gurgunday reviewed Nov 25, 2023

View reviewed changes

index.js Outdated Show resolved Hide resolved

mcollina approved these changes Nov 25, 2023

View reviewed changes

ivan-tymoshenko marked this pull request as draft November 25, 2023 14:42

gurgunday reviewed Nov 25, 2023

View reviewed changes

ivan-tymoshenko added 2 commits December 6, 2023 00:13

feat: merge schemas

672d897

fix: use proper schema merge for all cases

fe65ff4

ivan-tymoshenko force-pushed the fix-merge-schema-ref-resolving branch from 0d36211 to fe65ff4 Compare December 11, 2023 20:22

This was linked to issues Dec 11, 2023

Recursive validation schema compiles but fails at runtime #648

Closed

Schema serialization of anyOf/oneOf properties are parsed incorrectly #642

Closed

ivan-tymoshenko marked this pull request as ready for review December 11, 2023 20:25

ivan-tymoshenko requested review from mcollina, gurgunday, Uzlopak, Eomm and climba03003 December 11, 2023 20:29

ivan-tymoshenko changed the title ~~fix: update merged schema local refs~~ fix: update internal schema merging Dec 11, 2023

ivan-tymoshenko added 2 commits December 11, 2023 21:35

update

b7afc84

update

cd77a40

mcollina reviewed Dec 12, 2023

View reviewed changes

ivan-tymoshenko mentioned this pull request Jan 15, 2024

feat: add mergeSchemas function fastify/merge-json-schemas#1

Merged

deps: use merge-json-schema package

cfdb917

ivan-tymoshenko marked this pull request as draft January 16, 2024 12:51

deps: use @fastify/merge-json-schemas package

69a34a2

ivan-tymoshenko marked this pull request as ready for review January 17, 2024 14:39

ivan-tymoshenko mentioned this pull request Jan 17, 2024

fix: anyOf produces invalid json due to extra comma #651

Closed

4 tasks

Uzlopak reviewed Jan 17, 2024

View reviewed changes

test: update tests

99ba443

Uzlopak approved these changes Jan 18, 2024

View reviewed changes

mcollina merged commit d95ef87 into master Jan 18, 2024
21 checks passed

Uzlopak deleted the fix-merge-schema-ref-resolving branch January 18, 2024 15:06

ivan-tymoshenko referenced this pull request Feb 1, 2024

Bumped v5.11.0

3dc8dca

Signed-off-by: Matteo Collina <hello@matteocollina.com>

mcollina added a commit that referenced this pull request Feb 1, 2024

Revert "fix: update internal schema merging (#665)"

9dd8cb6

This reverts commit d95ef87.

ivan-tymoshenko added a commit that referenced this pull request Feb 4, 2024

Revert "Revert "fix: update internal schema merging (#665)""

4fa297b

This reverts commit 9dd8cb6.

mcollina pushed a commit that referenced this pull request Feb 5, 2024

Revert "Revert "fix: update internal schema merging (#665)""

35346a7

This reverts commit 9dd8cb6.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: update internal schema merging #665

fix: update internal schema merging #665

ivan-tymoshenko commented Nov 25, 2023 •

edited

gurgunday Nov 25, 2023

ivan-tymoshenko Nov 25, 2023

gurgunday Nov 25, 2023

mcollina left a comment

gurgunday Nov 25, 2023

ivan-tymoshenko Nov 25, 2023

ivan-tymoshenko commented Dec 11, 2023

mcollina Dec 12, 2023

ivan-tymoshenko Dec 12, 2023

ivan-tymoshenko commented Jan 16, 2024

Uzlopak Jan 17, 2024

Uzlopak Jan 17, 2024

Uzlopak Jan 17, 2024

ivan-tymoshenko Jan 18, 2024

Uzlopak Jan 17, 2024

ivan-tymoshenko Jan 18, 2024

Uzlopak Jan 17, 2024

ivan-tymoshenko Jan 18, 2024

Uzlopak Jan 17, 2024

ivan-tymoshenko Jan 18, 2024

Uzlopak commented Jan 17, 2024

mcollina commented Jan 18, 2024

Uzlopak left a comment

Pezmc commented Jan 30, 2024 •

edited

	else throw new TypeError(\`The value of '${schemaRef}' does not match schema definition.\`)
	else throw new TypeError(\`The value of '${schemaRef}' does not match the schema definition.\`)

fix: update internal schema merging #665

fix: update internal schema merging #665

Conversation

ivan-tymoshenko commented Nov 25, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivan-tymoshenko commented Dec 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivan-tymoshenko commented Jan 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uzlopak commented Jan 17, 2024

mcollina commented Jan 18, 2024

Uzlopak left a comment

Choose a reason for hiding this comment

Pezmc commented Jan 30, 2024 • edited

ivan-tymoshenko commented Nov 25, 2023 •

edited

Pezmc commented Jan 30, 2024 •

edited