fix: ref with same id in properties

[climba03003]

I don't think it is a good fix, but it just works.
Fixes fastify/fastify#4028

This PR is a fix before #462 lands. Since #462 changed a lot in the internal ref resolve and may introduce breaking change.
I see the needs of a temporary fix.

@ivan-tymoshenko do you have better idea on this?

Checklist

• run npm run test and npm run benchmark
• tests and/or benchmarks are included
• documentation is changed or added
• commit message and code follows the Developer's Certification of Origin
  and the Code of conduct

[ivan-tymoshenko]

@climba03003 I think it's better to do it in the fast-json-stringify-compiler.
fastify/fast-json-stringify-compiler#14

We do the same thing in the ajv-compiler.
https://github.com/fastify/ajv-compiler/blob/10879404784f69aa8c5488f77622b52532a28799/index.js#L80

[climba03003]

I think it's better to do it in the fast-json-stringify-compiler.

But the problem still exist in here if you use it directly.
I can actually document, don't add $id in external schema.

[ivan-tymoshenko]

I would say that it shouldn't work. fastify.getSchema creates the wrong schema, so I think I may sense to fix it in the Fastify (fast-json-stringify-compiler).

[climba03003]

I would say that it shouldn't work. fastify.getSchema creates the wrong schema, so I think I may sense to fix it in the Fastify (fast-json-stringify-compiler).

You can see my test, it is nothing related to fastify.getSchema.
People can use the same schema in both ajv and fjs directly.
It is strange for people should not add $id in their external schemas since it is valid json schema.

[ivan-tymoshenko]

I mean, it's not a problem of fjs that we pass target schema duplicate in the external schemas. If we continue this logic inside fjs and make schema $id not strict, we will have to also check for duplicates inside external schemas and we don't need that.

[climba03003]

The bug actually cause by fjs directly replace the schema with schema that contain $id.

fast-json-stringify/index.js

Line 896 in 12fa1e9

schema = refFinder(schema.$ref, location)

And later on in anyOf or oneOf. It blindly add the schema that may contain duplicate $id cause by the line above.

fast-json-stringify/index.js

