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

fix: swapping out the $ref parsing engine of flattenSchema #292

Merged
merged 2 commits into from
Oct 20, 2020
Merged
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
143 changes: 121 additions & 22 deletions __tests__/tooling/lib/flatten-schema.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ const flattenSchema = require('../../../tooling/lib/flatten-schema');
const petstore = require('@readme/oas-examples/3.0/json/petstore');
const petstoreExpanded = require('@readme/oas-examples/3.0/json/petstore-expanded');

test('should flatten schema to an array', () => {
test('should flatten schema to an array', async () => {
const schema = {
type: 'object',
properties: {
Expand All @@ -16,7 +16,7 @@ test('should flatten schema to an array', () => {
},
};

expect(flattenSchema(schema)).toStrictEqual([
expect(await flattenSchema(schema)).toStrictEqual([
{
name: 'category',
type: '[String]',
Expand All @@ -25,10 +25,10 @@ test('should flatten schema to an array', () => {
]);
});

test('should flatten a component schema to an array', () => {
test('should flatten a component schema to an array', async () => {
const schema = petstore.components.schemas.Pet;

expect(flattenSchema(schema, petstore)).toStrictEqual([
expect(await flattenSchema(schema, petstore)).toStrictEqual([
{ name: 'id', type: 'Integer', description: undefined },
{ name: 'category', type: 'Object', description: undefined },
{ name: 'category.id', type: 'Integer', description: undefined },
Expand Down Expand Up @@ -67,11 +67,11 @@ describe('$ref usages', () => {
{ name: 'tag.name', type: 'String', description: undefined },
];

it('should flatten a schema that is a mix of objects and arrays', () => {
expect(flattenSchema(schema, petstore)).toStrictEqual(expected);
it('should flatten a schema that is a mix of objects and arrays', async () => {
expect(await flattenSchema(schema, petstore)).toStrictEqual(expected);
});

it('should flatten a schema that is a mix of objects and arrays but without explicit `type` properties set`', () => {
it('should flatten a schema that is a mix of objects and arrays but without explicit `type` properties set`', async () => {
const oas = {
components: {
schemas: {
Expand All @@ -85,10 +85,10 @@ describe('$ref usages', () => {
},
};

expect(flattenSchema(schema, oas)).toStrictEqual(expected);
expect(await flattenSchema(schema, oas)).toStrictEqual(expected);
});

it('should flatten a complex schema that mixes $ref and allOf', () => {
it('should flatten a complex schema that mixes $ref and allOf', async () => {
const complexSchema = {
allOf: [
{
Expand Down Expand Up @@ -174,7 +174,7 @@ describe('$ref usages', () => {
},
};

expect(flattenSchema(complexSchema, oas)).toStrictEqual([
expect(await flattenSchema(complexSchema, oas)).toStrictEqual([
{ name: 'createdAt', type: 'String', description: 'Creation Date' },
{ name: 'id', type: 'String', description: 'Record ID' },
{ name: 'applicationId', type: 'String', description: 'The UUID of the application.' },
Expand All @@ -186,41 +186,140 @@ describe('$ref usages', () => {
{ name: 'documentReferences[].documentID', type: 'String', description: 'Document ID' },
]);
});

describe('external $refs', () => {
it('should ignore an external $ref inside of an array', async () => {
const externalSchema = {
properties: {
responses: {
type: 'array',
items: {
$ref: 'https://example.com/ApiResponse.yaml',
},
},
tag: {
$ref: '#/components/schemas/Tag',
},
},
};

expect(await flattenSchema(externalSchema, petstore)).toStrictEqual([
{ name: 'responses', type: '[Circular]', description: undefined },
{ name: 'tag', type: 'Object', description: undefined },
{ name: 'tag.id', type: 'Integer', description: undefined },
{ name: 'tag.name', type: 'String', description: undefined },
]);
});

it('should ignore an external $ref on an object', async () => {
const externalSchema = {
properties: {
responses: {
type: 'array',
items: {
// $ref: 'https://example.com/ApiResponse.yaml',
$ref: '#/components/schemas/ApiResponse',
},
},
tag: {
$ref: 'https://example.com/Tag.yaml',
// $ref: '#/components/schemas/Tag',
},
},
};

expect(await flattenSchema(externalSchema, petstore)).toStrictEqual([
{ name: 'responses', type: '[Object]', description: undefined },
{ name: 'responses[].code', type: 'Integer', description: undefined },
{ name: 'responses[].type', type: 'String', description: undefined },
{ name: 'responses[].message', type: 'String', description: undefined },
{ name: 'tag', type: 'Circular', description: undefined },
]);
});
});

describe('circular $ref', () => {
const oas = {
components: {
schemas: {
Customfields: {
type: 'array',
items: {
$ref: '#/components/schemas/Customfields',
},
},
},
},
};

it('should not recurse a circular $ref inside of an objects properties', async () => {
const circularSchema = {
type: 'array',
items: {
type: 'object',
properties: {
id: {
type: 'number',
},
fields: {
$ref: '#/components/schemas/Customfields',
},
},
},
};

expect(await flattenSchema(circularSchema, oas)).toStrictEqual([
{ name: 'id', type: 'Number', description: undefined },
{ name: 'fields', type: 'Circular', description: undefined },
]);
});

it('should not recurse a circular $ref inside of an array', async () => {
const circularSchema = {
dok marked this conversation as resolved.
Show resolved Hide resolved
type: 'array',
items: {
$ref: '#/components/schemas/Customfields',
},
};

expect(await flattenSchema(circularSchema, oas)).toStrictEqual([]);
});
});
});

describe('polymorphism cases', () => {
describe('allOf', () => {
it('should flatten a schema to an array', () => {
it('should flatten a schema to an array', async () => {
const schema = {
type: 'array',
items: {
$ref: '#/components/schemas/Pet',
},
};

expect(flattenSchema(schema, petstoreExpanded)).toStrictEqual([
expect(await flattenSchema(schema, petstoreExpanded)).toStrictEqual([
{ name: 'name', type: 'String', description: undefined },
{ name: 'tag', type: 'String', description: undefined },
{ name: 'id', type: 'Integer', description: undefined },
]);
});

it('should flatten a component schema to an array', () => {
it('should flatten a component schema to an array', async () => {
const schema = petstoreExpanded.components.schemas.Pet;

expect(flattenSchema(schema, petstoreExpanded)).toStrictEqual([
expect(await flattenSchema(schema, petstoreExpanded)).toStrictEqual([
{ name: 'name', type: 'String', description: undefined },
{ name: 'tag', type: 'String', description: undefined },
{ name: 'id', type: 'Integer', description: undefined },
]);
});

it("should be able to handle an allOf that's nested a level down", () => {
it("should be able to handle an allOf that's nested a level down", async () => {
// eslint-disable-next-line global-require
const oas = require('../__fixtures__/nested-allof-flattening.json');
const schema = oas.components.schemas.extendedAttribute;

expect(flattenSchema(schema, oas)).toStrictEqual([
expect(await flattenSchema(schema, oas)).toStrictEqual([
{ name: 'createdOn', type: 'String', description: undefined },
{ name: 'lastModifiedOn', type: 'String', description: undefined },
{ name: 'application.href', type: 'String', description: undefined },
Expand All @@ -236,7 +335,7 @@ describe('polymorphism cases', () => {
]);
});

it('should be able to handle an allOf that contains deep $refs', () => {
it('should be able to handle an allOf that contains deep $refs', async () => {
const schema = {
allOf: [
{
Expand All @@ -259,7 +358,7 @@ describe('polymorphism cases', () => {
delete newPetSchema.properties.id;

expect(
flattenSchema(schema, {
await flattenSchema(schema, {
components: {
schemas: {
Category: petstore.components.schemas.Category,
Expand All @@ -284,13 +383,13 @@ describe('polymorphism cases', () => {
});

describe('anyOf', () => {
it('should flatten only the first schema listed', () => {
it('should flatten only the first schema listed', async () => {
const schema = {
anyOf: [{ $ref: '#/components/schemas/PetByAge' }, { $ref: '#/components/schemas/PetByType' }],
};

expect(
flattenSchema(schema, {
await flattenSchema(schema, {
components: {
schemas: {
PetByAge: {
Expand Down Expand Up @@ -329,13 +428,13 @@ describe('polymorphism cases', () => {
});

describe('oneOf', () => {
it('should flatten only the first schema listed', () => {
it('should flatten only the first schema listed', async () => {
const schema = {
oneOf: [{ $ref: '#/components/schemas/Cat' }, { $ref: '#/components/schemas/Dog' }],
};

expect(
flattenSchema(schema, {
await flattenSchema(schema, {
components: {
schemas: {
Dog: {
Expand Down
2 changes: 1 addition & 1 deletion package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
"version": "conventional-changelog --pkg package.json -i CHANGELOG.md -s && git add CHANGELOG.md"
},
"dependencies": {
"@apidevtools/json-schema-ref-parser": "^9.0.6",
"cardinal": "^2.1.1",
"colors": "^1.1.2",
"figures": "^3.0.0",
Expand Down
46 changes: 35 additions & 11 deletions tooling/lib/flatten-schema.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/* eslint-disable no-use-before-define */
const findSchemaDefinition = require('./find-schema-definition');
const $RefParser = require('@apidevtools/json-schema-ref-parser');
const flattenArray = require('./flatten-array');

const getName = (parent, prop) => {
Expand All @@ -11,14 +11,32 @@ const getName = (parent, prop) => {

const capitalizeFirstLetter = (string = '') => string.charAt(0).toUpperCase() + string.slice(1);

module.exports = (schema, oas) => {
module.exports = async (schema, oas) => {
const derefSchema = await $RefParser.dereference(
{ ...schema, ...oas },
{
resolve: {
// We shouldn't be resolving external pointers at this point so just ignore them.
external: false,
},
dereference: {
// If circular `$refs` are ignored they'll remain in `derefSchema` as `$ref: String`, otherwise `$ref‘ just
// won't exist. This allows us to do easy circular reference detection.
circular: 'ignore',
Copy link
Contributor

Choose a reason for hiding this comment

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

What would be the way we would detect whether a dereferenced object after this function call? Whether a $ref: String exists within the dereferenced object?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

If $ref exists in derefSchema at all, then it's circular.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha. Are there any other times where a $ref cannot be satisfied?

Copy link
Member Author

Choose a reason for hiding this comment

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

If it's an external ref (like a URL to another spec) it'll remain unsatisfied if it's inaccessible.

That said, we shouldn't be attempting to resolve external refs at this point in the application cycle so I'm going to add resolve.external: false into the options here to prevent that as well as a test for that.

},
}
);

function flattenObject(obj, parent, level) {
return flattenArray(
Object.keys(obj.properties).map(prop => {
let value = obj.properties[prop];
const value = obj.properties[prop];
let array = [];

if (value.$ref) {
value = findSchemaDefinition(value.$ref, oas);
// If we have a $ref present then it's circular and mark the type as such so we at least have something to
// identify it as to the user.
value.type = 'circular';
Copy link
Member Author

Choose a reason for hiding this comment

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

Setting the type here to this is so that in the frontend it'll show up as:

Screen Shot 2020-10-20 at 4 19 53 PM

}

// If `value` doesn't have an explicit `type` declaration, but has `properties` present, then
Expand All @@ -30,9 +48,17 @@ module.exports = (schema, oas) => {
if (value.type === 'object') {
array.push(flattenSchema(value, getName(parent, prop), level + 1));
} else if (value.type === 'array' && value.items) {
dok marked this conversation as resolved.
Show resolved Hide resolved
let { items } = value;
const { items } = value;
if (items.$ref) {
items = findSchemaDefinition(items.$ref, oas);
// If we have a $ref present for the array items it's a circular reference so let's mark it as such and
// break out.
array.push({
name: getName(parent, prop),
type: `[Circular]`,
description: value.description,
});

return array;
}

// If `value` doesn't have an explicit `type` declaration, but has `properties` present,
Expand Down Expand Up @@ -111,17 +137,15 @@ module.exports = (schema, oas) => {

return flattenSchema(obj.anyOf.shift());
} else if ('$ref' in obj) {
const value = findSchemaDefinition(obj.$ref, oas);
return flattenSchema(value, parent, level);
return flattenSchema({}, parent, level);
}

// top level array
if (obj.type === 'array' && obj.items) {
const newParent = parent ? `${parent}.[]` : '';

if (obj.items.$ref) {
const value = findSchemaDefinition(obj.items.$ref, oas);
return flattenSchema(value, newParent, level);
return flattenSchema(obj.items, newParent, level);
}

return flattenSchema(obj.items, `${newParent}`, level + 1);
Expand All @@ -134,5 +158,5 @@ module.exports = (schema, oas) => {
return flattenObject(obj, parent, level);
}

return flattenSchema(schema);
return flattenSchema(derefSchema);
};