From ebdd6c63d6504ec9dac0e36b52479b3fa652b36e Mon Sep 17 00:00:00 2001 From: Dustin Popp Date: Wed, 4 Sep 2024 10:42:57 -0500 Subject: [PATCH] fix(ibm-accept-and-return-models): exempt non-object schemas When checking that request/response bodies are defined as models, we don't need to check schemas that aren't objects. These schemas should be objects, but there are other rules that will catch that and it's confusing for the validator to tell users they need the "properties" field on a non-object schema. Signed-off-by: Dustin Popp --- .../src/functions/accept-and-return-models.js | 13 +++++++++++- .../test/accept-and-return-models.test.js | 21 +++++++++++++++++-- .../tests/expected-output.test.js | 12 +++++------ .../tests/option-handling.test.js | 14 ++++--------- 4 files changed, 40 insertions(+), 20 deletions(-) diff --git a/packages/ruleset/src/functions/accept-and-return-models.js b/packages/ruleset/src/functions/accept-and-return-models.js index d0e8f43e0..3b6ebfccd 100644 --- a/packages/ruleset/src/functions/accept-and-return-models.js +++ b/packages/ruleset/src/functions/accept-and-return-models.js @@ -5,6 +5,7 @@ const { isObject, + isObjectSchema, schemaHasConstraint, } = require('@ibm-cloud/openapi-ruleset-utilities'); const { supportsJsonContent, LoggerFactory } = require('../utils'); @@ -17,6 +18,7 @@ let logger; * presence of the following other rules: * * ibm-operation-responses - all operations define a response + * ibm-no-array-responses - response bodies are not arrays * ibm-requestbody-is-object - JSON request bodies are object schemas * ibm-request-and-response content - request and response bodies define content * ibm-well-defined-dictionaries - additional properties aren't mixed with static properties @@ -51,6 +53,15 @@ function checkForProperties(schema, path) { return []; } + // Only check schemas that are already defined as an "object" type. + // Non-object request/response bodies are checked for elsewhere. + if (!isObjectSchema(schema)) { + logger.debug( + `${ruleId}: skipping non-object schema at location: ${path.join('.')}` + ); + return []; + } + if (!schemaHasConstraint(schema, s => schemaDefinesProperties(s))) { logger.debug( `${ruleId}: No properties found in schema at location: ${path.join('.')}` @@ -58,7 +69,7 @@ function checkForProperties(schema, path) { return [ { message: - 'Request and response bodies must include fields defined in `properties`', + 'Request and response bodies must be models - their schemas must define `properties`', path, }, ]; diff --git a/packages/ruleset/test/accept-and-return-models.test.js b/packages/ruleset/test/accept-and-return-models.test.js index 946e7f9b3..687dfb4a4 100644 --- a/packages/ruleset/test/accept-and-return-models.test.js +++ b/packages/ruleset/test/accept-and-return-models.test.js @@ -10,7 +10,7 @@ const rule = acceptAndReturnModels; const ruleId = 'ibm-accept-and-return-models'; const expectedSeverity = severityCodes.error; const expectedMessage = - 'Request and response bodies must include fields defined in `properties`'; + 'Request and response bodies must be models - their schemas must define `properties`'; // To enable debug logging in the rule function, copy this statement to an it() block: // LoggerFactory.getInstance().addLoggerSetting(ruleId, 'debug'); @@ -24,7 +24,7 @@ describe(`Spectral rule: ${ruleId}`, () => { expect(results).toHaveLength(0); }); - // non-JSON content, captured in root doc but might as well be explicit !!! + // Non-JSON content is captured in the root doc but adding this test to be explicit. it('Content is non-JSON', async () => { const testDocument = makeCopy(rootDocument); @@ -39,6 +39,23 @@ describe(`Spectral rule: ${ruleId}`, () => { const results = await testRule(ruleId, rule, testDocument); expect(results).toHaveLength(0); }); + + it('Content is not an object', async () => { + const testDocument = makeCopy(rootDocument); + + testDocument.paths['/v1/movies'].post.requestBody.content[ + 'application/json' + ].schema = { + type: 'array', + description: 'should be an object but that is caught elsewhere', + items: { + type: 'string', + }, + }; + + const results = await testRule(ruleId, rule, testDocument); + expect(results).toHaveLength(0); + }); }); describe('Should yield errors', () => { diff --git a/packages/validator/test/cli-validator/tests/expected-output.test.js b/packages/validator/test/cli-validator/tests/expected-output.test.js index a5f041a25..4c3c41a1b 100644 --- a/packages/validator/test/cli-validator/tests/expected-output.test.js +++ b/packages/validator/test/cli-validator/tests/expected-output.test.js @@ -86,26 +86,24 @@ describe('Expected output tests', function () { expect(capturedText[errorStart + 5].match(/\S+/g)[2]).toEqual('52'); expect(capturedText[errorStart + 10].match(/\S+/g)[2]).toEqual('96'); expect(capturedText[errorStart + 15].match(/\S+/g)[2]).toEqual('103'); - expect(capturedText[errorStart + 20].match(/\S+/g)[2]).toEqual('103'); - // Note: final number in this list captured below. // Specifically verify that the no-$ref-siblings error occurred. // We do this because this rule is inherited from Spectral's oas ruleset, // but we modify the rule definition in ibmoas.js so that it is run // against both OpenAPI 3.0 and 3.1 documents. - expect(capturedText[errorStart + 22].split(':')[1].trim()).toEqual( + expect(capturedText[errorStart + 17].split(':')[1].trim()).toEqual( '$ref must not be placed next to any other properties' ); - expect(capturedText[errorStart + 23].split(':')[1].trim()).toEqual( + expect(capturedText[errorStart + 18].split(':')[1].trim()).toEqual( 'no-$ref-siblings' ); - expect(capturedText[errorStart + 24].split(':')[1].trim()).toEqual( + expect(capturedText[errorStart + 19].split(':')[1].trim()).toEqual( 'components.schemas.Pet.properties.category.description' ); - expect(capturedText[errorStart + 25].match(/\S+/g)[2]).toEqual('176'); + expect(capturedText[errorStart + 20].match(/\S+/g)[2]).toEqual('176'); // warnings - const warningStart = 30; + const warningStart = 25; expect(capturedText[warningStart + 5].match(/\S+/g)[2]).toEqual('22'); expect(capturedText[warningStart + 10].match(/\S+/g)[2]).toEqual('24'); expect(capturedText[warningStart + 15].match(/\S+/g)[2]).toEqual('40'); diff --git a/packages/validator/test/cli-validator/tests/option-handling.test.js b/packages/validator/test/cli-validator/tests/option-handling.test.js index 08a816c9a..b9772f42c 100644 --- a/packages/validator/test/cli-validator/tests/option-handling.test.js +++ b/packages/validator/test/cli-validator/tests/option-handling.test.js @@ -114,25 +114,19 @@ describe('cli tool - test option handling', function () { expect(sumSection).toBe(3); // totals - expect(capturedText[sumSection + 2].match(/\S+/g)[5]).toEqual('5'); + expect(capturedText[sumSection + 2].match(/\S+/g)[5]).toEqual('4'); expect(capturedText[sumSection + 3].match(/\S+/g)[5]).toEqual('29'); // errors const errorSection = 8; expect(capturedText[errorSection + 1].match(/\S+/g)[0]).toEqual('1'); - expect(capturedText[errorSection + 1].match(/\S+/g)[1]).toEqual('(20%)'); + expect(capturedText[errorSection + 1].match(/\S+/g)[1]).toEqual('(25%)'); expect(capturedText[errorSection + 2].match(/\S+/g)[0]).toEqual('2'); - expect(capturedText[errorSection + 2].match(/\S+/g)[1]).toEqual('(40%)'); - - expect(capturedText[errorSection + 3].match(/\S+/g)[0]).toEqual('1'); - expect(capturedText[errorSection + 3].match(/\S+/g)[1]).toEqual('(20%)'); - - expect(capturedText[errorSection + 4].match(/\S+/g)[0]).toEqual('1'); - expect(capturedText[errorSection + 4].match(/\S+/g)[1]).toEqual('(20%)'); + expect(capturedText[errorSection + 2].match(/\S+/g)[1]).toEqual('(50%)'); // warnings - const warningSection = 14; + const warningSection = 13; expect(capturedText[warningSection + 1].match(/\S+/g)[0]).toEqual('2'); expect(capturedText[warningSection + 1].match(/\S+/g)[1]).toEqual('(7%)');