Lines 951 to 971 in 12fa1e9

	if (schema.anyOf || schema.oneOf) {
	// beware: dereferenceOfRefs has side effects and changes schema.anyOf
	const locations = dereferenceOfRefs(location, schema.anyOf ? 'anyOf' : 'oneOf')
	locations.forEach((location, index) => {
	const nestedResult = buildValue(locationPath + 'i' + index, input, location)
	// Since we are only passing the relevant schema to ajv.validate, it needs to be full dereferenced
	// otherwise any $ref pointing to an external schema would result in an error.
	// Full dereference of the schema happens as side effect of two functions:
	// 1. `dereferenceOfRefs` loops through the `schema.anyOf`` array and replaces any top level reference
	// with the actual schema
	// 2. `buildValue`, through `buildCode`, replaces any reference in object properties with the actual schema
	// (see https://github.com/fastify/fast-json-stringify/blob/6da3b3e8ac24b1ca5578223adedb4083b7adf8db/index.js#L631)
	// Ajv does not support js date format. In order to properly validate objects containing a date,
	// it needs to replace all occurrences of the string date format with a custom keyword fjs_date_type.
	// (see https://github.com/fastify/fast-json-stringify/pull/441)
	const extendedSchema = clone(location.schema)
	extendDateTimeType(extendedSchema)
	const schemaKey = location.schema.$id || randomUUID()
	ajvInstance.addSchema(extendedSchema, schemaKey)

Which is definitely something wrong within fjs. That's why I am mentioning #462 should fix it as well.

Edit: I am wrong #462 suffer from it as well.

[ivan-tymoshenko]

The reason why this test fails is that I forgot to clone external schemas, but not the duplicate schema. I added external schema cloning and your test to the #462. Thanks for finding it.

3b7a45c

This happens because fjs modifies external schemas. If you want to fix this bug as a separate PR, you need to clone external schemas. You don't need to delete any duplications.

[climba03003]

This happens because fjs modifies external schemas. If you want to fix this bug as a separate PR, you need to clone external schemas. You don't need to delete any duplications.

It does not work without the ajv changes. I can actually test it here and you can see the fail test.
with options.schema = clone(option.schema) only

https://github.com/fastify/fast-json-stringify/runs/6925112158?check_suite_focus=true

[ivan-tymoshenko]

Oh, I got it. Yeah, this is one of the reasons why I decided to use ajv. In #462, we pass json pointer to the original schema instead of coping oneOf/anyOf schema. I would say that this is a slightly different problem than fastify/fast-json-stringify-compiler#14.

Your solution fixes two things oneOf/anyOf case and top-level schema duplicate case. IMHO top-level schema duplicate case should be fixed in the fast-json-stringify-compiler. If we merge #462 oneOf/anyOf will be fixed.

Does it work for you?

If you want I can explain in more detail.

[climba03003]

Does it work for you?

Yes, #462 works.
but I am afraid it is a breaking change and cannot be able to lands on fastify@4.

So, this separate PR is needed for fastify@4.

[ivan-tymoshenko]

Why do you think #462 is a breaking change?

[climba03003]

Why do you think #462 is a breaking change?

Even if the test-case in here and fastify are all passed.
People in the outside world may already rely on the wrong behavior that we didn't test.
It is more safe to bump a major in this heavy internal changes.

There are two tests that use an incorrect format of json schema id/ref. They will not work after this fix.

From this line, it means we support some weird / wrong $refs resolution. You can consider it as fix but it also means breaking.

[ivan-tymoshenko]

There is said that Fastify supports the json schema draft 7 standard. I suppose if something is not working it's a bug. Each fix changes something in behavior.

[ivan-tymoshenko]

Your solution fixes it only partially. Only top level $ids.

test('ref with same id in properties', (t) => {
  t.plan(2)

  const externalSchema = {
    ObjectId: {
      $id: 'ObjectId',
      type: 'string',
      definitions: {
        p: { $id: 'p' }
      }
    },
    File: {
      $id: 'File',
      type: 'object',
      properties: {
        _id: { $ref: 'ObjectId' },
        name: { type: 'string' },
        owner: { $ref: 'ObjectId' }
      }
    }
  }

  t.test('anyOf', (t) => {
    t.plan(1)

    const schema = {
      $id: 'Article',
      type: 'object',
      properties: {
        _id: { $ref: 'ObjectId' },
        image: {
          anyOf: [
            { $ref: 'File' },
            { type: 'null' }
          ]
        }
      }
    }

    const stringify = build(schema, { schema: externalSchema })
    const output = stringify({ _id: 'foo', image: { _id: 'bar', name: 'hello', owner: 'baz' } })

    t.equal(output, '{"_id":"foo","image":{"_id":"bar","name":"hello","owner":"baz"}}')
  })

  t.test('oneOf', (t) => {
    t.plan(1)

    const schema = {
      $id: 'Article',
      type: 'object',
      properties: {
        _id: { $ref: 'ObjectId' },
        image: {
          oneOf: [
            { $ref: 'File' },
            { type: 'null' }
          ]
        }
      }
    }

    const stringify = build(schema, { schema: externalSchema })
    const output = stringify({ _id: 'foo', image: { _id: 'bar', name: 'hello', owner: 'baz' } })

    t.equal(output, '{"_id":"foo","image":{"_id":"bar","name":"hello","owner":"baz"}}')
  })
})

[climba03003]

There is said that Fastify supports the json schema draft 7 standard. I suppose if something is not working it's a bug. Each fix changes something in behavior.

I am not the one releasing this module and it can be land as patch or minor decided by who releasing it. But, if it's turns out breaking too many people. I think it might revert again.

I do see landing 462 benefit a lot.

Your solution fixes it only partially. Only top level $ids.

It will be complicated in fixing nested thing. I think I will stay in this step.
It do have workaround by extracting all nested $id schema to top level.

[ivan-tymoshenko]

And it should remove the duplicate schema only if schemas are equal. Otherwise, it should throw an error.

[climba03003]

And it should remove the duplicate schema only if schemas are equal. Otherwise, it should throw an error.

This solution do not mutate the original schema that added to FJS.
The current version never be able to resolve the nested schema with same $id.

So, when ever we delete the $id in refFinder. That $id schema should be the unique one and safe to delete.

[ivan-tymoshenko]

And it should remove the duplicate schema only if schemas are equal. Otherwise, it should throw an error.

This shouldn't compile correctly.

test('ref with same id in properties', (t) => {
  t.plan(2)

  const externalSchema = {
    ObjectId: {
      $id: 'ObjectId',
      type: 'string',
    },
    File: {
      $id: 'File',
      type: 'object',
      properties: {
        _id: { $ref: 'ObjectId' },
        name: { type: 'string' },
        owner: { $ref: 'ObjectId' }
      }
    }
  }

  t.test('anyOf', (t) => {
    t.plan(1)

    const schema = {
      $id: 'Article',
      type: 'object',
      properties: {
        _id: { $ref: 'ObjectId' },
        image: {
          anyOf: [
            { $ref: 'File' },
            {
              $id: 'File',
              type: 'number',
            },
            { type: 'null' },
          ]
        }
      }
    }

    const stringify = build(schema, { schema: externalSchema })
    const output = stringify({ _id: 'foo', image: { _id: 'bar', name: 'hello', owner: 'baz' } })

    t.equal(output, '{"_id":"foo","image":{"_id":"bar","name":"hello","owner":"baz"}}')
  })

  t.test('oneOf', (t) => {
    t.plan(1)

    const schema = {
      $id: 'Article',
      type: 'object',
      properties: {
        _id: { $ref: 'ObjectId' },
        image: {
          oneOf: [
            { $ref: 'File' },
            { type: 'null' }
          ]
        }
      }
    }

    const stringify = build(schema, { schema: externalSchema })
    const output = stringify({ _id: 'foo', image: { _id: 'bar', name: 'hello', owner: 'baz' } })

    t.equal(output, '{"_id":"foo","image":{"_id":"bar","name":"hello","owner":"baz"}}')
  })
})

[climba03003]

This shouldn't compile correctly.

Although it is the wrong behavior. But, it is possible currently.
This PR is not fixing this behavior.

const stringify = FJS(
  {
    $id: "https://example.com/schemas/base",
    type: "object",
    properties: {
      p1: { $id: "p1", type: "string" },
      p2: { $ref: "p1" },
    },
  },
  {
    schema: {
      p1: {
        $id: "p1",
        type: "number",
      },
    },
  }
);
console.log(stringify({ p1: "", p2: "" }));

[ivan-tymoshenko]

But, it is possible currently.

No, it's not. Why did you change the test?
This test works with your fix and doesn't work without it.

The key point is oneOf/anyOf/if-then - cases when we use ajv

test('anyOf', (t) => {
  t.plan(1)

  const externalSchema = {
    ObjectId: {
      $id: 'ObjectId',
      type: 'string',
    }
  }

  const schema = {
    type: 'object',
    properties: {
      image: {
        anyOf: [
          { $ref: 'ObjectId' },
          {
            $id: 'ObjectId',
            type: 'number',
          }
        ]
      }
    }
  }

  build(schema, { schema: externalSchema })
  t.pass()
})

[climba03003]

No, it's not. Why did you change the test?

This fix should handle both case now.

[mcollina]

Closing in favor of #462.