From 681c1031fb67a7f4edb456cf86c377683f59728f Mon Sep 17 00:00:00 2001 From: Barrett Schonefeld Date: Wed, 3 Mar 2021 13:20:34 -0600 Subject: [PATCH] fix: no error for inline array schema when items field is a ref Purpose: - generator flattens reference to an array schema back into an inline schema Changes: - Add a check to see if the schema is an array with an items field that uses a ref - Add a check to see if the array schema has primitive type items - Add reusable isPrimitiveType function Tests: - Add tests to ensure the validator does not flag different variations of this array schema - Update expected warnings in tests that use specs with this array schema - Add test to ensure arrays with primitive-type items are not flagged - Add tests for isPrimitiveType function --- src/plugins/utils/isPrimitiveType.js | 7 + .../2and3/semantic-validators/responses.js | 29 +++- .../tests/expected-output.test.js | 11 +- .../cli-validator/tests/info-and-hint.test.js | 2 +- .../tests/option-handling.test.js | 17 +-- test/plugins/utils/isPrimitiveType.test.js | 42 ++++++ .../validation/2and3/responses.test.js | 132 ++++++++++++++++++ 7 files changed, 218 insertions(+), 22 deletions(-) create mode 100644 src/plugins/utils/isPrimitiveType.js create mode 100644 test/plugins/utils/isPrimitiveType.test.js diff --git a/src/plugins/utils/isPrimitiveType.js b/src/plugins/utils/isPrimitiveType.js new file mode 100644 index 000000000..1fd098de3 --- /dev/null +++ b/src/plugins/utils/isPrimitiveType.js @@ -0,0 +1,7 @@ +// takes a schema object that has a 'type' field +module.exports = function(schema) { + return ( + schema.type && + ['boolean', 'integer', 'number', 'string'].includes(schema.type) + ); +}; diff --git a/src/plugins/validation/2and3/semantic-validators/responses.js b/src/plugins/validation/2and3/semantic-validators/responses.js index 043a24de9..01906a7ce 100644 --- a/src/plugins/validation/2and3/semantic-validators/responses.js +++ b/src/plugins/validation/2and3/semantic-validators/responses.js @@ -1,10 +1,20 @@ const each = require('lodash/each'); const { walk, isResponseObject } = require('../../../utils'); const MessageCarrier = require('../../../utils/messageCarrier'); +const isPrimitiveType = require('../../../utils/isPrimitiveType'); const INLINE_SCHEMA_MESSAGE = 'Response schemas should be defined with a named ref.'; +function arrayItemsAreRefOrPrimitive(schema) { + return ( + schema && + schema.type === 'array' && + schema.items && + (schema.items.$ref || isPrimitiveType(schema.items)) + ); +} + module.exports.validate = function({ jsSpec, isOAS3 }, config) { const messages = new MessageCarrier(); @@ -36,9 +46,12 @@ module.exports.validate = function({ jsSpec, isOAS3 }, config) { i < mediaType.schema[schemaType].length; i++ ) { - const hasInlineSchema = !mediaType.schema[schemaType][i] - .$ref; - if (hasInlineSchema) { + const currentSchema = mediaType.schema[schemaType][i]; + const hasInlineSchema = !currentSchema.$ref; + if ( + hasInlineSchema && + !arrayItemsAreRefOrPrimitive(currentSchema) + ) { messages.addMessage( [ ...path, @@ -57,7 +70,10 @@ module.exports.validate = function({ jsSpec, isOAS3 }, config) { } } }); - } else if (!mediaType.schema.$ref) { + } else if ( + !mediaType.schema.$ref && + !arrayItemsAreRefOrPrimitive(mediaType.schema) + ) { messages.addMessage( [...path, responseKey, 'content', mediaTypeKey, 'schema'], INLINE_SCHEMA_MESSAGE, @@ -72,7 +88,10 @@ module.exports.validate = function({ jsSpec, isOAS3 }, config) { if (responseKey.startsWith('x-')) return; const hasInlineSchema = response.schema && !response.schema.$ref; - if (hasInlineSchema) { + if ( + hasInlineSchema && + !arrayItemsAreRefOrPrimitive(response.schema) + ) { messages.addMessage( [...path, responseKey, 'schema'], INLINE_SCHEMA_MESSAGE, diff --git a/test/cli-validator/tests/expected-output.test.js b/test/cli-validator/tests/expected-output.test.js index 5321160d3..aa45cd80b 100644 --- a/test/cli-validator/tests/expected-output.test.js +++ b/test/cli-validator/tests/expected-output.test.js @@ -145,7 +145,7 @@ describe('cli tool - test expected output - Swagger 2', function() { const validationResults = await inCodeValidator(oas2Object, defaultMode); expect(validationResults.errors.length).toBe(5); - expect(validationResults.warnings.length).toBe(9); + expect(validationResults.warnings.length).toBe(8); expect(validationResults.infos).not.toBeDefined(); expect(validationResults.hints).not.toBeDefined(); @@ -181,10 +181,9 @@ describe('cli tool - test expected output - Swagger 2', function() { expect(capturedText[33].match(/\S+/g)[2]).toEqual('15'); expect(capturedText[37].match(/\S+/g)[2]).toEqual('15'); expect(capturedText[41].match(/\S+/g)[2]).toEqual('197'); - expect(capturedText[45].match(/\S+/g)[2]).toEqual('108'); - expect(capturedText[49].match(/\S+/g)[2]).toEqual('131'); - expect(capturedText[53].match(/\S+/g)[2]).toEqual('134'); - expect(capturedText[57].match(/\S+/g)[2]).toEqual('126'); + expect(capturedText[45].match(/\S+/g)[2]).toEqual('131'); + expect(capturedText[49].match(/\S+/g)[2]).toEqual('134'); + expect(capturedText[53].match(/\S+/g)[2]).toEqual('126'); }); it('should return exit code of 0 if there are only warnings', async function() { @@ -364,7 +363,7 @@ describe('test expected output - OpenAPI 3', function() { const validationResults = await inCodeValidator(oas3Object, defaultMode); expect(validationResults.errors.length).toBe(4); - expect(validationResults.warnings.length).toBe(11); + expect(validationResults.warnings.length).toBe(10); expect(validationResults.infos).not.toBeDefined(); expect(validationResults.hints).not.toBeDefined(); diff --git a/test/cli-validator/tests/info-and-hint.test.js b/test/cli-validator/tests/info-and-hint.test.js index d8f487339..10b88307c 100644 --- a/test/cli-validator/tests/info-and-hint.test.js +++ b/test/cli-validator/tests/info-and-hint.test.js @@ -39,7 +39,7 @@ describe('test info and hint rules - OAS3', function() { expect(jsonOutput['errors']['schema-ibm'].length).toBe(1); // Verify warnings - expect(jsonOutput['warnings']['responses'].length).toBe(3); + expect(jsonOutput['warnings']['responses'].length).toBe(2); expect(jsonOutput['warnings']['operations-shared'].length).toBe(2); expect(jsonOutput['warnings']['refs'].length).toBe(1); expect(jsonOutput['warnings']['schema-ibm'].length).toBe(1); diff --git a/test/cli-validator/tests/option-handling.test.js b/test/cli-validator/tests/option-handling.test.js index b3d0b1d65..a929716e1 100644 --- a/test/cli-validator/tests/option-handling.test.js +++ b/test/cli-validator/tests/option-handling.test.js @@ -121,7 +121,7 @@ describe('cli tool - test option handling', function() { // totals expect(capturedText[statsSection + 1].match(/\S+/g)[5]).toEqual('5'); - expect(capturedText[statsSection + 2].match(/\S+/g)[5]).toEqual('9'); + expect(capturedText[statsSection + 2].match(/\S+/g)[5]).toEqual('8'); // errors expect(capturedText[statsSection + 5].match(/\S+/g)[0]).toEqual('1'); @@ -138,25 +138,22 @@ describe('cli tool - test option handling', function() { // warnings expect(capturedText[statsSection + 11].match(/\S+/g)[0]).toEqual('2'); - expect(capturedText[statsSection + 11].match(/\S+/g)[1]).toEqual('(22%)'); + expect(capturedText[statsSection + 11].match(/\S+/g)[1]).toEqual('(25%)'); expect(capturedText[statsSection + 12].match(/\S+/g)[0]).toEqual('2'); - expect(capturedText[statsSection + 12].match(/\S+/g)[1]).toEqual('(22%)'); + expect(capturedText[statsSection + 12].match(/\S+/g)[1]).toEqual('(25%)'); expect(capturedText[statsSection + 13].match(/\S+/g)[0]).toEqual('1'); - expect(capturedText[statsSection + 13].match(/\S+/g)[1]).toEqual('(11%)'); + expect(capturedText[statsSection + 13].match(/\S+/g)[1]).toEqual('(13%)'); expect(capturedText[statsSection + 14].match(/\S+/g)[0]).toEqual('1'); - expect(capturedText[statsSection + 14].match(/\S+/g)[1]).toEqual('(11%)'); + expect(capturedText[statsSection + 14].match(/\S+/g)[1]).toEqual('(13%)'); expect(capturedText[statsSection + 15].match(/\S+/g)[0]).toEqual('1'); - expect(capturedText[statsSection + 15].match(/\S+/g)[1]).toEqual('(11%)'); + expect(capturedText[statsSection + 15].match(/\S+/g)[1]).toEqual('(13%)'); expect(capturedText[statsSection + 16].match(/\S+/g)[0]).toEqual('1'); - expect(capturedText[statsSection + 16].match(/\S+/g)[1]).toEqual('(11%)'); - - expect(capturedText[statsSection + 17].match(/\S+/g)[0]).toEqual('1'); - expect(capturedText[statsSection + 17].match(/\S+/g)[1]).toEqual('(11%)'); + expect(capturedText[statsSection + 16].match(/\S+/g)[1]).toEqual('(13%)'); }); it('should not print statistics report by default', async function() { diff --git a/test/plugins/utils/isPrimitiveType.test.js b/test/plugins/utils/isPrimitiveType.test.js new file mode 100644 index 000000000..9974034ad --- /dev/null +++ b/test/plugins/utils/isPrimitiveType.test.js @@ -0,0 +1,42 @@ +const expect = require('expect'); +const isPrimitiveType = require('../../../src/plugins/utils/isPrimitiveType'); + +describe('isPrimitiveType - util', () => { + it('should return true when the schema uses a primitive type', () => { + const exampleObject = { + schema: { + type: 'string' + } + }; + + expect(isPrimitiveType(exampleObject.schema)).toBe(true); + }); + + it('should return true when given items with a primitive type', () => { + const exampleObject = { + schema: { + type: 'array', + items: { + type: 'boolean' + } + } + }; + + expect(isPrimitiveType(exampleObject.schema.items)).toBe(true); + }); + + it('should return false when the schema uses a non-primitive type', () => { + const exampleObject = { + schema: { + type: 'object', + properties: { + exampleProp: { + type: 'string' + } + } + } + }; + + expect(isPrimitiveType(exampleObject.schema)).toBe(false); + }); +}); diff --git a/test/plugins/validation/2and3/responses.test.js b/test/plugins/validation/2and3/responses.test.js index 9d9e251c4..d92c059f3 100644 --- a/test/plugins/validation/2and3/responses.test.js +++ b/test/plugins/validation/2and3/responses.test.js @@ -76,6 +76,64 @@ describe('validation plugin - semantic - responses', function() { expect(res.errors.length).toEqual(0); }); + it('should not complain about inline array schema with items defined as ref', function() { + const spec = { + paths: { + '/stuff': { + get: { + summary: 'list stuff', + operationId: 'listStuff', + produces: ['application/json'], + responses: { + 200: { + description: 'successful operation', + schema: { + type: 'array', + items: { + $ref: '#/components/schemas/ListStuffResponseModel' + } + } + } + } + } + } + } + }; + + const res = validate({ jsSpec: spec }, config); + expect(res.warnings.length).toEqual(0); + expect(res.errors.length).toEqual(0); + }); + + it('should not complain about inline array schema with primitive type items', function() { + const spec = { + paths: { + '/stuff': { + get: { + summary: 'list stuff', + operationId: 'listStuff', + produces: ['application/json'], + responses: { + 200: { + description: 'successful operation', + schema: { + type: 'array', + items: { + type: 'string' + } + } + } + } + } + } + } + }; + + const res = validate({ jsSpec: spec }, config); + expect(res.warnings.length).toEqual(0); + expect(res.errors.length).toEqual(0); + }); + it('should not complain for a response with no schema', function() { const spec = { paths: { @@ -271,6 +329,80 @@ describe('validation plugin - semantic - responses', function() { expect(res.warnings.length).toEqual(0); expect(res.errors.length).toEqual(0); }); + + it('should not complain for a valid combined schema where one schema is an array with items defined as ref', function() { + const spec = { + paths: { + '/stuff': { + get: { + summary: 'list stuff', + operationId: 'listStuff', + responses: { + 200: { + description: 'successful operation', + content: { + 'application/json': { + schema: { + anyOf: [ + { + $ref: + '#/components/schemas/ListStuffResponseModel' + }, + { + type: 'array', + items: { + $ref: + '#/components/schemas/ListStuffSecondModel' + } + } + ] + } + } + } + } + } + } + } + } + }; + + const res = validate({ jsSpec: spec, isOAS3: true }, config); + expect(res.warnings.length).toEqual(0); + expect(res.errors.length).toEqual(0); + }); + + it('should not complain for a valid combined schema where one schema is an array with items defined as ref', function() { + const spec = { + paths: { + '/stuff': { + get: { + summary: 'list stuff', + operationId: 'listStuff', + responses: { + 200: { + description: 'successful operation', + content: { + 'application/json': { + schema: { + type: 'array', + items: { + $ref: '#/components/schemas/ListStuffResponseModel' + } + } + } + } + } + } + } + } + } + }; + + const res = validate({ jsSpec: spec, isOAS3: true }, config); + expect(res.warnings.length).toEqual(0); + expect(res.errors.length).toEqual(0); + }); + it('should complain about an inline schema', function() { const spec = { paths: {