feat: support local reference in schema, support definitions keyword

[asc11cat]

Addressing #675 and #639

Checklist

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

[asc11cat]

@ivan-tymoshenko I faced some issues related to relative definitions, and will appreciate if you can take a look

Right now when we will try to do something like this(even on master):

instance.addSchema({
      $id: 'NestedSchema',
      properties: {
        SchemaA: {
          type: 'object',
          properties: {
            id: { type: 'string' }
          }
        },
        SchemaB: {
          type: 'object',
          properties: {
            example: { $ref: '#/properties/SchemaA' }
          }
        }
      }
    })

We will get the 'Token "properties" does not exist' exception from Swagger.validate. As i understand, the relative definitions is the correct behavior for json-schema(so for json-schema-resolver which is used for ref resolve in this lib), but not for a swagger/openapi one, so we should patch the relative definitions to absolute?

Another problem is the definitions patching. We have 2 sources of schema's here, first one is original coming from addHook return value which is resolved by json-schema-resolver, before any swagger/openapi related funcs is called https://github.com/fastify/fastify-swagger/blob/master/lib/util/common.js#L51

The second one is the any schema that we get from transformDefsToComponents helper. And there are cases when both of this schemas are used together, for example to resolve a path, https://github.com/fastify/fastify-swagger/blob/master/lib/spec/openapi/utils.js#L263

So if we mutate something like definitions in the second schema, this will break this interactions, because the first schema remains original. So we need to consider patching on addHook level, or....?

And the last issue that I see currently is about transforming the references, right now its done in transformDefsToComponents https://github.com/fastify/fastify-swagger/blob/master/lib/spec/openapi/utils.js#L102

we don't know any info except the reference itself in this method, so we can't really determine the difference between type of references(absolute/relative/cross-reference/etc) and can't correctly patch them to the absolute. This becomes a problem mostly due to the json-schema-resolver behavior in our case, where it adds '#definitions/' to any input that we have later in transformDefsToComponents. E.g original Order#/properties/OrderId -> #/definitions/Order/properties/OrderId and so on.

[ivan-tymoshenko]

so we should patch the relative definitions to absolute?

According to json schema spec if you define subschema with a $id property the root inside this schema chages.

{
  $id: 'globalsSchema',
  properties: {
    a: { $ref: '#' }, // points to the 'globalSchema'
    b: {
      $id: 'localSchema',
      type: 'object',
      properties: {
        c: { $ref: '#' } // points to the local schema
      }
    }
  }
}

Swagger doesn't work like that. As I understand when you merge all schemas to the swagger schema you get one big schema with a new root. So # is no longer point to you local schema, but for one global schema. So yes, you should replace all local refs with absolute refs.

[ivan-tymoshenko]

What we can also try to do is to save each schema to the separate file and use path in $ref instead of schema id. Then it will will resolve all local references correctly. But I don't know how much overhead it will have.

[asc11cat]

so we should patch the relative definitions to absolute?

According to json schema spec if you define subschema with a $id property the root inside this schema chages.

{
  $id: 'globalsSchema',
  properties: {
    a: { $ref: '#' }, // points to the 'globalSchema'
    b: {
      $id: 'localSchema',
      type: 'object',
      properties: {
        c: { $ref: '#' } // points to the local schema
      }
    }
  }
}

Swagger doesn't work like that. As I understand when you merge all schemas to the swagger schema you get one big schema with a new root. So # is no longer point to you local schema, but for one global schema. So yes, you should replace all local refs with absolute refs.

so we should patch the relative definitions to absolute?

According to json schema spec if you define subschema with a $id property the root inside this schema chages.

{
  $id: 'globalsSchema',
  properties: {
    a: { $ref: '#' }, // points to the 'globalSchema'
    b: {
      $id: 'localSchema',
      type: 'object',
      properties: {
        c: { $ref: '#' } // points to the local schema
      }
    }
  }
}

Swagger doesn't work like that. As I understand when you merge all schemas to the swagger schema you get one big schema with a new root. So # is no longer point to you local schema, but for one global schema. So yes, you should replace all local refs with absolute refs.

Got it, ill try to see what i can do here in a few days, i think it will be better to rethink on how this process of schema mutation from json-schema->openapi works altogether, because right now its really difficult to extend it on need.

[asc11cat]

@ivan-tymoshenko finally got some time to finish this PR

Now local references are being transformed to absolute, and 'definitions' keyword is being merged with 'properties', with precedence to the last one

[ivan-tymoshenko]

I will check it deeply ASAP. But I have a question for now. I think that problem is in the json-schema-resolver. Shouldn't functions like localSchemaRefToAbs and patchDefinitionsKeywordInSchema be there?

[ivan-tymoshenko]

@Eomm Can you help us here?

[asc11cat]

I will check it deeply ASAP. But I have a question for now. I think that problem is in the json-schema-resolver. Shouldn't functions like localSchemaRefToAbs and patchDefinitionsKeywordInSchema be there?

Both functions are patching behaviour related only to openapi schema standart, not the json-schema one. In the first case, openapi schema don't support the local refs, in the second - no support for 'definitions' keyword

[asc11cat]

Now tests should pass, replaceAll is not supported in node 14, so I replaced it

[asc11cat]

@Eomm any thoughts on this yet?

[Eomm]

To review this PR, I need ~4h and I need to find such time 😮‍💨

[mathiasbockwoldt]

Hi @Eomm, did you find some time, yet? Would be great to get this PR merged. 🚀

[melroy89]

To review this PR, I need ~4h and I need to find such time 😮‍💨

This is now blocking for 1 year.. this is not good. Hopefully somebody else can review and merge this PR.

[mcollina]

@asc11cat can you refresh this PR? There are quite a few conflicts now.

[ivan-tymoshenko]

After 6 hours trying to understand what is going on here I have few conclusions:

• the library needs some brushing. There are a lot of bicycles invented here, so when you touching one part of the code another one is failing.
• about the issue. IMHO before adding a json-schema to openapi-swagger schema we should recursively check for all keywords that are restricted to use in openapi/swagger (not only the definition keyword). When we find the keyword we need to remove it from the original schema and add it to the global definitions/components section. At the same time we sshould have some global ref-resolver that would resolve any reference including the refs that includes path with restricted kwyword. Probably it should have some internal mapping like that #/components/schemas/def-0/definitions/foo/properties/bar -> #/components/schemas/def-42

[gurgunday]

I agree that this plugin needs a rewrite/refactor

[navalex]

Hello, any news on this case ? Does this PR just dropped because too complex too heavy with the current state of the library ? Or can we expect a little fix to manage it ?

Thanks :)

EDIT: I'm actually using this PR fork branch, and it seems to be a pretty good workaround, at least for my current project. Could be a short-time fix