Skip to content

Commit

Permalink
fix(ibm-well-defined-dictionaries): flag dictionaries with dictionary…
Browse files Browse the repository at this point in the history 
…-type values

The API Handbook states that the values for dictionaries should not themselves
be dictionaries (no dictionaries of dictionaries). Previously, this rule would
not catch that scenario. Now, it will.

Additionally, more robust logic is used to check for the presence of ambiguous
dictionaries. The old logic would miss flagging valid violations in complex,
composed schemas without the fix.

Signed-off-by: Dustin Popp <dpopp07@gmail.com>
Co-authored-by: Phil Adams <phil_adams@us.ibm.com>
  • Loading branch information
@dpopp07 @padamstx
dpopp07 and padamstx committed Nov 15, 2024
1 parent 8ae0e07 commit 7a4796a
Show file tree
Hide file tree
Showing 5 changed files with 247 additions and 49 deletions.
2 changes: 1 addition & 1 deletion .secrets.baseline
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
"files": "package-lock.json|^.secrets.baseline$",
"lines": null
},
"generated_at": "2024-11-08T16:48:42Z",
"generated_at": "2024-11-14T19:33:14Z",
"plugins_used": [
{
"name": "AWSKeyDetector"
Expand Down
2 changes: 1 addition & 1 deletion docs/ibm-cloud-rules.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7262,7 +7262,7 @@ paths:
Dictionaries are defined as object type schemas that have variable key names. They are distinct from model types,
which are objects with pre-defined properties. A schema must not define both concrete properties and variable key names.
Practically, this means a schema must explicitly define a `properties` object or an `additionalProperties` schema, but not both.
If used, the `additionalProperties` schema must define a concrete type. See the <a href="https://cloud.ibm.com/docs/api-handbook?topic=api-handbook-types">IBM Cloud API Handbook documentation on types</a> for more info.
If used, the `additionalProperties` schema must define a concrete type. The concrete type of the values must not be a dictionary itself. See the <a href="https://cloud.ibm.com/docs/api-handbook?topic=api-handbook-types">IBM Cloud API Handbook documentation on types</a> for more info.
</td>
</tr>
<tr>
Expand Down
36 changes: 2 additions & 34 deletions packages/ruleset/src/functions/api-symmetry.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ const {
isObjectSchema,
isArraySchema,
schemaHasConstraint,
schemaLooselyHasConstraint,
} = require('@ibm-cloud/openapi-ruleset-utilities');

const {
Expand Down Expand Up @@ -310,7 +311,7 @@ function checkForGraphFragmentPattern(
for (const [name, prop] of Object.entries(c.properties)) {
logger.debug(`${ruleId}: checking canonical property '${name}'`);

const included = schemaHasLooseConstraint(
const included = schemaLooselyHasConstraint(
variant,
s =>
'properties' in s &&
Expand Down Expand Up @@ -439,36 +440,3 @@ function canonicalSchemaMeetsConstraint(

return false;
}

/**
* This function is a looser adaptation of the "schemaHasConstraint()" function in the utilities package.
* Here we process oneOf and anyOf lists the same as allOf, where we return true if one (or more)
* of the oneOf/anyOf elements has the constraint (rather than all of the elements).
*/
function schemaHasLooseConstraint(schema, hasConstraint) {
if (!isObject(schema)) {
return false;
}

if (hasConstraint(schema)) {
return true;
}

const anySchemaHasConstraintReducer = (previousResult, currentSchema) => {
return (
previousResult || schemaHasLooseConstraint(currentSchema, hasConstraint)
);
};

for (const applicator of ['allOf', 'oneOf', 'anyOf']) {
if (
Array.isArray(schema[applicator]) &&
schema[applicator].length > 0 &&
schema[applicator].reduce(anySchemaHasConstraintReducer, false)
) {
return true;
}
}

return false;
}
36 changes: 35 additions & 1 deletion packages/ruleset/src/functions/well-defined-dictionaries.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ const {
isObject,
isObjectSchema,
schemaHasConstraint,
schemaLooselyHasConstraint,
validateNestedSchemas,
} = require('@ibm-cloud/openapi-ruleset-utilities');
const { LoggerFactory } = require('../utils');
Expand All @@ -29,6 +30,12 @@ function wellDefinedDictionaries(schema, path) {
return [];
}

// We will flag dictionaries of dictionaries, so we can skip
// providing guidance for directly nested dictionaries.
if (path.at(-1) === 'additionalProperties') {
return [];
}

logger.debug(
`${ruleId}: checking object schema at location: ${path.join('.')}`
);
Expand Down Expand Up @@ -78,6 +85,15 @@ function wellDefinedDictionaries(schema, path) {
});
}

// The Handbook guidelines state that dictionary types
// should not be dictionaries themselves.
if (schemaHasConstraint(schema, isDictionaryOfDictionaries)) {
errors.push({
message: 'Dictionaries must not have values that are also dictionaries.',
path,
});
}

return errors;
}

Expand All @@ -90,7 +106,25 @@ function isAmbiguousDictionary(schema) {
return false;
}

// additionalProperties must be an object (not a boolean) value
// and must define a `type` field in order to be considered an
// unambiguous dictionary.
return (
!isObject(schema.additionalProperties) || !schema.additionalProperties.type
!isObject(schema.additionalProperties) ||
!schemaDefinesField(schema.additionalProperties, 'type')
);
}

function isDictionaryOfDictionaries(schema) {
if (!isObject(schema.additionalProperties)) {
return false;
}

// We don't want any schema that may define the dictionary values
// to also be a dictionary, so we use a looser constraint that
// checks against any oneOf/anyOf schema.
return schemaLooselyHasConstraint(
schema.additionalProperties,
s => !!s['additionalProperties']
);
}
220 changes: 208 additions & 12 deletions packages/ruleset/test/rules/well-defined-dictionaries.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ const {
} = require('../test-utils');

const rule = wellDefinedDictionaries;
const ruleId = 'ibm-valid-path-segments';
const ruleId = 'ibm-well-defined-dictionaries';
const expectedSeverity = severityCodes.warning;

describe(`Spectral rule: ${ruleId}`, () => {
Expand All @@ -39,17 +39,25 @@ describe(`Spectral rule: ${ruleId}`, () => {
expect(results).toHaveLength(0);
});

it('Includes a dictionary that nests a well-defined dictionary with scalar values', async () => {
it('Includes a well-defined dictionary with composed scalar values', async () => {
const testDocument = makeCopy(rootDocument);

testDocument.components.schemas.Movie.properties.metadata = {
description: 'a dictionary mapping keys to string values',
description: 'a dictionary mapping keys to integer values',
type: 'object',
additionalProperties: {
type: 'object',
additionalProperties: {
type: 'string',
},
oneOf: [
{
type: 'integer',
minimum: 0,
maximum: 10,
},
{
type: 'integer',
minimum: 20,
maximum: 30,
},
],
},
};

Expand Down Expand Up @@ -84,6 +92,29 @@ describe(`Spectral rule: ${ruleId}`, () => {
const results = await testRule(ruleId, rule, testDocument);
expect(results).toHaveLength(0);
});

it('Includes a well-defined dictionary of models that include a dictionary property', async () => {
const testDocument = makeCopy(rootDocument);

testDocument.components.schemas.Movie.properties.metadata = {
description: 'a dictionary mapping keys to string values',
type: 'object',
additionalProperties: {
type: 'object',
properties: {
nested: {
type: 'object',
additionalProperties: {
type: 'string',
},
},
},
},
};

const results = await testRule(ruleId, rule, testDocument);
expect(results).toHaveLength(0);
});
});

describe('Should yield errors', () => {
Expand Down Expand Up @@ -204,16 +235,16 @@ describe(`Spectral rule: ${ruleId}`, () => {
expect(results).toHaveLength(4);

const expectedRulePaths = [
'paths./v1/movies.get.responses.200.content.application/json.schema.allOf.1.properties.movies.items.properties.metadata.additionalProperties',
'paths./v1/movies.post.responses.201.content.application/json.schema.properties.metadata.additionalProperties',
'paths./v1/movies/{movie_id}.get.responses.200.content.application/json.schema.properties.metadata.additionalProperties',
'paths./v1/movies/{movie_id}.put.responses.200.content.application/json.schema.properties.metadata.additionalProperties',
'paths./v1/movies.get.responses.200.content.application/json.schema.allOf.1.properties.movies.items.properties.metadata',
'paths./v1/movies.post.responses.201.content.application/json.schema.properties.metadata',
'paths./v1/movies/{movie_id}.get.responses.200.content.application/json.schema.properties.metadata',
'paths./v1/movies/{movie_id}.put.responses.200.content.application/json.schema.properties.metadata',
];

for (const i in results) {
expect(results[i].code).toBe(ruleId);
expect(results[i].message).toMatch(
'Dictionary schemas must have a single, well-defined value type in `additionalProperties`'
'Dictionaries must not have values that are also dictionaries.'
);
expect(results[i].severity).toBe(expectedSeverity);
expect(results[i].path.join('.')).toBe(expectedRulePaths[i]);
Expand Down Expand Up @@ -247,5 +278,170 @@ describe(`Spectral rule: ${ruleId}`, () => {
expect(results[i].path.join('.')).toBe(expectedPaths[i]);
}
});

it('Includes a dictionary of dictionaries, which is not allowed', async () => {
const testDocument = makeCopy(rootDocument);

testDocument.components.schemas.Movie.properties.metadata = {
description: 'a dictionary with no definition',
type: 'object',
additionalProperties: {
type: 'object',
additionalProperties: {
type: 'string',
},
},
};

const results = await testRule(ruleId, rule, testDocument);
expect(results).toHaveLength(4);

for (const i in results) {
expect(results[i].code).toBe(ruleId);
expect(results[i].message).toMatch(
'Dictionaries must not have values that are also dictionaries.'
);
expect(results[i].severity).toBe(expectedSeverity);
expect(results[i].path.join('.')).toBe(expectedPaths[i]);
}
});

it('Flags dictionary of dictionaries but ignores dictionary value issue', async () => {
const testDocument = makeCopy(rootDocument);

testDocument.components.schemas.Movie.properties.metadata = {
description: 'a dictionary with no definition',
type: 'object',
additionalProperties: {
type: 'object',
additionalProperties: true,
},
};

const results = await testRule(ruleId, rule, testDocument);
expect(results).toHaveLength(4);

for (const i in results) {
expect(results[i].code).toBe(ruleId);
expect(results[i].message).toMatch(
'Dictionaries must not have values that are also dictionaries.'
);
expect(results[i].severity).toBe(expectedSeverity);
expect(results[i].path.join('.')).toBe(expectedPaths[i]);
}
});

it('Includes a composed dictionary of dictionaries, which is not allowed', async () => {
const testDocument = makeCopy(rootDocument);

testDocument.components.schemas.Movie.properties.metadata = {
description: 'a dictionary with no definition',
type: 'object',
additionalProperties: {
type: 'object',
allOf: [
{
additionalProperties: {
type: 'string',
},
},
{
properties: {
name: {
type: 'string',
},
},
},
],
},
};

const results = await testRule(ruleId, rule, testDocument);
expect(results).toHaveLength(4);

for (const i in results) {
expect(results[i].code).toBe(ruleId);
expect(results[i].message).toMatch(
'Dictionaries must not have values that are also dictionaries.'
);
expect(results[i].severity).toBe(expectedSeverity);
expect(results[i].path.join('.')).toBe(expectedPaths[i]);
}
});

it('Includes a single oneOf element that creates a nested dictionary, which is not allowed', async () => {
const testDocument = makeCopy(rootDocument);

testDocument.components.schemas.Movie.properties.metadata = {
description: 'a dictionary with no definition',
type: 'object',
additionalProperties: {
type: 'object',
oneOf: [
{
additionalProperties: {
type: 'string',
},
},
{
properties: {
name: {
type: 'string',
},
},
},
],
},
};

const results = await testRule(ruleId, rule, testDocument);
expect(results).toHaveLength(4);

for (const i in results) {
expect(results[i].code).toBe(ruleId);
expect(results[i].message).toMatch(
'Dictionaries must not have values that are also dictionaries.'
);
expect(results[i].severity).toBe(expectedSeverity);
expect(results[i].path.join('.')).toBe(expectedPaths[i]);
}
});

it('Includes a well-defined dictionary of models that include a poorly-defined dictionary property', async () => {
const testDocument = makeCopy(rootDocument);

testDocument.components.schemas.Movie.properties.metadata = {
description: 'a dictionary mapping keys to string values',
type: 'object',
additionalProperties: {
type: 'object',
properties: {
nested: {
type: 'object',
additionalProperties: true,
},
},
},
};

const results = await testRule(ruleId, rule, testDocument);
expect(results).toHaveLength(4);

const expectedRulePaths = [
'paths./v1/movies.get.responses.200.content.application/json.schema.allOf.1.properties.movies.items.properties.metadata.additionalProperties.properties.nested',
'paths./v1/movies.post.responses.201.content.application/json.schema.properties.metadata.additionalProperties.properties.nested',
'paths./v1/movies/{movie_id}.get.responses.200.content.application/json.schema.properties.metadata.additionalProperties.properties.nested',
'paths./v1/movies/{movie_id}.put.responses.200.content.application/json.schema.properties.metadata.additionalProperties.properties.nested',
];

for (const i in results) {
expect(results[i].code).toBe(ruleId);
expect(results[i].message).toMatch(
'Dictionary schemas must have a single, well-defined value type in `additionalProperties`'
);
expect(results[i].severity).toBe(expectedSeverity);
expect(results[i].path.join('.')).toBe(expectedRulePaths[i]);
}
});
});
});

0 comments on commit 7a4796a

Please sign in to comment.