From 603e689ad8903f652c87e4ec911e8a2d39bdf0ad Mon Sep 17 00:00:00 2001 From: Dustin Popp Date: Mon, 16 Oct 2023 10:48:24 -0500 Subject: [PATCH] fix(api-symmetry): handle nested reference schemas (#639) If reference schemas were nested in composed properties on the canonical schema, the code would not resolve them the way it would for non-nested reference schemas. This moves the resolving code to the constraint checker, so that every schema and applicator sub-schema will be resolved if it is a reference. Signed-off-by: Dustin Popp --- .../ruleset/src/functions/api-symmetry.js | 102 +++++++++++------- packages/ruleset/test/api-symmetry.test.js | 56 ++++++++++ 2 files changed, 120 insertions(+), 38 deletions(-) diff --git a/packages/ruleset/src/functions/api-symmetry.js b/packages/ruleset/src/functions/api-symmetry.js index 6b6120495..dfdeaa525 100644 --- a/packages/ruleset/src/functions/api-symmetry.js +++ b/packages/ruleset/src/functions/api-symmetry.js @@ -145,60 +145,43 @@ function checkForGraphFragmentPattern( schemaFinder ) { let variantOmitsProperties = false; - let isValidGraphFragment = true; // The function is wrapped in a partial so that we can track `variantOmitsProperties` // across the entire schema without overriding it during recursive invocations of the // graph fragment checker. function isGraphFragment(variant, canonical, canonicalPath, fromApplicator) { + 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` ); - isValidGraphFragment = false; + result = false; } if (variant.patternProperties) { logger.info( `${ruleId}: schema defines 'patternProperties' - it is not a proper graph fragment` ); - isValidGraphFragment = false; - } - - // First check if the canonical version of the current schema is actually - // a reference to a "Reference" schema. This is allowed but we need to - // check for "graph-fragment-ness" using the canonical version of that - // Reference schema. - const schemaName = getSchemaNameAtPath( - canonicalPath.join('.'), - schemaFinder.refMap - ); - - // Use "slice" to remove the "Reference" appendix we guaranteed to be there. - // If we find the canonical schema, use it for the current function invocation, - // otherwise proceed as usual. - if ( - schemaName && - schemaName.endsWith('Reference') && - schemaFinder.allSchemas[schemaName.slice(0, -9)] - ) { - canonical = schemaFinder.allSchemas[schemaName.slice(0, -9)]; - canonicalPath = ['components', 'schemas', schemaName]; + 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. // - // 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. + // We use a custom, 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)) { - const valid = schemaHasLooserConstraint( + const valid = canonicalSchemaMeetsConstraint( canonical, + canonicalPath, + schemaFinder, c => 'properties' in c && isObject(c.properties[name]) && @@ -211,16 +194,20 @@ function checkForGraphFragmentPattern( logger.info( `${ruleId}: property '${name}' does not exist on the canonical schema` ); - isValidGraphFragment = false; + result = false; } // Ensure nested schemas are also graph fragments of the corresponding // nested schemas in the canonical schema. if ( isObjectSchema(prop) && - !schemaHasLooserConstraint( + !canonicalSchemaMeetsConstraint( canonical, + canonicalPath, + schemaFinder, s => + // Note that these first two conditions are guaranteed to be met at + // least once by the first call to `canonicalSchemaMeetsConstraint` 'properties' in s && isObject(s.properties[name]) && isGraphFragment( @@ -231,16 +218,20 @@ function checkForGraphFragmentPattern( ) ) ) { - isValidGraphFragment = false; + result = false; } // Ensure lists of schemas also maintain a graph fragment structure. if ( isArraySchema(prop) && isObjectSchema(prop.items) && - !schemaHasLooserConstraint( + !canonicalSchemaMeetsConstraint( canonical, + canonicalPath, + schemaFinder, s => + // Note that these first two conditions are guaranteed to be met at + // least once by the first call to `canonicalSchemaMeetsConstraint` 'properties' in s && isObject(s.properties[name]) && isObject(s.properties[name].items) && @@ -252,7 +243,7 @@ function checkForGraphFragmentPattern( ) ) ) { - isValidGraphFragment = false; + result = false; } } } @@ -270,7 +261,7 @@ function checkForGraphFragmentPattern( true ) ) { - isValidGraphFragment = false; + result = false; } }); @@ -284,6 +275,9 @@ function checkForGraphFragmentPattern( // // If we've already determined that the variant schema omits at least one // property, we don't need to re-perform this check anymore. + // + // Note: we don't bother checking for reference schemas here because there + // should be differences other than omitting non-reference properties. if (!fromApplicator && !variantOmitsProperties) { const variantIncludesAll = schemaHasConstraint(canonical, c => { @@ -314,7 +308,7 @@ function checkForGraphFragmentPattern( } } - return isValidGraphFragment; + return result; } // Invoke primary algorithm. @@ -340,11 +334,38 @@ function checkForGraphFragmentPattern( * sufficient for returning a `true` value, as opposed to all of them * needing the meet the constraint. */ -function schemaHasLooserConstraint(schema, hasConstraint) { +function canonicalSchemaMeetsConstraint( + schema, + path, + schemaFinder, + hasConstraint +) { if (!isObject(schema)) { return false; } + // First check if the canonical version of the current schema is actually + // a reference to a "Reference" schema. This is allowed but we need to + // check for "graph-fragment-ness" using the canonical version of that + // Reference schema. + const schemaName = getSchemaNameAtPath(path.join('.'), schemaFinder.refMap); + + // Use "slice" to remove the "Reference" appendix we guaranteed to be there. + // If we find the canonical schema, use it for the current function invocation, + // otherwise proceed as usual. + if ( + schemaName && + schemaName.endsWith('Reference') && + schemaFinder.allSchemas[schemaName.slice(0, -9)] + ) { + const canonicalVersion = schemaName.slice(0, -9); + logger.debug( + `Replacing reference schema ${schemaName} with canonical version ${canonicalVersion}` + ); + schema = schemaFinder.allSchemas[canonicalVersion]; + path = ['components', 'schemas', canonicalVersion]; + } + if (hasConstraint(schema)) { return true; } @@ -354,9 +375,14 @@ function schemaHasLooserConstraint(schema, hasConstraint) { Array.isArray(schema[applicator]) && schema[applicator].length > 0 && schema[applicator].reduce( - (previousResult, currentSchema) => + (previousResult, currentSchema, index) => previousResult || - schemaHasLooserConstraint(currentSchema, hasConstraint), + canonicalSchemaMeetsConstraint( + currentSchema, + [...path, applicator, index.toString()], + schemaFinder, + hasConstraint + ), false ) ) { diff --git a/packages/ruleset/test/api-symmetry.test.js b/packages/ruleset/test/api-symmetry.test.js index 267396691..8cf497b4b 100644 --- a/packages/ruleset/test/api-symmetry.test.js +++ b/packages/ruleset/test/api-symmetry.test.js @@ -222,6 +222,62 @@ describe(`Spectral rule: ${ruleId}`, () => { expect(results).toHaveLength(0); }); + it('prototype schema is compliant with canonical version of reference schema nested in composed schema', async () => { + const testDocument = makeCopy(rootDocument); + + testDocument.components.schemas.Actor = { + type: 'object', + properties: { + id: { type: 'string' }, + name: { type: 'string' }, + info: { + type: 'object', + properties: { + age: { + type: 'integer', + }, + }, + }, + }, + }; + testDocument.components.schemas.ActorReference = { + type: 'object', + properties: { + id: { type: 'string' }, + }, + }; + testDocument.components.schemas.ActorPrototype = { + type: 'object', + properties: { + name: { type: 'string' }, + info: { + oneOf: [ + { + type: 'object', + properties: { + age: { type: 'integer' }, + }, + }, + ], + }, + }, + }; + + testDocument.components.schemas.Movie.properties.lead = { + allOf: [ + { description: 'Lead actor in canonical context' }, + { $ref: '#/components/schemas/ActorReference' }, + ], + }; + + testDocument.components.schemas.MoviePrototype.properties.lead = { + $ref: '#/components/schemas/ActorPrototype', + }; + + const results = await testRule(ruleId, rule, testDocument); + expect(results).toHaveLength(0); + }); + it('patch schema with complex, composed structure is compliant', async () => { const testDocument = makeCopy(rootDocument); testDocument.components.schemas.Speakers = {