fix: remove getSchema from schema creation

[ivan-tymoshenko]

The getSchema method returns the original schema with its $id and all nested schemas with their $ids. In tests, the purpose of using the getSchema method is to create a reference to an already added schema, not to add a new one. The correct behavior is well described in the description of the addSchema method.

fastify.addSchema(schemaObj), adds a JSON schema to the Fastify instance. This allows you to reuse it everywhere in your application just by using the standard $ref keyword.

I marked this PR as a draft because I guess it can be a popular use case, so maybe there can be a more elegant solution. I would like to hear any comments about it.

[climba03003]

The test is valid, you cannot avoid this usage from public and we should validate the stored schema can be used directly through getSchema.

[ivan-tymoshenko]

@climba03003 The problem is that when we use getSchema we create a duplicate of one or more schemas that are stored in one global context. It creates inconsistency.

[climba03003]

I didn't get you about how should we validate schema.

I means the test ensure the store schema can be used directly from getSchema.

@climba03003 The problem is that when we use getSchema we create a duplicate of one or more schemas that are stored in one global context. It creates inconsistency.

Yes, it may not be a good usage. But it is a valid usage.
Test is used to ensure thing will works. You can duplicate the test case using $ref but it should not be removed unless breaking change introduced.

[ivan-tymoshenko]

I marked this PR as a draft because I guess it can be a popular use case, so maybe there can be a more elegant solution. I would like to hear any comments about it.

That's why this PR is drarf. I don't tell that avoiding this behavior is the only solution. But there is a problem. Do you have any suggestions on how this can be fixed?

[ivan-tymoshenko]

I wanted to show the simple test with inconsistency and found out even the basic reference is not working.

test('inner reference', t => {
  t.plan(1)
  const fastify = Fastify()

  fastify.get('/', {
    handler: () => {},
    schema: {
      response: {
        200: {
          type: 'object',
          properties: {
            p1: { $id: 'p1', type: 'string' },
            p2: { $ref: 'p1' }
          }
        }
      }
    }
  })

  fastify.ready(err => t.error(err))
})

[climba03003]

I wanted to show the simple test with inconsistency and found out even the basic reference is not working.

If it is inner reference of the schema, didn't it should be

test('inner reference', t => {
  t.plan(1)
  const fastify = Fastify()

  fastify.get('/', {
    handler: () => {},
    schema: {
      response: {
        200: {
          type: 'object',
          properties: {
            p1: { $id: 'p1', type: 'string' },
            p2: { $ref: '#/properties/p1' }
          }
        }
      }
    }
  })

  fastify.ready(err => t.error(err))
})

Specify the $id inside the property do not means it is a bundled schema.
The direct reference of it should not works.

{
  $id: '<hidden>', // here is the hidden uri-base, if it is the uri resources. It becomes http://example.com/schemas/xxx
  type: 'object',
  properties: {
    p1: { $id: 'p1', type: 'string' }, // id here is relative uri to the schema root $id
    p2: { $ref: 'p1' } // it is actually referencing to nothing
  }
}

[climba03003]

I have double check with how ajv performs and json-schema
The schema actually compiled without error. The inner schema $id field should replace the base-uri which means we should put it inside the schema pool.

I also see the point of fastify/fast-json-stringify#462.
schema reference resolve is complicated and easily to do it wrong.
The problem is inherited from fast-json-stringify.

import FJS from "fast-json-stringify";
const stringify = FJS({
  $id: "https://example.com/schemas/base",
  type: "object",
  properties: {
    p1: { $id: "p1", type: "string" },
    p2: { $ref: "p1" },
  },
});

[ivan-tymoshenko]

I have double check with how ajv performs and json-schema

It's not just how ajv performs it, it's a part of the json schema draft 7 standard.

The inner schema $id field should replace the base-uri which means we should put it inside the schema pool.

I didn't get about the replacement, but inside the schema, you should be able to define as many schemas as you want.

[ivan-tymoshenko]

I have made some investigation. Ajv checks for nested schemas with the same $ids. If these schemas have the same structure, it doesn't throw an error and processed them as one schema. The only case when it's not working is a top-level $ids, which we can handle.

An important fact is that it is not ok. Using fastify.getSchema creates an incorrect schema and then it can be fixed by
fast-json-stringify-compiler (fastify/fast-json-stringify-compiler#14) and by Ajv in the fast-json-stringify (if we use ajv for schema ref resolving). It's better to deprecate this behavior in the major release.

[mcollina]

It's better to deprecate this behavior in the major release.

Can you clarify what behavior exactly?

[Eomm]

In tests, the purpose of using the getSchema method is to create a reference to an already added schema, not to add a new one.

I totally agree with this statement and I understand the case.
I think that the getSchema test's usage is correct from the user perspective, so I would try to think of a new solution.

The scope is: the user wants the same schema it was put in using addSchema.
So we can't evict the $id, even if the user has already it because the $id is the getSchema argument.

So, I wonder if marking the object returned by the getSchema as Symbol.for('already-storead') in order to do not it twice.

I think this feature has an impact on fjs (as it should ignore these schemas).

I will try to think other solutions

[ivan-tymoshenko]

So, I wonder if marking the object returned by the getSchema as Symbol.for('already-storead') in order to do not it twice.

@Eomm I thought about this solution. One note. Users can use getSchema to insert nested subschema. Example:

fastify/test/schema-feature.test.js

Line 618 in 00f4088

fastify.getSchema('first')

So, we will have to make a recursive check.

[Eomm]

Mmmm you are right but I don't like it: we should check how many recursive chek we are running to minimise these operations because I'm losing the count personally 😅

What if we check that the object pointer (using schema === schema) is the same?
We should check if we are using the same object of course, but this would be much more efficient