Skip to content

Commit f4cfcf1

Browse files
dpopp07dpopp07
committed
fix(ibm-api-symmetry): handle dictionaries and top-level schemas
This rule was subject to both false positives, as it flagged any schema with a dictionary property (even when inappropriate), and false negatives, as it did not check top-level schemas, only schema properties. This fix resolves both issues and cleans up the rule logic on the whole. Signed-off-by: Dustin Popp <[email protected]>
1 parent 6709f07 commit f4cfcf1

File tree

2 files changed

+362
-72
lines changed

2 files changed

+362
-72
lines changed

‎packages/ruleset/src/functions/api-symmetry.js‎

Lines changed: 117 additions & 71 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,11 @@
11
/**
2-
* Copyright 2023 - 2024 IBM Corporation.
2+
* Copyright 2023 - 2025 IBM Corporation.
33
* SPDX-License-Identifier: Apache2.0
44
*/
55

66
const {
77
getSchemaType,
88
isObject,
9-
isObjectSchema,
109
isArraySchema,
1110
schemaHasConstraint,
1211
schemaLooselyHasConstraint,
@@ -89,7 +88,7 @@ function checkApiForSymmetry(apidef, nodes) {
8988
['Summary', 'Prototype', 'Patch'].forEach(variantType => {
9089
const variantSchemaName = `${canonicalSchemaName}${variantType}`;
9190
const variantSchema = apidef.components.schemas[`${variantSchemaName}`];
92-
if (variantSchema && isObjectSchema(variantSchema)) {
91+
if (variantSchema && isObject(variantSchema)) {
9392
logger.info(
9493
`${ruleId}: checking variant schema ${variantSchemaName} against canonical schema ${canonicalSchemaName}`
9594
);
@@ -128,9 +127,11 @@ function checkApiForSymmetry(apidef, nodes) {
128127
/**
129128
* Determine if the variant schema is indeed a proper graph fragment of the
130129
* canonical schema, using the following conditions:
131-
* - The variant does not define additional or pattern properties
132130
* - The variant does not define any properties that do not exist on the
133131
* canonical schema (exists = present and has same type)
132+
* - The variant does not define any nested schemas (including those defined
133+
* by arrays or dictionaries) that aren't themselves graph fragments of
134+
* the corresponding canonical schema.
134135
* - The variant omits at least one property from the canonical schema
135136
*
136137
* @param {object} variant - the variant schema to check
@@ -163,103 +164,148 @@ function checkForGraphFragmentPattern(
163164
);
164165
let result = true;
165166

166-
// A variant schema cannot allow extraneous properties and still be
167-
// an explicit graph fragment of the canonical schema.
168-
if (variant.additionalProperties) {
167+
// Check for simple type equivalency - if the types are not the same,
168+
// the graph fragment pattern is violated.
169+
if (
170+
!fromApplicator &&
171+
!canonicalSchemaMeetsConstraint(
172+
canonical,
173+
canonicalPath,
174+
schemaFinder,
175+
schema => getSchemaType(variant) === getSchemaType(schema)
176+
)
177+
) {
169178
logger.info(
170-
`${ruleId}: schema defines 'additionalProperties' - it is not a proper graph fragment`
179+
`${ruleId}: variant and canonical schemas are different types`
171180
);
172181
result = false;
173182
}
174-
if (variant.patternProperties) {
183+
184+
// Ensure list schemas also maintain a graph fragment structure.
185+
if (
186+
isObject(variant.items) &&
187+
isArraySchema(variant) &&
188+
!canonicalSchemaMeetsConstraint(
189+
canonical,
190+
canonicalPath,
191+
schemaFinder,
192+
(schema, path) =>
193+
isObject(schema.items) &&
194+
isGraphFragment(
195+
variant.items,
196+
schema.items,
197+
[...path, 'items'],
198+
false
199+
)
200+
)
201+
) {
175202
logger.info(
176-
`${ruleId}: schema defines 'patternProperties' - it is not a proper graph fragment`
203+
`${ruleId}: variant is array with schema that is not a graph fragment of canonical items schema`
204+
);
205+
result = false;
206+
}
207+
208+
// Ensure dictionary schemas also maintain a graph fragment structure
209+
// (additional properties).
210+
if (
211+
variant.additionalProperties &&
212+
!canonicalSchemaMeetsConstraint(
213+
canonical,
214+
canonicalPath,
215+
schemaFinder,
216+
(schema, path) =>
217+
schema.additionalProperties &&
218+
isGraphFragment(
219+
variant.additionalProperties,
220+
schema.additionalProperties,
221+
[...path, 'additionalProperties'],
222+
false
223+
)
224+
)
225+
) {
226+
logger.info(
227+
`${ruleId}: variant is dictionary with an additionalProperties schema that is not a graph fragment of canonical`
228+
);
229+
result = false;
230+
}
231+
232+
// Ensure dictionary schemas also maintain a graph fragment structure
233+
// (pattern properties).
234+
if (
235+
isObject(variant.patternProperties) &&
236+
!canonicalSchemaMeetsConstraint(
237+
canonical,
238+
canonicalPath,
239+
schemaFinder,
240+
(schema, path) =>
241+
isObject(schema.patternProperties) &&
242+
// This is a little convoluted but it is enforcing that
243+
// 1) every pattern in the variant schema is also in canonical
244+
// schema, and
245+
// 2) every patterned schema in the variant must be a graph fragment
246+
// of at least one patterned schema in the canonical schema.
247+
Object.entries(variant.patternProperties).every(
248+
([variantPattern, variantPatternSchema]) =>
249+
Object.keys(schema.patternProperties).includes(variantPattern) &&
250+
Object.entries(schema.patternProperties).some(
251+
([canonPattern, canonPatternSchema]) =>
252+
isGraphFragment(
253+
variantPatternSchema,
254+
canonPatternSchema,
255+
[...path, 'patternProperties', canonPattern],
256+
false
257+
)
258+
)
259+
)
260+
)
261+
) {
262+
logger.info(
263+
`${ruleId}: variant is dictionary with a patternProperties schema that is not a graph fragment of canonical`
177264
);
178265
result = false;
179266
}
180267

181268
// If the variant schema (or sub-schema) has properties, ensure that each
182269
// property is defined *somewhere* on the corresponding canonical schema
183-
// (or sub-schema) and has the same, specific type.
270+
// (or sub-schema) and ensure it is also a valid graph fragment of the
271+
// corresponding property in the canonical schema.
184272
//
185-
// We use a custom, looser contraint-checking function here because it is
273+
// We use a looser contraint-checking function here because it is
186274
// sufficient for "one of" or "any of" the canonical schemas to define the
187275
// property defined on the variant schema, and we need to resolve reference
188276
// schemas on the fly each time we check the canonical schema for a constraint.
189277
if (isObject(variant.properties)) {
190278
for (const [name, prop] of Object.entries(variant.properties)) {
279+
let propExistsSomewhere = false;
280+
191281
const valid = canonicalSchemaMeetsConstraint(
192282
canonical,
193283
canonicalPath,
194284
schemaFinder,
195-
c =>
196-
'properties' in c &&
197-
isObject(c.properties[name]) &&
198-
getSchemaType(c.properties[name]) === getSchemaType(prop)
199-
);
285+
(schema, path) => {
286+
const exists =
287+
'properties' in schema && isObject(schema.properties[name]);
288+
propExistsSomewhere = propExistsSomewhere || exists;
200289

201-
// Note: Prototype schemas are allowed to define writeOnly properties
202-
// that don't exist on the canonical schema.
203-
if (!valid && !(considerWriteOnly && prop.writeOnly)) {
204-
logger.info(
205-
`${ruleId}: property '${name}' does not exist on the canonical schema`
206-
);
207-
result = false;
208-
}
209-
210-
// Ensure nested schemas are also graph fragments of the corresponding
211-
// nested schemas in the canonical schema.
212-
if (
213-
valid &&
214-
isObjectSchema(prop) &&
215-
!canonicalSchemaMeetsConstraint(
216-
canonical,
217-
canonicalPath,
218-
schemaFinder,
219-
(schema, path) =>
220-
// Note that these first two conditions are guaranteed to be met at
221-
// least once by the first call to `canonicalSchemaMeetsConstraint`
222-
'properties' in schema &&
223-
isObject(schema.properties[name]) &&
290+
return (
291+
exists &&
224292
isGraphFragment(
225293
prop,
226294
schema.properties[name],
227295
[...path, 'properties', name],
228296
false
229297
)
230-
)
231-
) {
232-
logger.info(
233-
`${ruleId}: nested object property ${name} is not a graph fragment of canonical property ${name}`
234-
);
235-
result = false;
236-
}
298+
);
299+
}
300+
);
237301

238-
// Ensure lists of schemas also maintain a graph fragment structure.
239-
if (
240-
valid &&
241-
isArraySchema(prop) &&
242-
isObjectSchema(prop.items) &&
243-
!canonicalSchemaMeetsConstraint(
244-
canonical,
245-
canonicalPath,
246-
schemaFinder,
247-
(schema, path) =>
248-
// Note that these first two conditions are guaranteed to be met at
249-
// least once by the first call to `canonicalSchemaMeetsConstraint`
250-
'properties' in schema &&
251-
isObject(schema.properties[name]) &&
252-
isObject(schema.properties[name].items) &&
253-
isGraphFragment(
254-
prop.items,
255-
schema.properties[name].items,
256-
[...path, 'properties', name, 'items'],
257-
false
258-
)
259-
)
260-
) {
302+
// Note: Prototype schemas are allowed to define writeOnly properties
303+
// that don't exist on the canonical schema.
304+
if (!valid && !(considerWriteOnly && prop.writeOnly)) {
261305
logger.info(
262-
`${ruleId}: array property ${name} items schema is not a graph fragment of canonical property ${name} items schema`
306+
propExistsSomewhere
307+
? `${ruleId}: nested object property ${name} is not a graph fragment of canonical property ${name}`
308+
: `${ruleId}: property '${name}' does not exist on the canonical schema`
263309
);
264310
result = false;
265311
}

0 commit comments

Comments
 (0)