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-well-defined-dictionaries): include patternProperties in validation #713

Merged
merged 1 commit into from
Jan 10, 2025
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
2 changes: 1 addition & 1 deletion .secrets.baseline
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-12-19T16:14:03Z",
"generated_at": "2025-01-09T19:49:59Z",
"plugins_used": [
{
"name": "AWSKeyDetector"
Expand Down
4 changes: 2 additions & 2 deletions docs/ibm-cloud-rules.md
Original file line number Diff line number Diff line change
Expand Up @@ -7328,8 +7328,8 @@ paths:
This rule validates that any dictionary schemas are well defined and that all values share a single type.
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. 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.
Practically, this means a schema must explicitly define a `properties` object or an `(additional|pattern)Properties` schema, but not both.
Copy link
Member

Choose a reason for hiding this comment

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

I never realized that our API handbook was this opinionated on "properties" vs "additionalProperties" 😂

Copy link
Member Author

Choose a reason for hiding this comment

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

It's new-ish guidance 🙂 but good guidance, I think!

If used, the `(additional|pattern)Properties` 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
80 changes: 51 additions & 29 deletions packages/ruleset/src/functions/well-defined-dictionaries.js
Original file line number Diff line number Diff line change
@@ -1,20 +1,26 @@
/**
* Copyright 2024 IBM Corporation.
* Copyright 2024 - 2025 IBM Corporation.
* SPDX-License-Identifier: Apache2.0
*/

const {
isObject,
isObjectSchema,
schemaHasConstraint,
schemaLooselyHasConstraint,
validateNestedSchemas,
} = require('@ibm-cloud/openapi-ruleset-utilities');
const { LoggerFactory } = require('../utils');

let ruleId;
let logger;

/**
* The implementation for this rule makes assumptions that are dependent on the
* presence of the following other rules:
*
* - ibm-pattern-properties: patternProperties isn't empty or the wrong type
*/

module.exports = function (schema, _opts, context) {
if (!logger) {
ruleId = context.rule.name;
Expand All @@ -32,23 +38,23 @@ function wellDefinedDictionaries(schema, path) {

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

logger.debug(
`${ruleId}: checking object schema at location: ${path.join('.')}`
);

// Dictionaries should have additionalProperties defined on them.
// If the schema doesn't, make sure it has properties and then
// abandon the check.
if (!schemaDefinesField(schema, 'additionalProperties')) {
// Dictionaries should have additionalProperties or patternProperties
// defined on them. If the schema doesn't, make sure it has properties
// and then abandon the check.
if (!isDictionarySchema(schema)) {
if (!schemaDefinesField(schema, 'properties')) {
return [
{
message:
'Object schemas must define either properties, or additionalProperties with a concrete type',
'Object schemas must define either properties, or (additional/pattern)Properties with a concrete type',
path,
},
];
Expand Down Expand Up @@ -79,8 +85,7 @@ function wellDefinedDictionaries(schema, path) {
// more strict in the future but this meets our current purposes.
if (schemaHasConstraint(schema, isAmbiguousDictionary)) {
errors.push({
message:
'Dictionary schemas must have a single, well-defined value type in `additionalProperties`',
message: 'Dictionary schemas must have a single, well-defined value type',
path,
});
}
Expand All @@ -89,7 +94,7 @@ function wellDefinedDictionaries(schema, path) {
// should not be dictionaries themselves.
if (schemaHasConstraint(schema, isDictionaryOfDictionaries)) {
errors.push({
message: 'Dictionaries must not have values that are also dictionaries.',
message: 'Dictionaries must not have values that are also dictionaries',
path,
});
}
Expand All @@ -102,29 +107,46 @@ function schemaDefinesField(schema, field) {
}

function isAmbiguousDictionary(schema) {
if (!schema.additionalProperties) {
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) ||
!schemaDefinesField(schema.additionalProperties, 'type')
return dictionaryValuesHaveConstraint(
schema,
valueSchema =>
!isObject(valueSchema) || !schemaDefinesField(valueSchema, 'type')
);
}

function isDictionaryOfDictionaries(schema) {
if (!isObject(schema.additionalProperties)) {
return dictionaryValuesHaveConstraint(
schema,
valueSchema => isObject(valueSchema) && isDictionarySchema(valueSchema)
);
}

function dictionaryValuesHaveConstraint(schema, hasConstraint) {
return schemaHasConstraint(schema, s => {
if (s.additionalProperties !== undefined) {
return hasConstraint(s.additionalProperties);
}

if (s.patternProperties !== undefined) {
return Object.values(s.patternProperties).some(p => hasConstraint(p));
}

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']
// Check, *by path*, if the current schema is a dictionary value schema.
function isDictionaryValueSchema(path) {
return (
path.at(-1) === 'additionalProperties' ||
path.at(-2) === 'patternProperties'
);
}

// Check, *by object fields* if the current schema is a dictionary or not.
function isDictionarySchema(schema) {
return (
schemaDefinesField(schema, 'additionalProperties') ||
schemaDefinesField(schema, 'patternProperties')
);
}
Loading
Loading