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(ibm-accept-and-return-models): exempt non-object schemas #681

Merged
merged 1 commit into from
Sep 4, 2024
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
13 changes: 12 additions & 1 deletion packages/ruleset/src/functions/accept-and-return-models.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

const {
isObject,
isObjectSchema,
schemaHasConstraint,
} = require('@ibm-cloud/openapi-ruleset-utilities');
const { supportsJsonContent, LoggerFactory } = require('../utils');
Expand All @@ -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
Expand Down Expand Up @@ -51,14 +53,23 @@ 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('.')}`
);
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,
},
];
Expand Down
21 changes: 19 additions & 2 deletions packages/ruleset/test/accept-and-return-models.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand All @@ -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);

Expand All @@ -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', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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%)');

Expand Down