Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support local reference in schema, support definitions keyword #676

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions lib/spec/openapi/index.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
'use strict'

const yaml = require('yaml')
const { shouldRouteHide } = require('../../util/common')
const { shouldRouteHide, patchDefinitionsKeywordInSchema } = require('../../util/common')
const { prepareDefaultOptions, prepareOpenapiObject, prepareOpenapiMethod, prepareOpenapiSchemas, normalizeUrl, resolveServerUrls } = require('./utils')

module.exports = function (opts, cache, routes, Ref, done) {
Expand All @@ -20,14 +20,19 @@ module.exports = function (opts, cache, routes, Ref, done) {
const openapiObject = prepareOpenapiObject(defOpts, done)

ref = Ref()
const resolvedDefs = ref.definitions().definitions

const schemaFromOptions = openapiObject.components.schemas
openapiObject.components.schemas = prepareOpenapiSchemas({
...openapiObject.components.schemas,
...(ref.definitions().definitions)
...schemaFromOptions,
...resolvedDefs
}, ref)

const serverUrls = resolveServerUrls(defOpts.servers)

for (const route of routes) {
route.schema = patchDefinitionsKeywordInSchema(route.schema)

const transformResult = defOpts.transform
? defOpts.transform({ schema: route.schema, url: route.url })
: {}
Expand Down
22 changes: 12 additions & 10 deletions lib/spec/openapi/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ function resolveBodyParams (body, schema, consumes, ref) {
}
}

function resolveCommonParams (container, parameters, schema, ref, sharedSchemas, securityIgnores) {
function resolveCommonParams (container, parameters, schema, ref, securityIgnores) {
const schemasPath = '#/components/schemas/'
let resolved = transformDefsToComponents(ref.resolve(schema))

Expand All @@ -270,7 +270,7 @@ function resolveCommonParams (container, parameters, schema, ref, sharedSchemas,
resolved = pathParts.reduce((resolved, pathPart) => resolved[pathPart], ref.definitions().definitions)
}

const arr = plainJsonObjectToOpenapi3(container, resolved, { ...sharedSchemas, ...ref.definitions().definitions }, securityIgnores)
const arr = plainJsonObjectToOpenapi3(container, resolved, { ...ref.definitions().definitions }, securityIgnores)
arr.forEach(swaggerSchema => parameters.push(swaggerSchema))
}

Expand Down Expand Up @@ -377,16 +377,16 @@ function prepareOpenapiMethod (schema, ref, openapiObject) {
if (schema.tags) openapiMethod.tags = schema.tags
if (schema.description) openapiMethod.description = schema.description
if (schema.externalDocs) openapiMethod.externalDocs = schema.externalDocs
if (schema.querystring) resolveCommonParams('query', parameters, schema.querystring, ref, openapiObject.definitions, securityIgnores.query)
if (schema.querystring) resolveCommonParams('query', parameters, schema.querystring, ref, securityIgnores.query)
if (schema.body) {
openapiMethod.requestBody = { content: {} }
resolveBodyParams(openapiMethod.requestBody, schema.body, schema.consumes, ref)
}
if (schema.params) resolveCommonParams('path', parameters, schema.params, ref, openapiObject.definitions)
if (schema.headers) resolveCommonParams('header', parameters, schema.headers, ref, openapiObject.definitions, securityIgnores.header)
if (schema.params) resolveCommonParams('path', parameters, schema.params, ref)
if (schema.headers) resolveCommonParams('header', parameters, schema.headers, ref, securityIgnores.header)
// TODO: need to documentation, we treat it same as the querystring
// fastify do not support cookies schema in first place
if (schema.cookies) resolveCommonParams('cookie', parameters, schema.cookies, ref, openapiObject.definitions, securityIgnores.cookie)
if (schema.cookies) resolveCommonParams('cookie', parameters, schema.cookies, ref, securityIgnores.cookie)
if (parameters.length > 0) openapiMethod.parameters = parameters
if (schema.deprecated) openapiMethod.deprecated = schema.deprecated
if (schema.security) openapiMethod.security = schema.security
Expand All @@ -407,16 +407,18 @@ function prepareOpenapiSchemas (schemas, ref) {
return Object.entries(schemas)
.reduce((res, [name, schema]) => {
const _ = { ...schema }
const resolved = transformDefsToComponents(ref.resolve(_, { externalSchemas: [schemas] }))

const resolved = ref.resolve(_, { externalSchemas: [schemas] })
const transformed = transformDefsToComponents(resolved)

// Swagger doesn't accept $id on /definitions schemas.
// The $ids are needed by Ref() to check the URI so we need
// to remove them at the end of the process
// definitions are added by resolve but they are replace by components.schemas
delete resolved.$id
delete resolved.definitions
delete transformed.$id
delete transformed.definitions

res[name] = resolved
res[name] = transformed
return res
}, {})
}
Expand Down
4 changes: 3 additions & 1 deletion lib/spec/swagger/index.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
'use strict'

const yaml = require('yaml')
const { shouldRouteHide } = require('../../util/common')
const { shouldRouteHide, patchDefinitionsKeywordInSchema } = require('../../util/common')
const { prepareDefaultOptions, prepareSwaggerObject, prepareSwaggerMethod, normalizeUrl, prepareSwaggerDefinitions } = require('./utils')

module.exports = function (opts, cache, routes, Ref, done) {
Expand All @@ -26,6 +26,8 @@ module.exports = function (opts, cache, routes, Ref, done) {

swaggerObject.paths = {}
for (const route of routes) {
route.schema = patchDefinitionsKeywordInSchema(route.schema)

const transformResult = defOpts.transform
? defOpts.transform({ schema: route.schema, url: route.url })
: {}
Expand Down
72 changes: 70 additions & 2 deletions lib/util/common.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,13 @@ function addHook (fastify, pluginOptions) {
routes,
Ref () {
const externalSchemas = cloner(Array.from(sharedSchemasMap.values()))
const resolvedToAbsolute = externalSchemas.map(e => localSchemaRefToAbs(e))
const withPatchedDefinitions = resolvedToAbsolute.map(e => patchDefinitionsKeywordInSchema(e))

return Ref(Object.assign(
{ applicationUri: 'todo.com' },
pluginOptions.refResolver,
{ clone: true, externalSchemas })
{ clone: true, externalSchemas: withPatchedDefinitions })
)
}
}
Expand Down Expand Up @@ -210,11 +213,76 @@ function resolveSwaggerFunction (opts, cache, routes, Ref, done) {
}
}

function localSchemaRefToAbs (schema, fullPath, pathFromLastId) {
if (schema.$id) {
if (!fullPath) {
fullPath = schema.$id + '#'
}
// start from the last obj with $id
pathFromLastId = ''
}

Object.keys(schema).forEach(key => {
if (key === '$ref' && schema[key] && schema[key].startsWith('#')) {
// #/something/... -> something/...
const woLocalSymbol = schema[key].substring(2)

// where ObjectA is last obj with #id
// where pathFromLastId is the path from ObjectA to the current object

// root#/properties/ObjectA/properties -> root#/properties/ObjectA
// todo: cov fail ???
/* istanbul ignore else */
if (pathFromLastId) {
const pathIdxForReplace = fullPath.lastIndexOf(pathFromLastId)
fullPath = fullPath.substring(0, pathIdxForReplace)
}

// handle cases like '#' or '#/something'
const absPath = woLocalSymbol ? fullPath + '/' + woLocalSymbol : fullPath
schema[key] = absPath
} else if (schema[key] && typeof schema[key] === 'object') {
const isRefObj = schema[key].$ref
schema[key] = localSchemaRefToAbs(schema[key],
// don't extend paths if we are going to $ref object
isRefObj ? fullPath : fullPath + '/' + key,
isRefObj ? pathFromLastId : pathFromLastId + '/' + key
)
}
})

return schema
}

function patchDefinitionsKeywordInSchema (schema) {
if (!schema) return schema

Object.keys(schema).forEach(key => {
if (key === '$ref' && schema[key]) {
schema[key] = schema[key].split('definitions').join('properties')
} else if (schema[key] && typeof schema[key] === 'object') {
if (key === 'definitions') {
schema.properties = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you move schema from the definitions to the properties? You shouldn't create new properties.

Copy link
Author

@asc11cat asc11cat Nov 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you move schema from the definitions to the properties? You shouldn't create new properties.

This is a workaround because 'definitions' keyword itself is not allowed(and will trigger error on openapi schema validation), but we still need to support it(#676 (comment))

The only other way I can think of is somehow allowing additional keywords, adding something like 'x-definitions'(or even the 'definitions' keyword itself) and moving data there, but I don't see a straightforward way to implement it.

...schema[key],
...schema.properties
}
delete schema[key]
key = 'properties'
}
schema[key] = patchDefinitionsKeywordInSchema(schema[key])
}
})

return schema
}

module.exports = {
addHook,
shouldRouteHide,
readPackageJson,
formatParamUrl,
resolveLocalRef,
resolveSwaggerFunction
resolveSwaggerFunction,
localSchemaRefToAbs,
patchDefinitionsKeywordInSchema
}
152 changes: 152 additions & 0 deletions test/spec/openapi/refs.js
Original file line number Diff line number Diff line change
Expand Up @@ -352,3 +352,155 @@ test('renders $ref schema with additional keywords', async (t) => {
t.match(res.statusCode, 400)
t.match(openapiObject.paths['/url1'].get.parameters[0].schema, cookie)
})

test('support absolute refs in schema', async (t) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These due tests:

  • support absolute refs in schema
  • support relative refs in schema

Are already supported by json-schema-resolver Eomm/json-schema-resolver#8

The openapi conversion instead ( test named support definitions keyword in schema) is provided by this module.

So the questions are:

  • is this an issue with the conversion from swagger to openapi?
  • is this an issue with fast-json-stringify? (as it resolves the schemas by its own)

So, right now, I think this fix does not solve the core problem

Copy link
Author

@asc11cat asc11cat Nov 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Eomm

Indeed, this tests were passing in the json-schema-resolver lib, and lib itself is perfectly fine. However, the core problem is in the difference between json-schema standard, and swagger/openapi one.

First, the 'definitions' keyword. It is supported by json-schema standard, but not supported in openapi/swagger. However, it can be provided by user in schema. Issues related to that problem: #639, #524, #612. Here is my comment on why this occurs - #639 (comment).

To fix that, patchDefinitionsKeywordInSchema method was introduced. This method is changing any occurrences of 'definitions' keyword to 'properties' one, including refs. It also merges them with precedence to the existing properties.

Second one, the support of local $refs in openapi(and possibly swagger). This is explained in #676 (comment) by Ivan. In short, local $ref like #/definitions/something in final openapi schema is not supported by openapi itself.

To fix that, we use localSchemaRefToAbs method, which resolves any local refs to the absolute ones.

support relative refs in schema and support absolute refs in schema tests were made for the second issue, first one for the issue itself, and the second one to ensure that we don't break anything that worked before, when introducing the local->abs resolver.

Additional tests that were introduced in test/util.js can give a more clear picture on this I think.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the detailed information, I need to catch up on this 👍🏽
I want to explore a bit the openapi repo as it is source of best practices in these cases

const fastify = Fastify()
await fastify.register(fastifySwagger, { openapi: {} })
fastify.register(async (instance) => {
instance.addSchema(
{
$id: 'ObjectA',
type: 'object',
properties: {
example: {
type: 'string'
}
}
}
)
instance.addSchema(
{
$id: 'ObjectC',
type: 'object',
properties: {
referencedObjA: {
$ref: 'ObjectA#'
},
referencedObjC: {
$ref: 'ObjectC#/properties/ObjectD'
},
ObjectD: {
type: 'object',
properties: {
d: {
type: 'string'
}
}
}
}
}
)
instance.post('/third/:sample', {
schema: {
body: {
$ref: 'ObjectC#'
},
params: {
$ref: 'ObjectC#'
},
response: { 200: { $ref: 'ObjectC#' } }
}
}, async () => ({ result: true }))
})

await fastify.ready()

const openapiObject = fastify.swagger()
t.equal(typeof openapiObject, 'object')

// if validation is passed = success
await Swagger.validate(openapiObject)
})

test('support relative refs in schema', async (t) => {
const fastify = Fastify()
await fastify.register(fastifySwagger, { openapi: {} })
fastify.register(async (instance) => {
instance.addSchema({
$id: 'ObjectA',
type: 'object',
properties: {
sample: {
type: 'object',
properties: {
a: { type: 'string' },
b: { type: 'object', properties: { d: { type: 'string' } } }
}
},
someValue: { type: 'string' },
relativeExample: {
$ref: '#/properties/sample'
}
}
})

instance.post('/first/:sample', {
schema: {
body: {
$ref: 'ObjectA#/properties/relativeExample'
},
params: {
$ref: 'ObjectA#/properties/relativeExample'
},
response: { 200: { $ref: 'ObjectA#/properties/relativeExample' } }
}
}, async () => ({ result: true }))
})

await fastify.ready()

const openapiObject = fastify.swagger()
t.equal(typeof openapiObject, 'object')

// if validation is passed = success
await Swagger.validate(openapiObject)
})

test('support definitions keyword in schema', async (t) => {
const fastify = Fastify()
await fastify.register(fastifySwagger, { openapi: {} })

fastify.register(async (instance) => {
instance.addSchema({
$id: 'ObjectA',
type: 'object',
definitions: {
sample: {
type: 'object',
properties: {
a: { type: 'string' },
b: { type: 'object', properties: { d: { type: 'string' } } }
}
},
someValue: { type: 'string' },
relativeExample: {
$ref: '#/definitions/sample'
}
}
})

instance.post('/first/:sample', {
schema: {
body: {
$ref: 'ObjectA#/definitions/relativeExample'
},
params: {
$ref: 'ObjectA#/definitions/relativeExample'
},
response: { 200: { $ref: 'ObjectA#/definitions/relativeExample' } }
}
}, async () => ({ result: true }))
})

await fastify.ready()

const openapiObject = fastify.swagger()
t.equal(typeof openapiObject, 'object')

// definitions are transformed to properties
// previous properties obj take precedence over definitions obj
t.equal(openapiObject.paths['/first/{sample}'].post.requestBody.content['application/json'].schema.$ref, '#/components/schemas/def-0/properties/relativeExample')
t.equal(openapiObject.paths['/first/{sample}'].post.responses['200'].content['application/json'].schema.$ref, '#/components/schemas/def-0/properties/relativeExample')

await Swagger.validate(openapiObject)
})
Loading