Skip to content

fix(ibm-api-symmetry): handle dictionaries and top-level schemas #715

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jan 24, 2025
Merged

fix(ibm-api-symmetry): handle dictionaries and top-level schemas #715

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9d0986f
fix(ibm-api-symmetry): handle dictionaries and top-level schemas
dpopp07 Jan 9, 2025
2a68a5e
fix(ibm-api-symmetry): print info logs in coherent order
dpopp07 Jan 16, 2025
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
206 changes: 131 additions & 75 deletions packages/ruleset/src/functions/api-symmetry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
/**
* Copyright 2023 - 2024 IBM Corporation.
* Copyright 2023 - 2025 IBM Corporation.
* SPDX-License-Identifier: Apache2.0
*/

const {
getSchemaType,
isObject,
isObjectSchema,
isArraySchema,
schemaHasConstraint,
schemaLooselyHasConstraint,
Expand All @@ -23,6 +22,10 @@ const {
let ruleId;
let logger;

// The graph fragment check is depth-first. Use this stack to
// print the relevant info logs in a sane order.
const infoLogStack = [];

/**
* The implementation for this rule makes assumptions that are dependent on the
* presence of the following other rules:
Expand Down Expand Up @@ -89,7 +92,7 @@ function checkApiForSymmetry(apidef, nodes) {
['Summary', 'Prototype', 'Patch'].forEach(variantType => {
const variantSchemaName = `${canonicalSchemaName}${variantType}`;
const variantSchema = apidef.components.schemas[`${variantSchemaName}`];
if (variantSchema && isObjectSchema(variantSchema)) {
if (variantSchema && isObject(variantSchema)) {
logger.info(
`${ruleId}: checking variant schema ${variantSchemaName} against canonical schema ${canonicalSchemaName}`
);
Expand Down Expand Up @@ -128,9 +131,11 @@ function checkApiForSymmetry(apidef, nodes) {
/**
* Determine if the variant schema is indeed a proper graph fragment of the
* canonical schema, using the following conditions:
* - The variant does not define additional or pattern properties
* - The variant does not define any properties that do not exist on the
* canonical schema (exists = present and has same type)
* - The variant does not define any nested schemas (including those defined
* by arrays or dictionaries) that aren't themselves graph fragments of
* the corresponding canonical schema.
* - The variant omits at least one property from the canonical schema
*
* @param {object} variant - the variant schema to check
Expand Down Expand Up @@ -163,103 +168,148 @@ function checkForGraphFragmentPattern(
);
let result = true;

// A variant schema cannot allow extraneous properties and still be
// an explicit graph fragment of the canonical schema.
if (variant.additionalProperties) {
logger.info(
`${ruleId}: schema defines 'additionalProperties' - it is not a proper graph fragment`
// Check for simple type equivalency - if the types are not the same,
// the graph fragment pattern is violated.
if (
!fromApplicator &&
!canonicalSchemaMeetsConstraint(
canonical,
canonicalPath,
schemaFinder,
schema => getSchemaType(variant) === getSchemaType(schema)
)
) {
infoLogStack.push(
`${ruleId}: variant and canonical schemas are different types`
);
result = false;
}
if (variant.patternProperties) {
logger.info(
`${ruleId}: schema defines 'patternProperties' - it is not a proper graph fragment`

// Ensure list schemas also maintain a graph fragment structure.
if (
isObject(variant.items) &&
isArraySchema(variant) &&
!canonicalSchemaMeetsConstraint(
canonical,
canonicalPath,
schemaFinder,
(schema, path) =>
isObject(schema.items) &&
isGraphFragment(
variant.items,
schema.items,
[...path, 'items'],
false
)
)
) {
infoLogStack.push(
`${ruleId}: variant is array with schema that is not a graph fragment of canonical items schema`
);
result = false;
}

// Ensure dictionary schemas also maintain a graph fragment structure
// (additional properties).
if (
variant.additionalProperties &&
!canonicalSchemaMeetsConstraint(
canonical,
canonicalPath,
schemaFinder,
(schema, path) =>
schema.additionalProperties &&
isGraphFragment(
variant.additionalProperties,
schema.additionalProperties,
[...path, 'additionalProperties'],
false
)
)
) {
infoLogStack.push(
`${ruleId}: variant is dictionary with an additionalProperties schema that is not a graph fragment of canonical`
);
result = false;
}

// Ensure dictionary schemas also maintain a graph fragment structure
// (pattern properties).
if (
isObject(variant.patternProperties) &&
!canonicalSchemaMeetsConstraint(
canonical,
canonicalPath,
schemaFinder,
(schema, path) =>
isObject(schema.patternProperties) &&
// This is a little convoluted but it is enforcing that
// 1) every pattern in the variant schema is also in canonical
// schema, and
// 2) every patterned schema in the variant must be a graph fragment
// of at least one patterned schema in the canonical schema.
Object.entries(variant.patternProperties).every(
([variantPattern, variantPatternSchema]) =>
Object.keys(schema.patternProperties).includes(variantPattern) &&
Object.entries(schema.patternProperties).some(
([canonPattern, canonPatternSchema]) =>
isGraphFragment(
variantPatternSchema,
canonPatternSchema,
[...path, 'patternProperties', canonPattern],
false
)
)
)
)
) {
infoLogStack.push(
`${ruleId}: variant is dictionary with a patternProperties schema that is not a graph fragment of canonical`
);
result = false;
}

// If the variant schema (or sub-schema) has properties, ensure that each
// property is defined *somewhere* on the corresponding canonical schema
// (or sub-schema) and has the same, specific type.
// (or sub-schema) and ensure it is also a valid graph fragment of the
// corresponding property in the canonical schema.
//
// We use a custom, looser contraint-checking function here because it is
// We use a looser contraint-checking function here because it is
// sufficient for "one of" or "any of" the canonical schemas to define the
// property defined on the variant schema, and we need to resolve reference
// schemas on the fly each time we check the canonical schema for a constraint.
if (isObject(variant.properties)) {
for (const [name, prop] of Object.entries(variant.properties)) {
let propExistsSomewhere = false;

const valid = canonicalSchemaMeetsConstraint(
canonical,
canonicalPath,
schemaFinder,
c =>
'properties' in c &&
isObject(c.properties[name]) &&
getSchemaType(c.properties[name]) === getSchemaType(prop)
);
(schema, path) => {
const exists =
'properties' in schema && isObject(schema.properties[name]);
propExistsSomewhere = propExistsSomewhere || exists;

// Note: Prototype schemas are allowed to define writeOnly properties
// that don't exist on the canonical schema.
if (!valid && !(considerWriteOnly && prop.writeOnly)) {
logger.info(
`${ruleId}: property '${name}' does not exist on the canonical schema`
);
result = false;
}

// Ensure nested schemas are also graph fragments of the corresponding
// nested schemas in the canonical schema.
if (
valid &&
isObjectSchema(prop) &&
!canonicalSchemaMeetsConstraint(
canonical,
canonicalPath,
schemaFinder,
(schema, path) =>
// Note that these first two conditions are guaranteed to be met at
// least once by the first call to `canonicalSchemaMeetsConstraint`
'properties' in schema &&
isObject(schema.properties[name]) &&
return (
exists &&
isGraphFragment(
prop,
schema.properties[name],
[...path, 'properties', name],
false
)
)
) {
logger.info(
`${ruleId}: nested object property ${name} is not a graph fragment of canonical property ${name}`
);
result = false;
}
);
}
);

// Ensure lists of schemas also maintain a graph fragment structure.
if (
valid &&
isArraySchema(prop) &&
isObjectSchema(prop.items) &&
!canonicalSchemaMeetsConstraint(
canonical,
canonicalPath,
schemaFinder,
(schema, path) =>
// Note that these first two conditions are guaranteed to be met at
// least once by the first call to `canonicalSchemaMeetsConstraint`
'properties' in schema &&
isObject(schema.properties[name]) &&
isObject(schema.properties[name].items) &&
isGraphFragment(
prop.items,
schema.properties[name].items,
[...path, 'properties', name, 'items'],
false
)
)
) {
logger.info(
`${ruleId}: array property ${name} items schema is not a graph fragment of canonical property ${name} items schema`
// Note: Prototype schemas are allowed to define writeOnly properties
// that don't exist on the canonical schema.
if (!valid && !(considerWriteOnly && prop.writeOnly)) {
infoLogStack.push(
propExistsSomewhere
? `${ruleId}: nested object property ${name} is not a graph fragment of canonical property ${name}`
: `${ruleId}: property '${name}' does not exist on the canonical schema`
);
result = false;
}
Expand All @@ -279,7 +329,7 @@ function checkForGraphFragmentPattern(
true
)
) {
logger.info(
infoLogStack.push(
`${ruleId}: variant schema applicator '${applicator}' is not a graph fragment of the canonical schema`
);
result = false;
Expand Down Expand Up @@ -364,6 +414,12 @@ function checkForGraphFragmentPattern(
false
);

// Print the logs gathered within the `isGraphFragment` function,
// in reverse order - it will be coherent for the user.
while (infoLogStack.length) {
logger.info(infoLogStack.pop());
}

logger.debug(
`${ruleId}: isGraphFragment() returned [${variantIsGraphFragment},${variantOmitsProperties}]`
);
Expand Down
Loading
Loading