From 04da2d11484d03dd45006b5625f8e0c4abfe1623 Mon Sep 17 00:00:00 2001 From: Phil Adams Date: Mon, 3 Oct 2022 16:57:22 -0500 Subject: [PATCH 1/4] feat(composed-schema-restrictions): add new spectral-style rule This commit introduces the new spectral-style 'composed-schema-restrictions' rule, which will verify that oneOf and anyOf compositions comply with the restrictions imposed by the SDK generator. Signed-off-by: Phil Adams --- docs/ibm-cloud-rules.md | 92 +++ package-lock.json | 10 +- packages/ruleset/package.json | 1 + .../functions/composed-schema-restrictions.js | 327 +++++++++ packages/ruleset/src/functions/index.js | 1 + .../src/functions/inline-schema-rules.js | 18 +- packages/ruleset/src/ibm-oas.js | 1 + .../src/rules/composed-schema-restrictions.js | 16 + packages/ruleset/src/rules/index.js | 1 + packages/ruleset/src/utils/get-schema-type.js | 24 + packages/ruleset/src/utils/index.js | 1 - .../ruleset/src/utils/is-primitive-type.js | 13 - .../test/composed-schema-restrictions.test.js | 627 ++++++++++++++++++ .../ruleset/test/is-object-schema.test.js | 30 + .../ruleset/test/is-primitive-schema.test.js | 142 ++++ .../merge-allof-schema-properties.test.js | 2 +- 16 files changed, 1279 insertions(+), 27 deletions(-) create mode 100644 packages/ruleset/src/functions/composed-schema-restrictions.js create mode 100644 packages/ruleset/src/rules/composed-schema-restrictions.js delete mode 100644 packages/ruleset/src/utils/is-primitive-type.js create mode 100644 packages/ruleset/test/composed-schema-restrictions.test.js create mode 100644 packages/ruleset/test/is-primitive-schema.test.js diff --git a/docs/ibm-cloud-rules.md b/docs/ibm-cloud-rules.md index c4c892268..07833fb1d 100644 --- a/docs/ibm-cloud-rules.md +++ b/docs/ibm-cloud-rules.md @@ -37,6 +37,7 @@ which is delivered in the `@ibm-cloud/openapi-ruleset` NPM package. * [Rule: binary-schemas](#rule-binary-schemas) * [Rule: circular-refs](#rule-circular-refs) * [Rule: collection-array-property](#rule-collection-array-property) + * [Rule: composed-schema-restrictions](#rule-composed-schema-restrictions) * [Rule: consecutive-path-param-segments](#rule-consecutive-path-param-segments) * [Rule: content-entry-contains-schema](#rule-content-entry-contains-schema) * [Rule: content-entry-provided](#rule-content-entry-provided) @@ -178,6 +179,13 @@ within the operation's path string, which should also match the plural form of t oas2, oas3 +composed-schema-restrictions +error +Verifies that schema compositions involving oneOf and anyOf comply with the +restrictions imposed by the SDK generator. +oas3 + + consecutive-path-param-segments error Checks each path string in the API definition to detect the presence of two or more consecutive @@ -1321,6 +1329,90 @@ paths: +### Rule: composed-schema-restrictions + + + + + + + + + + + + + + + + + + + + + + + + + +
Rule id:composed-schema-restrictions
Description:This rule examines each schema that inclues a oneOf/anyOf composition and verifies that +it complies with the restrictions imposed by the SDK generator. The restrictions are: +
    +
  • Any object schema containing a oneOf/anyOf composition must include only object schemas within +the oneOf/anyOf list.
    • +
  • The union of the properties defined by the main schema and its +oneOf/anyOf sub-schemas is examined to detect any like-named properties defined by two or more of the schemas. +Like-named properties that are found to be of different types will trigger an error.
    • +
+
Severity:error
OAS Versions:oas3
Non-compliant example: +
+components:
+  schemas:
+    Foo:
+      description: A schema that violates the restrictions
+      type: object
+      oneOf:
+        - $ref: '#/components/schemas/SubSchema1
+        - $ref: '#/components/schemas/SubSchema2
+    SubSchema1:
+      properties:
+        common_property:
+          type: string
+        another_property:
+          type: integer
+    SubSchema2:
+      properties:
+        common_property:
+          type: integer       <<< incompatible type
+        some_other_property:
+          type: string
+
+
Compliant example: +
+components:
+  schemas:
+    Foo:
+      description: A schema that complies with the restrictions
+      type: object
+      oneOf:
+        - $ref: '#/components/schemas/SubSchema1
+        - $ref: '#/components/schemas/SubSchema2
+    SubSchema1:
+      properties:
+        common_property:
+          type: string
+        another_property:
+          type: string
+    SubSchema2:
+      properties:
+        common_property:
+          type: string
+        some_other_property:
+          type: string
+
+
+ + ### Rule: consecutive-path-param-segments diff --git a/package-lock.json b/package-lock.json index 5e4d62645..5d2fb24a1 100644 --- a/package-lock.json +++ b/package-lock.json @@ -12924,12 +12924,13 @@ }, "packages/ruleset": { "name": "@ibm-cloud/openapi-ruleset", - "version": "0.43.0", + "version": "0.44.0", "license": "Apache-2.0", "dependencies": { "@stoplight/spectral-formats": "^1.1.0", "@stoplight/spectral-functions": "^1.6.1", "@stoplight/spectral-rulesets": "^1.6.0", + "@stoplight/spectral-runtime": "^1.1.2", "lodash": "^4.17.21" }, "devDependencies": { @@ -12942,10 +12943,10 @@ }, "packages/validator": { "name": "ibm-openapi-validator", - "version": "0.94.0", + "version": "0.95.0", "license": "Apache-2.0", "dependencies": { - "@ibm-cloud/openapi-ruleset": "0.43.0", + "@ibm-cloud/openapi-ruleset": "0.44.0", "@stoplight/spectral-cli": "^6.4.2", "@stoplight/spectral-core": "^1.12.4", "@stoplight/spectral-parsers": "^1.0.1", @@ -13441,6 +13442,7 @@ "@stoplight/spectral-formats": "^1.1.0", "@stoplight/spectral-functions": "^1.6.1", "@stoplight/spectral-rulesets": "^1.6.0", + "@stoplight/spectral-runtime": "^1.1.2", "jest": "^27.4.5", "lodash": "^4.17.21" } @@ -16932,7 +16934,7 @@ "ibm-openapi-validator": { "version": "file:packages/validator", "requires": { - "@ibm-cloud/openapi-ruleset": "0.43.0", + "@ibm-cloud/openapi-ruleset": "0.44.0", "@stoplight/spectral-cli": "^6.4.2", "@stoplight/spectral-core": "^1.12.4", "@stoplight/spectral-parsers": "^1.0.1", diff --git a/packages/ruleset/package.json b/packages/ruleset/package.json index 75d9832dd..c5f7413db 100644 --- a/packages/ruleset/package.json +++ b/packages/ruleset/package.json @@ -17,6 +17,7 @@ "@stoplight/spectral-formats": "^1.1.0", "@stoplight/spectral-functions": "^1.6.1", "@stoplight/spectral-rulesets": "^1.6.0", + "@stoplight/spectral-runtime": "^1.1.2", "lodash": "^4.17.21" }, "devDependencies": { diff --git a/packages/ruleset/src/functions/composed-schema-restrictions.js b/packages/ruleset/src/functions/composed-schema-restrictions.js new file mode 100644 index 000000000..5a573c292 --- /dev/null +++ b/packages/ruleset/src/functions/composed-schema-restrictions.js @@ -0,0 +1,327 @@ +const { + isPrimitiveSchema, + validateSubschemas, + getCompositeSchemaAttribute, + getSchemaType, + isObjectSchema, + isArraySchema, + isObject, + mergeAllOfSchemaProperties, + SchemaType +} = require('../utils'); + +let unresolvedSpec; +module.exports = function(schema, opts, context) { + unresolvedSpec = context.document.parserResult.data; + return validateSubschemas(schema, context.path, composedSchemaRestrictions); +}; + +/** + * This function checks "schema" to make sure that if it's a composed schema + * using oneOf or anyOf that it complies with the restrictions imposed by the + * SDK generator. + * The specific checks that are performed here are: + * 1. Any schema that is a primitive or an array, or contains no oneOf or anyOf + * is skipped (automatically passes). + * 2. A schema's oneOf or anyOf list must contain only object schemas. + * Any non-object schema is rejected. + * 3. The union of the properties defined by the main schema and its + * oneOf/anyOf sub-schemas is examined to detect any duplicates. + * Duplicate properties with different types are rejected. + * + * @param {*} schema the schema to check + * @param {*} path the array of path segments indicating the location of "schema" within the API definition + * @returns an array containing the violations found or [] if no violations + */ +function composedSchemaRestrictions(schema, path) { + // We're mainly interested in object schemas within this validation rule. + // If "schema" is a primitive, then the SDK generator will unconditionally + // replace it with an equivalent schema without the oneOf/anyOf list, + // so nothing more to check for that scenario. + // If "schema" is an array schema, then it can't have an allOf/oneOf/anyOf list, + // so this scenario is not interesting either (the array's "items" field will be + // validated separately). + if (isPrimitiveSchema(schema) || isArraySchema(schema)) { + return []; + } + + // Next, we'll look for oneOf and then anyOf. Only one should be present. + let subSchemas = getCompositeSchemaAttribute(schema, 'oneOf'); + let fieldName = 'oneOf'; + if (!isNonemptyArray(subSchemas)) { + subSchemas = getCompositeSchemaAttribute(schema, 'anyOf'); + fieldName = 'anyOf'; + } + + // If we found neither oneOf nor anyOf, then bail out now. + if (!isNonemptyArray(subSchemas)) { + return []; + } + + // Next, we need to verify that it's oneOf/anyOf list complies. + return checkSchemaList(schema, subSchemas, fieldName, path); +} + +/** + * This function checks "schema" and it's oneOf/anyOf list "subSchemas" to verify that + * it complies with the generator restrictions around the use of oneOf/anyOf. + * @param {*} schema the main schema to check + * @param {*} subSchemas the oneOf or anyOf list belonging to "schema" + * @param {*} fieldName the name of the field containing "subSchemas" within "schema" ('oneOf' or 'anyOf') + * @param {*} path the array of path segments indicating the location of "schema" within the API definition + * @returns an array containing the violations found or [] if no violations + */ +function checkSchemaList(schema, subSchemas, fieldName, path) { + const errors = []; + + schema = mergeAllOfSchemaProperties(schema); + + // "targetProperties" will initially contain the properties defined in the main schema, + // then we'll add each property from the subSchemas to it to simulate the SDK generator's + // handling of the oneOf/anyOf list (hybrid hierarchy mode). + const targetProperties = isObject(schema.properties) + ? copyObject(schema.properties) + : {}; + + for (let i = 0; i < subSchemas.length; i++) { + const s = mergeAllOfSchemaProperties(subSchemas[i]); + + // "s" should be an object schema. If not, return an error. + if (!isObjectSchema(s)) { + errors.push({ + message: 'A schema within a oneOf/anyOf list must be an object schema', + path: [...path, fieldName, i.toString()] + }); + } else { + // "s" is an object schema as expected, so let's check each of its properties. + const properties = isObject(s.properties) ? s.properties : {}; + for (const [name, prop] of Object.entries(properties)) { + const targetProp = targetProperties[name]; + if (targetProp) { + // If "targetProperties" contains a like-named property, make sure the types are compatible. + // We can only guess at the paths associated with target and sub-schema properties + // due to the allOf processing that needs to be performed on each schema. + const propsAreCompatible = propsHaveCompatibleTypes( + targetProp, + [...path, 'properties', name], + prop, + [...path, fieldName, i, 'properties', name] + ); + if (!propsAreCompatible) { + const error = { + message: `Duplicate property with incompatible type defined in schema within a oneOf/anyOf list: ${name}`, + path: [...path, fieldName, i.toString()] + }; + errors.push(error); + } + } else { + // Property doesn't exist in main schema, so just add it. + targetProperties[name] = prop; + } + } + } + } + + return errors; +} + +/** + * Returns true iff "s" is an array with at least one element. + * @param {} s the object to check + * @returns boolean + */ +function isNonemptyArray(s) { + return Array.isArray(s) && s.length; +} + +/** + * Performs a rudimentary deep copy by simply marshalling, then unmarshalling "obj". + * @param {*} obj the object to copy + * @returns a deep copy of "obj" + */ +function copyObject(obj) { + return JSON.parse(JSON.stringify(obj)); +} + +/** + * This function is a wrapper around the "getSchemaType()" utility that maps + * the "ENUMERATION" type to be "STRING". We need to do this mapping to ensure that + * the type-compatibility checks will work correctly. + * @param {} s the schema + * @returns SchemaType enum value + */ +function getType(s) { + const schemaType = getSchemaType(s); + return schemaType === SchemaType.ENUMERATION ? SchemaType.STRING : schemaType; +} + +/** + * Returns true iff properties "p1" and "p2" have the same type. + * @param {*} p1 the first property to compare + * @param {*} p2 the second property to compare + * @returns boolean + */ +function propsHaveCompatibleTypes(p1, path1, p2, path2) { + if (isArraySchema(p1) && isArraySchema(p2)) { + return propsHaveCompatibleTypes(p1.items, [...path1, 'items'], p2.items, [ + ...path2, + 'items' + ]); + } + + const type1 = getType(p1); + const type2 = getType(p2); + + // If p1 and p2 are object schemas, then in order for them to be + // compatible in the context of this rule, they must have been defined + // with the same $ref within the original unresolved API definition. + // Unfortunately, we're using the resolved API def and so the $ref's have + // been resolved. To accommodate this, we'll try to "unresolve" the path + // associated with each of the properties to try to find the original object + // within the unresolved spec. + // If we're able to "unresolve" both properties, then we'll take the position + // that they SHOULD have a $ref and the $refs should be the same in order to + // declare the two properties compatible. + // If we're unable to unresolve one property or the other (which is not unexpected + // due to the rudimentary unresolver), then the next best thing would be to compare + // the JSON-stringified versions of p1 and p2 and if they are in fact exactly the same, + // we can make a (hopefully educated) guess that they originated from the same $ref. + if (type1 === SchemaType.OBJECT && type2 === SchemaType.OBJECT) { + // Unresolve each path to find the original object in the unresolved spec. + const p1Orig = unresolvePath(path1, unresolvedSpec); + const p2Orig = unresolvePath(path2, unresolvedSpec); + + if (p1Orig && p2Orig) { + // If both properties have a $ref field, then we'll compare them. + if (p1Orig.$ref && p2Orig.$ref) { + return p1Orig.$ref === p2Orig.$ref; + } else { + // We unresolved both paths successfully, but one or both of the original + // objects lacks a $ref field, so the properties are NOT compatible. + return false; + } + } + + // Just compare their JSON strings. + const p1String = JSON.stringify(p1); + const p2String = JSON.stringify(p2); + return p1String === p2String; + } + + // For non-object types, just compare the actual type values. + return type1 === type2; +} + +/** + * This function will try to "unresolve" the specified path (i.e. find the object + * corresponding to "resolvedPath" within the (original) unresolved API definition). + * + * The overall approach taken by this function is the following: + * Given the unresolved spec and a jsonpath from the resolved spec, the function + * will traverse the unresolved spec, one path segment at a time. At each hop, + * if the current object contains a $ref, then the "cursor" is changed from its current + * position (where the $ref was found) to the object referenced by the $ref. Then the traveral + * continues with the next path segment. Once we've exhausted the path segments, the final + * cursor postion is the desired object. + * This algorithm isn't foolproof: + * 1. The algorithm doesn't support external references. + * 2. "resolvedPath" may point to an object that has had allOf processing performed on it, therefore + * it might not be possible to find the corresponding object within the unresolved spec. + * 3. At any point if we're unable to traverse to the next path segment, we simply give up. + * + * @param {*} resolvedPath an array of path segments that specifies the location of + * an object within the resolved API definition. + * For example, for a scenario like the following: + * components: + * schemas: + * Foo: + * properties: + * foo: + * $ref: '#/components/schemas/Bar' + * Bar: + * properties: + * bar: + * type: string + * the path ['components', 'schemas', 'Foo', 'properties', 'foo', 'properties', 'bar'] would + * be mapped to the "bar" property within the Bar schema. + * @param {*} spec the unresolved API definition + * @returns the object within the unresolved API definition that corresponds to the + * object located at "resolvedPath" within the resolved API definition, or undefined if + * the mapping failes + */ +function unresolvePath(resolvedPath, spec) { + // Start our journey at the root of the unresolvedSpec. + let cursor = spec; + + for (const pathSeg of resolvedPath) { + // Make sure we're currently at a valid node in the document. + if (!cursor) { + return undefined; + } + + // If "cursor" is a $ref, then move to the referenced object before + // traversing to the "pathSeg" segment. + if (cursor.$ref) { + // Convert the reference to a jsonpath + // (e.g. '#/components/schemas/Bar' --> ['components', 'schemas', 'Bar']) + const newPath = refToPath(cursor.$ref); + if (!newPath || !newPath.length) { + return undefined; + } + + // Reset our cursor to the referenced object. + cursor = objectAtLocation(spec, newPath); + if (!cursor) { + return undefined; + } + } + + // Finally, take the next step in the path by traversing to the "pathSeg" segment. + cursor = objectAtLocation(cursor, [pathSeg]); + } + + return cursor; +} + +/** + * Starting at "rootObject", traverse to the object indicated by "path". + * @param {*} rootObject an object to be traversed + * @param {*} path an array of path segments indicating the location of the object + * (e.g. ['components', 'schemas', 'Foo', 'properties', 'foo']) + * @returns the object located at "path" or undefined if not found + */ +function objectAtLocation(rootObject, path) { + let obj = rootObject; + for (const segment of path) { + if (Array.isArray(obj)) { + const index = typeof segment === 'number' ? segment : parseInt(segment); + obj = obj[index]; + } else { + obj = obj[segment]; + } + + if (!obj) { + return undefined; + } + } + + return obj; +} + +/** + * Returns the path for the specified internal reference. + * @param {*} ref the reference (e.g. '#/components/schemas/Foo') + * @returns an array of path segments (e.g. ['components', 'schemas', 'Foo']) + * or undefined if "ref" is not an internal reference. + */ +function refToPath(ref) { + const parts = ref.split('#'); + + // Make sure that "ref" is an internal $ref. + const filename = parts[0] ? parts[0].trim() : undefined; + if (filename) { + return undefined; + } + + return parts[1].split('/').slice(1); +} diff --git a/packages/ruleset/src/functions/index.js b/packages/ruleset/src/functions/index.js index cde9bcbc9..78f393be6 100644 --- a/packages/ruleset/src/functions/index.js +++ b/packages/ruleset/src/functions/index.js @@ -7,6 +7,7 @@ module.exports = { checkMajorVersion: require('./check-major-version'), circularRefs: require('./circular-refs'), collectionArrayProperty: require('./collection-array-property'), + composedSchemaRestrictions: require('./composed-schema-restrictions'), consecutivePathParamSegments: require('./consecutive-path-param-segments'), deleteBody: require('./delete-body'), descriptionMentionsJSON: require('./description-mentions-json'), diff --git a/packages/ruleset/src/functions/inline-schema-rules.js b/packages/ruleset/src/functions/inline-schema-rules.js index fffa47d22..6599c9510 100644 --- a/packages/ruleset/src/functions/inline-schema-rules.js +++ b/packages/ruleset/src/functions/inline-schema-rules.js @@ -3,7 +3,7 @@ const { isJsonMimeType, isArraySchema, isEmptyObjectSchema, - isPrimitiveType, + isPrimitiveSchema, isRefSiblingSchema, validateSubschemas } = require('../utils'); @@ -35,7 +35,7 @@ function inlineResponseSchema(schema, options, { path }) { if ( !schema.$ref && isJsonMimeType(mimeType) && - !isPrimitiveType(schema) && + !isPrimitiveSchema(schema) && !arrayItemsAreRefOrPrimitive(schema) && !isRefSiblingSchema(schema) ) { @@ -72,7 +72,7 @@ function inlineRequestSchema(schema, options, { path }) { if ( !schema.$ref && isJsonMimeType(mimeType) && - !isPrimitiveType(schema) && + !isPrimitiveSchema(schema) && !arrayItemsAreRefOrPrimitive(schema) && !isRefSiblingSchema(schema) ) { @@ -111,9 +111,11 @@ function inlineRequestSchema(schema, options, { path }) { * @returns an array containing the violations found or [] if no violations */ function inlinePropertySchema(schema, options, { path }) { - // Check each sub-schema that is reachable from "schema" (properties, - // additionalProperties, allOf/anyOf/oneOf, array items, etc.) . - return validateSubschemas(schema, path, checkForInlineNestedObjectSchema); + // If "schema" is not a primitive, then check each sub-schema that is reachable from + // "schema" (properties, additionalProperties, allOf/anyOf/oneOf, array items, etc.). + return isPrimitiveSchema(schema) + ? [] + : validateSubschemas(schema, path, checkForInlineNestedObjectSchema); } /** @@ -129,7 +131,7 @@ function checkForInlineNestedObjectSchema(schema, path) { // then bail out now to avoid a warning. if ( schema.$ref || - isPrimitiveType(schema) || + isPrimitiveSchema(schema) || isRefSiblingSchema(schema) || isEmptyObjectSchema(schema) || isArraySchema(schema) @@ -182,5 +184,5 @@ function checkForInlineNestedObjectSchema(schema, path) { function arrayItemsAreRefOrPrimitive(schema) { const isArray = isArraySchema(schema); const items = isArray && getCompositeSchemaAttribute(schema, 'items'); - return items && (items.$ref || isPrimitiveType(items)); + return items && (items.$ref || isPrimitiveSchema(items)); } diff --git a/packages/ruleset/src/ibm-oas.js b/packages/ruleset/src/ibm-oas.js index 95a2f8bc4..ae32f5a18 100644 --- a/packages/ruleset/src/ibm-oas.js +++ b/packages/ruleset/src/ibm-oas.js @@ -99,6 +99,7 @@ module.exports = { 'binary-schemas': ibmRules.binarySchemas, 'circular-refs': ibmRules.circularRefs, 'collection-array-property': ibmRules.collectionArrayProperty, + 'composed-schema-restrictions': ibmRules.composedSchemaRestrictions, 'consecutive-path-param-segments': ibmRules.consecutivePathParamSegments, 'content-entry-contains-schema': ibmRules.contentEntryContainsSchema, 'content-entry-provided': ibmRules.contentEntryProvided, diff --git a/packages/ruleset/src/rules/composed-schema-restrictions.js b/packages/ruleset/src/rules/composed-schema-restrictions.js new file mode 100644 index 000000000..452ab739b --- /dev/null +++ b/packages/ruleset/src/rules/composed-schema-restrictions.js @@ -0,0 +1,16 @@ +const { oas3 } = require('@stoplight/spectral-formats'); +const { composedSchemaRestrictions } = require('../functions'); +const { schemas } = require('../collections'); + +module.exports = { + description: + 'Composed schemas using oneOf or anyOf should comply with restrictions imposed by the SDK generator', + message: '{{error}}', + given: schemas, + severity: 'error', + formats: [oas3], + resolved: true, + then: { + function: composedSchemaRestrictions + } +}; diff --git a/packages/ruleset/src/rules/index.js b/packages/ruleset/src/rules/index.js index 8faf37957..d15bdf581 100644 --- a/packages/ruleset/src/rules/index.js +++ b/packages/ruleset/src/rules/index.js @@ -8,6 +8,7 @@ module.exports = { binarySchemas: require('./binary-schemas'), circularRefs: require('./circular-refs'), collectionArrayProperty: require('./collection-array-property'), + composedSchemaRestrictions: require('./composed-schema-restrictions'), consecutivePathParamSegments: require('./consecutive-path-param-segments'), contentEntryContainsSchema: require('./content-entry-contains-schema'), contentEntryProvided: require('./content-entry-provided'), diff --git a/packages/ruleset/src/utils/get-schema-type.js b/packages/ruleset/src/utils/get-schema-type.js index 85dbee9d5..fb49c554a 100644 --- a/packages/ruleset/src/utils/get-schema-type.js +++ b/packages/ruleset/src/utils/get-schema-type.js @@ -294,6 +294,29 @@ const isStringSchema = schema => { return checkCompositeSchemaForConstraint(schema, s => s.type === 'string'); }; +/** + * Returns true if "schema" is a primitive schema (string, integer, number, boolean) + * @param {*} schema the schema to check + * @returns boolean + */ +function isPrimitiveSchema(s) { + const primitiveTypes = [ + SchemaType.BOOLEAN, + SchemaType.BYTE, + SchemaType.DOUBLE, + SchemaType.ENUMERATION, + SchemaType.FLOAT, + SchemaType.INT32, + SchemaType.INT64, + SchemaType.INTEGER, + SchemaType.NUMBER, + SchemaType.STRING + ]; + + const type = getSchemaType(s); + return primitiveTypes.includes(type); +} + module.exports = { SchemaType, getSchemaType, @@ -311,5 +334,6 @@ module.exports = { isIntegerSchema, isNumberSchema, isObjectSchema, + isPrimitiveSchema, isStringSchema }; diff --git a/packages/ruleset/src/utils/index.js b/packages/ruleset/src/utils/index.js index 5318c3036..6cad71835 100644 --- a/packages/ruleset/src/utils/index.js +++ b/packages/ruleset/src/utils/index.js @@ -6,7 +6,6 @@ module.exports = { isDeprecated: require('./is-deprecated'), isEmptyObjectSchema: require('./is-empty-object-schema'), isObject: require('./is-object'), - isPrimitiveType: require('./is-primitive-type'), isRefSiblingSchema: require('./is-ref-sibling-schema'), mergeAllOfSchemaProperties: require('./merge-allof-schema-properties'), ...require('./mimetype-utils'), diff --git a/packages/ruleset/src/utils/is-primitive-type.js b/packages/ruleset/src/utils/is-primitive-type.js deleted file mode 100644 index d708e932b..000000000 --- a/packages/ruleset/src/utils/is-primitive-type.js +++ /dev/null @@ -1,13 +0,0 @@ -/** - * Returns true if "schema" is a primitive schema. - * @param {*} schema the schema to check - * @returns boolean - */ -function isPrimitiveType(schema) { - return ( - schema.type && - ['boolean', 'integer', 'number', 'string'].includes(schema.type) - ); -} - -module.exports = isPrimitiveType; diff --git a/packages/ruleset/test/composed-schema-restrictions.test.js b/packages/ruleset/test/composed-schema-restrictions.test.js new file mode 100644 index 000000000..8af25c08e --- /dev/null +++ b/packages/ruleset/test/composed-schema-restrictions.test.js @@ -0,0 +1,627 @@ +const { composedSchemaRestrictions } = require('../src/rules'); +const { makeCopy, rootDocument, testRule, severityCodes } = require('./utils'); + +const rule = composedSchemaRestrictions; +const ruleId = 'composed-schema-restrictions'; +const expectedSeverity = severityCodes.error; +const expectedMsgObjSchema = + 'A schema within a oneOf/anyOf list must be an object schema'; +const expectedMsgProp = + 'Duplicate property with incompatible type defined in schema within a oneOf/anyOf list:'; + +describe('Spectral rule: schema-description', () => { + describe('Should not yield errors', () => { + it('Clean spec', async () => { + const results = await testRule(ruleId, rule, rootDocument); + expect(results).toHaveLength(0); + }); + + it('Primitive property w/oneOf', async () => { + const testDocument = makeCopy(rootDocument); + + testDocument.components.schemas['Drink'].properties = { + type: { + type: 'string', + oneOf: [ + { + enum: ['soda'] + }, + { + enum: ['juice'] + } + ] + } + }; + + const results = await testRule(ruleId, rule, testDocument); + expect(results).toHaveLength(0); + }); + + it('Primitive `items` schema w/anyOf', async () => { + const testDocument = makeCopy(rootDocument); + + testDocument.components.schemas['Drink'].properties = { + drink_types: { + type: 'array', + items: { + type: 'string', + anyOf: [ + { + minLength: 1, + maxLength: 10 + }, + { + enum: ['juice'] + } + ] + } + } + }; + + const results = await testRule(ruleId, rule, testDocument); + expect(results).toHaveLength(0); + }); + + it('Primitive `additionalProperties` schema w/oneOf', async () => { + const testDocument = makeCopy(rootDocument); + + testDocument.components.schemas['Drink'].additionalProperties = { + type: 'string', + oneOf: [ + { + minLength: 1, + maxLength: 10 + }, + { + enum: ['foo', 'bar'] + } + ] + }; + + const results = await testRule(ruleId, rule, testDocument); + expect(results).toHaveLength(0); + }); + + it('Compatible properties defined in allOf', async () => { + const testDocument = makeCopy(rootDocument); + + testDocument.components.schemas['Drink'].allOf = [ + { + properties: { + float_prop: { + type: 'number', + format: 'float' + } + } + }, + { + properties: { + double_prop: { + type: 'number', + format: 'double' + } + } + } + ]; + + testDocument.components.schemas['Soda'].allOf = [ + { + properties: { + float_prop: { + type: 'number', + format: 'float' + } + } + } + ]; + + testDocument.components.schemas['Juice'].allOf = [ + { + properties: { + double_prop: { + type: 'number', + format: 'double' + } + } + } + ]; + + const results = await testRule(ruleId, rule, testDocument); + expect(results).toHaveLength(0); + }); + + it('Compatible properties that are objects (refs)', async () => { + const testDocument = makeCopy(rootDocument); + + const objectSchema = { + type: 'object', + properties: { + foo: { + type: 'string' + }, + bar: { + type: 'integer' + } + } + }; + + const schemaRef = { + $ref: '#/components/schemas/ObjectSchema' + }; + + testDocument.components.schemas['ObjectSchema'] = objectSchema; + + testDocument.components.schemas['Drink'].properties = { + object_prop: schemaRef + }; + + testDocument.components.schemas[ + 'Soda' + ].properties.object_prop = schemaRef; + + testDocument.components.schemas[ + 'Juice' + ].properties.object_prop = schemaRef; + + const results = await testRule(ruleId, rule, testDocument); + expect(results).toHaveLength(0); + }); + + it('Compatible properties that are objects (refs, allOf)', async () => { + const testDocument = makeCopy(rootDocument); + + const objectSchema = { + allOf: [ + { + properties: { + foo: { + type: 'string' + } + } + }, + { + properties: { + bar: { + type: 'integer' + } + } + } + ] + }; + + testDocument.components.schemas['ObjectSchema'] = objectSchema; + + const schemaRef = { + $ref: '#/components/schemas/ObjectSchema' + }; + + testDocument.components.schemas['Drink'].properties = { + object_prop: schemaRef + }; + + testDocument.components.schemas[ + 'Soda' + ].properties.object_prop = schemaRef; + + testDocument.components.schemas[ + 'Juice' + ].properties.object_prop = schemaRef; + + const results = await testRule(ruleId, rule, testDocument); + expect(results).toHaveLength(0); + }); + + it('Empty oneOf', async () => { + const testDocument = makeCopy(rootDocument); + + testDocument.components.schemas['Drink'].additionalProperties = { + type: 'object', + properties: { + oneOf: { + type: 'string' + } + }, + oneOf: [] + }; + + const results = await testRule(ruleId, rule, testDocument); + expect(results).toHaveLength(0); + }); + + it('Empty anyOf', async () => { + const testDocument = makeCopy(rootDocument); + + testDocument.components.schemas['Drink'].additionalProperties = { + type: 'object', + properties: { + anyOf: { + type: 'string' + } + }, + anyOf: [] + }; + + const results = await testRule(ruleId, rule, testDocument); + expect(results).toHaveLength(0); + }); + }); + + describe('Should yield errors', () => { + it('AnyOf with non-object schemas', async () => { + const testDocument = makeCopy(rootDocument); + + const newDrinkSchema = makeCopy(testDocument.components.schemas['Drink']); + + const notAnObject = { + description: 'non-object schema within anyOf', + anyOf: [ + { + type: 'string', + format: 'date-time' + }, + { + properties: { + foo: { + type: 'string' + } + } + } + ] + }; + + testDocument.components.schemas['NewDrink'] = newDrinkSchema; + testDocument.components.schemas['NotAnObject'] = notAnObject; + + newDrinkSchema.additionalProperties = { + $ref: '#/components/schemas/NotAnObject' + }; + + testDocument.components.schemas[ + 'DrinkCollection' + ].allOf[1].properties.drinks.items.$ref = '#/components/schemas/NewDrink'; + + const results = await testRule(ruleId, rule, testDocument); + expect(results).toHaveLength(1); + expect(results[0].code).toBe(ruleId); + expect(results[0].message).toBe(expectedMsgObjSchema); + expect(results[0].severity).toBe(expectedSeverity); + expect(results[0].path.join('.')).toBe( + 'paths./v1/drinks.get.responses.200.content.application/json.schema.allOf.1.properties.drinks.items.additionalProperties.anyOf.0' + ); + }); + + it('Incompatible properties defined in main & subschema', async () => { + const testDocument = makeCopy(rootDocument); + + const newDrinkSchema = makeCopy(testDocument.components.schemas['Drink']); + const newJuiceSchema = makeCopy(testDocument.components.schemas['Juice']); + + testDocument.components.schemas['NewDrink'] = newDrinkSchema; + testDocument.components.schemas['NewJuice'] = newJuiceSchema; + + testDocument.components.schemas[ + 'DrinkCollection' + ].allOf[1].properties.drinks.items.$ref = '#/components/schemas/NewDrink'; + newDrinkSchema.oneOf[0].$ref = '#/components/schemas/NewJuice'; + + newDrinkSchema.properties = { + drink_size: { + type: 'integer', + format: 'int64' + } + }; + newJuiceSchema.properties.drink_size = { + type: 'integer', + format: 'int32' + }; + + const results = await testRule(ruleId, rule, testDocument); + expect(results).toHaveLength(1); + expect(results[0].code).toBe(ruleId); + expect(results[0].message).toBe(`${expectedMsgProp} drink_size`); + expect(results[0].severity).toBe(expectedSeverity); + expect(results[0].path.join('.')).toBe( + 'paths./v1/drinks.get.responses.200.content.application/json.schema.allOf.1.properties.drinks.items.oneOf.0' + ); + }); + + it('Incompatible properties defined in two subschemas', async () => { + const testDocument = makeCopy(rootDocument); + + const newDrinkSchema = makeCopy(testDocument.components.schemas['Drink']); + const newJuiceSchema = makeCopy(testDocument.components.schemas['Juice']); + const newSodaSchema = makeCopy(testDocument.components.schemas['Soda']); + + testDocument.components.schemas['NewDrink'] = newDrinkSchema; + testDocument.components.schemas['NewJuice'] = newJuiceSchema; + testDocument.components.schemas['NewSoda'] = newSodaSchema; + + testDocument.components.schemas[ + 'DrinkCollection' + ].allOf[1].properties.drinks.items.$ref = '#/components/schemas/NewDrink'; + newDrinkSchema.oneOf[0].$ref = '#/components/schemas/NewJuice'; + newDrinkSchema.oneOf[1].$ref = '#/components/schemas/NewSoda'; + + newSodaSchema.properties.bad_prop = { + type: 'string' + }; + newJuiceSchema.properties.bad_prop = { + type: 'integer' + }; + + const results = await testRule(ruleId, rule, testDocument); + expect(results).toHaveLength(1); + expect(results[0].code).toBe(ruleId); + expect(results[0].message).toBe(`${expectedMsgProp} bad_prop`); + expect(results[0].severity).toBe(expectedSeverity); + expect(results[0].path.join('.')).toBe( + 'paths./v1/drinks.get.responses.200.content.application/json.schema.allOf.1.properties.drinks.items.oneOf.1' + ); + }); + + it('Incompatible properties defined in subschema allOfs', async () => { + const testDocument = makeCopy(rootDocument); + + const newDrinkSchema = makeCopy(testDocument.components.schemas['Drink']); + const newSodaSchema = makeCopy(testDocument.components.schemas['Soda']); + const newJuiceSchema = makeCopy(testDocument.components.schemas['Juice']); + + testDocument.components.schemas['NewDrink'] = newDrinkSchema; + testDocument.components.schemas['NewJuice'] = newJuiceSchema; + testDocument.components.schemas['NewSoda'] = newSodaSchema; + + testDocument.components.schemas[ + 'DrinkCollection' + ].allOf[1].properties.drinks.items.$ref = '#/components/schemas/NewDrink'; + newDrinkSchema.oneOf[0].$ref = '#/components/schemas/NewJuice'; + newDrinkSchema.oneOf[1].$ref = '#/components/schemas/NewSoda'; + + newSodaSchema.allOf = [ + { + properties: { + bad_prop: { + type: 'string' + } + } + } + ]; + newJuiceSchema.allOf = [ + { + properties: { + bad_prop: { + type: 'integer' + } + } + } + ]; + + const results = await testRule(ruleId, rule, testDocument); + expect(results).toHaveLength(1); + expect(results[0].code).toBe(ruleId); + expect(results[0].message).toBe(`${expectedMsgProp} bad_prop`); + expect(results[0].severity).toBe(expectedSeverity); + expect(results[0].path.join('.')).toBe( + 'paths./v1/drinks.get.responses.200.content.application/json.schema.allOf.1.properties.drinks.items.oneOf.1' + ); + }); + + it('Incompatible properties defined in complex compositions', async () => { + const testDocument = makeCopy(rootDocument); + + const newDrinkSchema = makeCopy(testDocument.components.schemas['Drink']); + const newSodaSchema = makeCopy(testDocument.components.schemas['Soda']); + const newJuiceSchema = makeCopy(testDocument.components.schemas['Juice']); + + testDocument.components.schemas['NewDrink'] = newDrinkSchema; + testDocument.components.schemas['NewJuice'] = newJuiceSchema; + testDocument.components.schemas['NewSoda'] = newSodaSchema; + + testDocument.components.schemas[ + 'DrinkCollection' + ].allOf[1].properties.drinks.items.$ref = '#/components/schemas/NewDrink'; + newDrinkSchema.oneOf[0].$ref = '#/components/schemas/NewJuice'; + newDrinkSchema.oneOf[1].$ref = '#/components/schemas/NewSoda'; + + newSodaSchema.allOf = [ + { + allOf: [ + { + allOf: [ + { + properties: { + bad_prop: { + type: 'string' + } + } + } + ] + } + ] + } + ]; + newJuiceSchema.allOf = [ + { + allOf: [ + { + allOf: [ + { + allOf: [ + { + properties: { + bad_prop: { + type: 'integer' + } + } + } + ] + } + ] + } + ] + } + ]; + + const results = await testRule(ruleId, rule, testDocument); + expect(results).toHaveLength(1); + expect(results[0].code).toBe(ruleId); + expect(results[0].message).toBe(`${expectedMsgProp} bad_prop`); + expect(results[0].severity).toBe(expectedSeverity); + expect(results[0].path.join('.')).toBe( + 'paths./v1/drinks.get.responses.200.content.application/json.schema.allOf.1.properties.drinks.items.oneOf.1' + ); + }); + + it('Incompatible properties that are objects (non-refs)', async () => { + const testDocument = makeCopy(rootDocument); + + const newDrinkSchema = makeCopy(testDocument.components.schemas['Drink']); + const newJuiceSchema = makeCopy(testDocument.components.schemas['Juice']); + + testDocument.components.schemas['NewDrink'] = newDrinkSchema; + testDocument.components.schemas['NewJuice'] = newJuiceSchema; + + testDocument.components.schemas[ + 'DrinkCollection' + ].allOf[1].properties.drinks.items.$ref = '#/components/schemas/NewDrink'; + newDrinkSchema.oneOf[0].$ref = '#/components/schemas/NewJuice'; + + const objectSchema = { + type: 'object', + properties: { + foo: { + type: 'string' + }, + bar: { + type: 'integer' + } + } + }; + + testDocument.components.schemas['NewDrink'].properties = { + object_prop: objectSchema + }; + + newJuiceSchema.properties.object_prop = { + allOf: [ + { + objectSchema + }, + { + description: 'This makes the schema different.' + } + ] + }; + + const results = await testRule(ruleId, rule, testDocument); + expect(results).toHaveLength(1); + expect(results[0].code).toBe(ruleId); + expect(results[0].message).toBe(`${expectedMsgProp} object_prop`); + expect(results[0].severity).toBe(expectedSeverity); + expect(results[0].path.join('.')).toBe( + 'paths./v1/drinks.get.responses.200.content.application/json.schema.allOf.1.properties.drinks.items.oneOf.0' + ); + }); + + it('Incompatible properties that are objects (refs)', async () => { + const testDocument = makeCopy(rootDocument); + + const newDrinkSchema = makeCopy(testDocument.components.schemas['Drink']); + const newJuiceSchema = makeCopy(testDocument.components.schemas['Juice']); + + testDocument.components.schemas['NewDrink'] = newDrinkSchema; + testDocument.components.schemas['NewJuice'] = newJuiceSchema; + + testDocument.components.schemas[ + 'DrinkCollection' + ].allOf[1].properties.drinks.items.$ref = '#/components/schemas/NewDrink'; + newDrinkSchema.oneOf[0].$ref = '#/components/schemas/NewJuice'; + + const objectSchema = { + type: 'object', + properties: { + foo: { + type: 'string' + }, + bar: { + type: 'integer' + } + } + }; + + // Two identical schemas with different names -> incompatible due + // to different datatypes in generated code. + testDocument.components.schemas['Object1'] = objectSchema; + testDocument.components.schemas['Object2'] = objectSchema; + + testDocument.components.schemas['NewDrink'].properties = { + object_prop: { + $ref: '#/components/schemas/Object1' + } + }; + + newJuiceSchema.properties.object_prop = { + $ref: '#/components/schemas/Object2' + }; + + const results = await testRule(ruleId, rule, testDocument); + expect(results).toHaveLength(1); + expect(results[0].code).toBe(ruleId); + expect(results[0].message).toBe(`${expectedMsgProp} object_prop`); + expect(results[0].severity).toBe(expectedSeverity); + expect(results[0].path.join('.')).toBe( + 'paths./v1/drinks.get.responses.200.content.application/json.schema.allOf.1.properties.drinks.items.oneOf.0' + ); + }); + + it('Incompatible properties that are objects but not refs', async () => { + const testDocument = makeCopy(rootDocument); + + const newDrinkSchema = makeCopy(testDocument.components.schemas['Drink']); + const newJuiceSchema = makeCopy(testDocument.components.schemas['Juice']); + + testDocument.components.schemas['NewDrink'] = newDrinkSchema; + testDocument.components.schemas['NewJuice'] = newJuiceSchema; + + testDocument.components.schemas[ + 'DrinkCollection' + ].allOf[1].properties.drinks.items.$ref = '#/components/schemas/NewDrink'; + newDrinkSchema.oneOf[0].$ref = '#/components/schemas/NewJuice'; + + const objectSchema = { + type: 'object', + properties: { + foo: { + type: 'string' + }, + bar: { + type: 'integer' + } + } + }; + + testDocument.components.schemas['NewDrink'].properties = { + object_prop: objectSchema + }; + + newJuiceSchema.properties.object_prop = { + allOf: [ + { + objectSchema + }, + { + description: 'This makes the schema different.' + } + ] + }; + + const results = await testRule(ruleId, rule, testDocument); + expect(results).toHaveLength(1); + expect(results[0].code).toBe(ruleId); + expect(results[0].message).toBe(`${expectedMsgProp} object_prop`); + expect(results[0].severity).toBe(expectedSeverity); + expect(results[0].path.join('.')).toBe( + 'paths./v1/drinks.get.responses.200.content.application/json.schema.allOf.1.properties.drinks.items.oneOf.0' + ); + }); + }); +}); diff --git a/packages/ruleset/test/is-object-schema.test.js b/packages/ruleset/test/is-object-schema.test.js index 833a6ae54..b82ddc625 100644 --- a/packages/ruleset/test/is-object-schema.test.js +++ b/packages/ruleset/test/is-object-schema.test.js @@ -58,4 +58,34 @@ describe('Utility function: isObjectSchema()', () => { }) ).toBe(true); }); + + it('should recurse through `oneOf` and `allOf` (implicit object type)', async () => { + expect( + isObjectSchema({ + oneOf: [ + { + allOf: [{ properties: {} }, {}] + }, + { + properties: {} + } + ] + }) + ).toBe(true); + }); + + it('should recurse through `allOf` (implicit object type)', async () => { + expect( + isObjectSchema({ + allOf: [ + { + allOf: [{ properties: {} }, {}] + }, + { + properties: {} + } + ] + }) + ).toBe(true); + }); }); diff --git a/packages/ruleset/test/is-primitive-schema.test.js b/packages/ruleset/test/is-primitive-schema.test.js new file mode 100644 index 000000000..6f552d3de --- /dev/null +++ b/packages/ruleset/test/is-primitive-schema.test.js @@ -0,0 +1,142 @@ +const { isPrimitiveSchema } = require('../src/utils'); + +describe('Utility function: isPrimitiveSchema()', () => { + it('should return `false` for `undefined`', async () => { + expect(isPrimitiveSchema(undefined)).toBe(false); + }); + + it('should return `false` for `null`', async () => { + expect(isPrimitiveSchema(null)).toBe(false); + }); + + it('should return `false` for an array', async () => { + expect(isPrimitiveSchema([])).toBe(false); + }); + + it('should return `false` for an empty object', async () => { + expect(isPrimitiveSchema({})).toBe(false); + }); + + it('should return `true` for a boolean schema', async () => { + expect(isPrimitiveSchema({ type: 'boolean' })).toBe(true); + }); + + it('should return `true` for a byte schema', async () => { + expect(isPrimitiveSchema({ type: 'string', format: 'byte' })).toBe(true); + }); + + it('should return `true` for a double schema', async () => { + expect(isPrimitiveSchema({ type: 'number', format: 'double' })).toBe(true); + }); + + it('should return `true` for an enumeration', async () => { + expect(isPrimitiveSchema({ type: 'string', enum: ['foo', 'bar'] })).toBe( + true + ); + }); + + it('should return `true` for a float schema', async () => { + expect(isPrimitiveSchema({ type: 'number', format: 'float' })).toBe(true); + }); + + it('should return `true` for a int32 schema', async () => { + expect(isPrimitiveSchema({ type: 'integer', format: 'int32' })).toBe(true); + }); + + it('should return `true` for a int64 schema', async () => { + expect(isPrimitiveSchema({ type: 'integer', format: 'int64' })).toBe(true); + }); + + it('should return `true` for a integer schema', async () => { + expect(isPrimitiveSchema({ type: 'integer' })).toBe(true); + }); + + it('should return `true` for a number schema', async () => { + expect(isPrimitiveSchema({ type: 'number' })).toBe(true); + }); + + it('should return `true` for a string schema', async () => { + expect(isPrimitiveSchema({ type: 'string' })).toBe(true); + }); + + it('should return true for a composed schema that resolves to "int32"', async () => { + expect( + isPrimitiveSchema({ + oneOf: [ + { + allOf: [ + { + anyOf: [ + { type: 'integer', format: 'int32' }, + { type: 'integer', format: 'int32' } + ] + }, + {} + ] + }, + { type: 'integer', format: 'int32' } + ] + }) + ).toBe(true); + }); + + it('should return true for a composed schema that resolves to "double"', async () => { + expect( + isPrimitiveSchema({ + oneOf: [ + { + allOf: [ + { + anyOf: [ + { type: 'number', format: 'double' }, + { type: 'number', format: 'double' } + ] + }, + {} + ] + }, + { type: 'number', format: 'double' } + ] + }) + ).toBe(true); + }); + + it('should return true for a composed schema that resolves to "number"', async () => { + expect( + isPrimitiveSchema({ + oneOf: [ + { + allOf: [{ anyOf: [{ type: 'number' }, { type: 'number' }] }, {}] + }, + { type: 'number' } + ] + }) + ).toBe(true); + }); + + it('should return true for a composed schema that resolves to "boolean"', async () => { + expect( + isPrimitiveSchema({ + oneOf: [ + { + allOf: [{ anyOf: [{ type: 'boolean' }, { type: 'boolean' }] }, {}] + }, + { type: 'boolean' } + ] + }) + ).toBe(true); + }); + + it('should return true for a composed schema that resolves to "string"', async () => { + expect( + isPrimitiveSchema({ + oneOf: [ + { + allOf: [{ anyOf: [{ type: 'string' }, { type: 'string' }] }, {}] + }, + { type: 'string' } + ] + }) + ).toBe(true); + }); +}); diff --git a/packages/ruleset/test/merge-allof-schema-properties.test.js b/packages/ruleset/test/merge-allof-schema-properties.test.js index 1b4f219f0..71179d485 100644 --- a/packages/ruleset/test/merge-allof-schema-properties.test.js +++ b/packages/ruleset/test/merge-allof-schema-properties.test.js @@ -15,7 +15,7 @@ describe('Utility function: mergeAllOfSchemaProperties()', () => { expect(mergeAllOfSchemaProperties(schema)).toStrictEqual(schema); }); - it('should return original schema (minum allOf) if empty allOf', async () => { + it('should return original schema (minus allOf) if empty allOf', async () => { const schema = { description: 'the description', type: 'object', From 8dbe0ff4facfba53e39de7458581743cdc8fa677 Mon Sep 17 00:00:00 2001 From: Phil Adams Date: Wed, 5 Oct 2022 10:54:48 -0500 Subject: [PATCH 2/4] fix: address review comments Signed-off-by: Phil Adams --- docs/ibm-cloud-rules.md | 35 ++++++++++--------- .../functions/composed-schema-restrictions.js | 6 ++-- .../src/rules/composed-schema-restrictions.js | 4 +-- .../test/composed-schema-restrictions.test.js | 8 ++--- 4 files changed, 26 insertions(+), 27 deletions(-) diff --git a/docs/ibm-cloud-rules.md b/docs/ibm-cloud-rules.md index 07833fb1d..0053c068b 100644 --- a/docs/ibm-cloud-rules.md +++ b/docs/ibm-cloud-rules.md @@ -180,9 +180,9 @@ within the operation's path string, which should also match the plural form of t - - + + @@ -1337,20 +1337,21 @@ paths: - - + @@ -1379,7 +1380,7 @@ components: common_property: type: integer <<< incompatible type some_other_property: - type: string + type: integer @@ -1406,7 +1407,7 @@ components: common_property: type: string some_other_property: - type: string + type: integer @@ -2337,12 +2338,12 @@ it is a best practice to define the schema as a named schema within the co of the API definition, and then reference it with a schema $ref instead of defining it as an inline object schema. This is documented in the [API Handbook](https://cloud.ibm.com/docs/api-handbook?topic=api-handbook-schemas#nested-object-schemas). -

The use of a schema $ref is preferred instead of a nested object schema, because the SDK generator will +

The use of a schema $ref is preferred instead of a nested object schema, because IBM's SDK generator will use the schema $ref when determining the datatype associated with the nested object within the generated SDK code. -If the SDK generator encounters a nested objet schema, it must refactor it by moving it to the components.schemas +If IBM's SDK generator encounters a nested object schema, it must refactor it by moving it to the components.schemas section of the API definition and then replacing it with a schema $ref. -However, the names computed by the SDK generator are not optimal (e.g. MyModelProp1), -so the recommendation is to define any nested object schema as a $ref so that the SDK generator's +However, the names computed by the generator are not optimal (e.g. MyModelProp1), +so the recommendation is to define any nested object schema as a $ref so that the generator's refactoring (and it's sub-optimal name computation) can be avoided. @@ -2496,10 +2497,10 @@ paths:

diff --git a/packages/ruleset/src/functions/composed-schema-restrictions.js b/packages/ruleset/src/functions/composed-schema-restrictions.js index 5a573c292..05843cf18 100644 --- a/packages/ruleset/src/functions/composed-schema-restrictions.js +++ b/packages/ruleset/src/functions/composed-schema-restrictions.js @@ -58,12 +58,12 @@ function composedSchemaRestrictions(schema, path) { return []; } - // Next, we need to verify that it's oneOf/anyOf list complies. + // Next, we need to verify that its oneOf/anyOf list complies. return checkSchemaList(schema, subSchemas, fieldName, path); } /** - * This function checks "schema" and it's oneOf/anyOf list "subSchemas" to verify that + * This function checks "schema" and its oneOf/anyOf list "subSchemas" to verify that * it complies with the generator restrictions around the use of oneOf/anyOf. * @param {*} schema the main schema to check * @param {*} subSchemas the oneOf or anyOf list belonging to "schema" @@ -89,7 +89,7 @@ function checkSchemaList(schema, subSchemas, fieldName, path) { // "s" should be an object schema. If not, return an error. if (!isObjectSchema(s)) { errors.push({ - message: 'A schema within a oneOf/anyOf list must be an object schema', + message: `A schema within an object schema's oneOf/anyOf list must be an object schema`, path: [...path, fieldName, i.toString()] }); } else { diff --git a/packages/ruleset/src/rules/composed-schema-restrictions.js b/packages/ruleset/src/rules/composed-schema-restrictions.js index 452ab739b..29b95f009 100644 --- a/packages/ruleset/src/rules/composed-schema-restrictions.js +++ b/packages/ruleset/src/rules/composed-schema-restrictions.js @@ -4,10 +4,10 @@ const { schemas } = require('../collections'); module.exports = { description: - 'Composed schemas using oneOf or anyOf should comply with restrictions imposed by the SDK generator', + 'Composed schemas using oneOf or anyOf should comply with SDK generation best practices', message: '{{error}}', given: schemas, - severity: 'error', + severity: 'warn', formats: [oas3], resolved: true, then: { diff --git a/packages/ruleset/test/composed-schema-restrictions.test.js b/packages/ruleset/test/composed-schema-restrictions.test.js index 8af25c08e..cb1c63402 100644 --- a/packages/ruleset/test/composed-schema-restrictions.test.js +++ b/packages/ruleset/test/composed-schema-restrictions.test.js @@ -3,11 +3,9 @@ const { makeCopy, rootDocument, testRule, severityCodes } = require('./utils'); const rule = composedSchemaRestrictions; const ruleId = 'composed-schema-restrictions'; -const expectedSeverity = severityCodes.error; -const expectedMsgObjSchema = - 'A schema within a oneOf/anyOf list must be an object schema'; -const expectedMsgProp = - 'Duplicate property with incompatible type defined in schema within a oneOf/anyOf list:'; +const expectedSeverity = severityCodes.warning; +const expectedMsgObjSchema = `A schema within an object schema's oneOf/anyOf list must be an object schema`; +const expectedMsgProp = `Duplicate property with incompatible type defined in schema within a oneOf/anyOf list:`; describe('Spectral rule: schema-description', () => { describe('Should not yield errors', () => { From 3adcba4f2bfabba7ddb5932c6c448fbde8a0a22b Mon Sep 17 00:00:00 2001 From: Dan Hudlow Date: Tue, 11 Oct 2022 22:43:56 -0500 Subject: [PATCH 3/4] refactor: work-in-progress proposed refactor of composed schema restrictions Signed-off-by: Dan Hudlow --- .../functions/composed-schema-restrictions.js | 342 +++--------------- .../src/utils/get-property-schemas-by-name.js | 52 +++ packages/ruleset/src/utils/index.js | 1 + .../test/composed-schema-restrictions.test.js | 18 +- 4 files changed, 114 insertions(+), 299 deletions(-) create mode 100644 packages/ruleset/src/utils/get-property-schemas-by-name.js diff --git a/packages/ruleset/src/functions/composed-schema-restrictions.js b/packages/ruleset/src/functions/composed-schema-restrictions.js index 05843cf18..f318520a9 100644 --- a/packages/ruleset/src/functions/composed-schema-restrictions.js +++ b/packages/ruleset/src/functions/composed-schema-restrictions.js @@ -1,123 +1,44 @@ const { - isPrimitiveSchema, validateSubschemas, - getCompositeSchemaAttribute, + getPropertySchemasByName, getSchemaType, isObjectSchema, isArraySchema, + isEnumerationSchema, isObject, - mergeAllOfSchemaProperties, SchemaType } = require('../utils'); -let unresolvedSpec; -module.exports = function(schema, opts, context) { - unresolvedSpec = context.document.parserResult.data; - return validateSubschemas(schema, context.path, composedSchemaRestrictions); +module.exports = function(schema, options, { path }) { + return validateSubschemas(schema, path, composedSchemaRestrictions); }; /** - * This function checks "schema" to make sure that if it's a composed schema - * using oneOf or anyOf that it complies with the restrictions imposed by the - * SDK generator. - * The specific checks that are performed here are: - * 1. Any schema that is a primitive or an array, or contains no oneOf or anyOf - * is skipped (automatically passes). - * 2. A schema's oneOf or anyOf list must contain only object schemas. - * Any non-object schema is rejected. - * 3. The union of the properties defined by the main schema and its - * oneOf/anyOf sub-schemas is examined to detect any duplicates. - * Duplicate properties with different types are rejected. + * Checks to make sure properties across a composed object schema have types that are deemed + * compatible for modeling that schema. * * @param {*} schema the schema to check * @param {*} path the array of path segments indicating the location of "schema" within the API definition * @returns an array containing the violations found or [] if no violations */ function composedSchemaRestrictions(schema, path) { - // We're mainly interested in object schemas within this validation rule. - // If "schema" is a primitive, then the SDK generator will unconditionally - // replace it with an equivalent schema without the oneOf/anyOf list, - // so nothing more to check for that scenario. - // If "schema" is an array schema, then it can't have an allOf/oneOf/anyOf list, - // so this scenario is not interesting either (the array's "items" field will be - // validated separately). - if (isPrimitiveSchema(schema) || isArraySchema(schema)) { - return []; - } - - // Next, we'll look for oneOf and then anyOf. Only one should be present. - let subSchemas = getCompositeSchemaAttribute(schema, 'oneOf'); - let fieldName = 'oneOf'; - if (!isNonemptyArray(subSchemas)) { - subSchemas = getCompositeSchemaAttribute(schema, 'anyOf'); - fieldName = 'anyOf'; - } - - // If we found neither oneOf nor anyOf, then bail out now. - if (!isNonemptyArray(subSchemas)) { - return []; - } - - // Next, we need to verify that its oneOf/anyOf list complies. - return checkSchemaList(schema, subSchemas, fieldName, path); -} - -/** - * This function checks "schema" and its oneOf/anyOf list "subSchemas" to verify that - * it complies with the generator restrictions around the use of oneOf/anyOf. - * @param {*} schema the main schema to check - * @param {*} subSchemas the oneOf or anyOf list belonging to "schema" - * @param {*} fieldName the name of the field containing "subSchemas" within "schema" ('oneOf' or 'anyOf') - * @param {*} path the array of path segments indicating the location of "schema" within the API definition - * @returns an array containing the violations found or [] if no violations - */ -function checkSchemaList(schema, subSchemas, fieldName, path) { const errors = []; - schema = mergeAllOfSchemaProperties(schema); - - // "targetProperties" will initially contain the properties defined in the main schema, - // then we'll add each property from the subSchemas to it to simulate the SDK generator's - // handling of the oneOf/anyOf list (hybrid hierarchy mode). - const targetProperties = isObject(schema.properties) - ? copyObject(schema.properties) - : {}; - - for (let i = 0; i < subSchemas.length; i++) { - const s = mergeAllOfSchemaProperties(subSchemas[i]); - - // "s" should be an object schema. If not, return an error. - if (!isObjectSchema(s)) { - errors.push({ - message: `A schema within an object schema's oneOf/anyOf list must be an object schema`, - path: [...path, fieldName, i.toString()] - }); - } else { - // "s" is an object schema as expected, so let's check each of its properties. - const properties = isObject(s.properties) ? s.properties : {}; - for (const [name, prop] of Object.entries(properties)) { - const targetProp = targetProperties[name]; - if (targetProp) { - // If "targetProperties" contains a like-named property, make sure the types are compatible. - // We can only guess at the paths associated with target and sub-schema properties - // due to the allOf processing that needs to be performed on each schema. - const propsAreCompatible = propsHaveCompatibleTypes( - targetProp, - [...path, 'properties', name], - prop, - [...path, fieldName, i, 'properties', name] - ); - if (!propsAreCompatible) { - const error = { - message: `Duplicate property with incompatible type defined in schema within a oneOf/anyOf list: ${name}`, - path: [...path, fieldName, i.toString()] - }; - errors.push(error); - } - } else { - // Property doesn't exist in main schema, so just add it. - targetProperties[name] = prop; - } + // Only object schemas have properties + if (isObjectSchema(schema)) { + // Collects all composed property schemas indexed by the name of the property they define + const schemasByName = getPropertySchemasByName(schema); + + for (const propertyName in schemasByName) { + // The reducer will result in a `false` sentinel if two schemas for the same property + // are not deemed compatible + if ( + schemasByName[propertyName].reduce(schemaCompatibilityReducer) === false + ) { + errors.push({ + message: `SDK generation may fail due to incompatible types for property across composite object schema: ${propertyName}`, + path + }); } } } @@ -126,202 +47,43 @@ function checkSchemaList(schema, subSchemas, fieldName, path) { } /** - * Returns true iff "s" is an array with at least one element. - * @param {} s the object to check - * @returns boolean - */ -function isNonemptyArray(s) { - return Array.isArray(s) && s.length; -} - -/** - * Performs a rudimentary deep copy by simply marshalling, then unmarshalling "obj". - * @param {*} obj the object to copy - * @returns a deep copy of "obj" - */ -function copyObject(obj) { - return JSON.parse(JSON.stringify(obj)); -} - -/** - * This function is a wrapper around the "getSchemaType()" utility that maps - * the "ENUMERATION" type to be "STRING". We need to do this mapping to ensure that - * the type-compatibility checks will work correctly. - * @param {} s the schema - * @returns SchemaType enum value - */ -function getType(s) { - const schemaType = getSchemaType(s); - return schemaType === SchemaType.ENUMERATION ? SchemaType.STRING : schemaType; -} - -/** - * Returns true iff properties "p1" and "p2" have the same type. - * @param {*} p1 the first property to compare - * @param {*} p2 the second property to compare - * @returns boolean - */ -function propsHaveCompatibleTypes(p1, path1, p2, path2) { - if (isArraySchema(p1) && isArraySchema(p2)) { - return propsHaveCompatibleTypes(p1.items, [...path1, 'items'], p2.items, [ - ...path2, - 'items' - ]); - } - - const type1 = getType(p1); - const type2 = getType(p2); - - // If p1 and p2 are object schemas, then in order for them to be - // compatible in the context of this rule, they must have been defined - // with the same $ref within the original unresolved API definition. - // Unfortunately, we're using the resolved API def and so the $ref's have - // been resolved. To accommodate this, we'll try to "unresolve" the path - // associated with each of the properties to try to find the original object - // within the unresolved spec. - // If we're able to "unresolve" both properties, then we'll take the position - // that they SHOULD have a $ref and the $refs should be the same in order to - // declare the two properties compatible. - // If we're unable to unresolve one property or the other (which is not unexpected - // due to the rudimentary unresolver), then the next best thing would be to compare - // the JSON-stringified versions of p1 and p2 and if they are in fact exactly the same, - // we can make a (hopefully educated) guess that they originated from the same $ref. - if (type1 === SchemaType.OBJECT && type2 === SchemaType.OBJECT) { - // Unresolve each path to find the original object in the unresolved spec. - const p1Orig = unresolvePath(path1, unresolvedSpec); - const p2Orig = unresolvePath(path2, unresolvedSpec); - - if (p1Orig && p2Orig) { - // If both properties have a $ref field, then we'll compare them. - if (p1Orig.$ref && p2Orig.$ref) { - return p1Orig.$ref === p2Orig.$ref; - } else { - // We unresolved both paths successfully, but one or both of the original - // objects lacks a $ref field, so the properties are NOT compatible. - return false; - } - } - - // Just compare their JSON strings. - const p1String = JSON.stringify(p1); - const p2String = JSON.stringify(p2); - return p1String === p2String; - } - - // For non-object types, just compare the actual type values. - return type1 === type2; -} - -/** - * This function will try to "unresolve" the specified path (i.e. find the object - * corresponding to "resolvedPath" within the (original) unresolved API definition). - * - * The overall approach taken by this function is the following: - * Given the unresolved spec and a jsonpath from the resolved spec, the function - * will traverse the unresolved spec, one path segment at a time. At each hop, - * if the current object contains a $ref, then the "cursor" is changed from its current - * position (where the $ref was found) to the object referenced by the $ref. Then the traveral - * continues with the next path segment. Once we've exhausted the path segments, the final - * cursor postion is the desired object. - * This algorithm isn't foolproof: - * 1. The algorithm doesn't support external references. - * 2. "resolvedPath" may point to an object that has had allOf processing performed on it, therefore - * it might not be possible to find the corresponding object within the unresolved spec. - * 3. At any point if we're unable to traverse to the next path segment, we simply give up. + * Reducer for an array of schemas; for each pair of schemas returns one of them if they're deemed + * compatible and a `false` sentinel otherwise. The `false` sentinel is guaranteed to propagate. * - * @param {*} resolvedPath an array of path segments that specifies the location of - * an object within the resolved API definition. - * For example, for a scenario like the following: - * components: - * schemas: - * Foo: - * properties: - * foo: - * $ref: '#/components/schemas/Bar' - * Bar: - * properties: - * bar: - * type: string - * the path ['components', 'schemas', 'Foo', 'properties', 'foo', 'properties', 'bar'] would - * be mapped to the "bar" property within the Bar schema. - * @param {*} spec the unresolved API definition - * @returns the object within the unresolved API definition that corresponds to the - * object located at "resolvedPath" within the resolved API definition, or undefined if - * the mapping failes + * @param {*} left the "left" schema in the comparison + * @param {*} right the "right" schema in the comparison + * @returns a schema for the next comparison if the two are compatible, `false` otherwise */ -function unresolvePath(resolvedPath, spec) { - // Start our journey at the root of the unresolvedSpec. - let cursor = spec; - - for (const pathSeg of resolvedPath) { - // Make sure we're currently at a valid node in the document. - if (!cursor) { - return undefined; - } - - // If "cursor" is a $ref, then move to the referenced object before - // traversing to the "pathSeg" segment. - if (cursor.$ref) { - // Convert the reference to a jsonpath - // (e.g. '#/components/schemas/Bar' --> ['components', 'schemas', 'Bar']) - const newPath = refToPath(cursor.$ref); - if (!newPath || !newPath.length) { - return undefined; - } - - // Reset our cursor to the referenced object. - cursor = objectAtLocation(spec, newPath); - if (!cursor) { - return undefined; - } - } - - // Finally, take the next step in the path by traversing to the "pathSeg" segment. - cursor = objectAtLocation(cursor, [pathSeg]); - } - - return cursor; +function schemaCompatibilityReducer(left, right) { + return getComparandum(left) === getComparandum(right) ? left : false; } /** - * Starting at "rootObject", traverse to the object indicated by "path". - * @param {*} rootObject an object to be traversed - * @param {*} path an array of path segments indicating the location of the object - * (e.g. ['components', 'schemas', 'Foo', 'properties', 'foo']) - * @returns the object located at "path" or undefined if not found + * Returns the value appropriate to compare a schema to another schema. This is dependent on type. + * - For indeterminate schemas, deem incomparable and return a unique value + * - For object schemas, deem compatible only for an exact match and return the schema itself + * - For array schemas, deem compatible if their items are compatible (recursive) + * - For enumeration schemas, deem them compatible with strings + * - For all other schemas, deem them compatible with schemas of the same derived type and return it + * + * @param {*} schema the schema from which to derive a "comparandum" to compare with other schemas + * @returns a "comparandum" value to compare with other schemas */ -function objectAtLocation(rootObject, path) { - let obj = rootObject; - for (const segment of path) { - if (Array.isArray(obj)) { - const index = typeof segment === 'number' ? segment : parseInt(segment); - obj = obj[index]; - } else { - obj = obj[segment]; - } - - if (!obj) { - return undefined; - } +function getComparandum(schema) { + if (!isObject(schema) || getSchemaType(schema) === SchemaType.UNKNOWN) { + // not compatible with any other schema + return Symbol(); } - - return obj; -} - -/** - * Returns the path for the specified internal reference. - * @param {*} ref the reference (e.g. '#/components/schemas/Foo') - * @returns an array of path segments (e.g. ['components', 'schemas', 'Foo']) - * or undefined if "ref" is not an internal reference. - */ -function refToPath(ref) { - const parts = ref.split('#'); - - // Make sure that "ref" is an internal $ref. - const filename = parts[0] ? parts[0].trim() : undefined; - if (filename) { - return undefined; + if (isObjectSchema(schema)) { + // only compatible with same exact schema + return schema; + } else if (isArraySchema(schema)) { + // compatible with other arrays whose values are compatible + return getComparandum(schema.items); + } else if (isEnumerationSchema(schema)) { + // compatible with all strings + return SchemaType.STRING; + } else { + return getSchemaType(schema); } - - return parts[1].split('/').slice(1); } diff --git a/packages/ruleset/src/utils/get-property-schemas-by-name.js b/packages/ruleset/src/utils/get-property-schemas-by-name.js new file mode 100644 index 000000000..7c9549ede --- /dev/null +++ b/packages/ruleset/src/utils/get-property-schemas-by-name.js @@ -0,0 +1,52 @@ +const isObject = require('./is-object'); + +/** + * Returns a dictionary object of property schemas in arrays keyed by property name for a simple or + * composite schema. + * + * @param {object} schema - Simple or composite OpenAPI 3.0 schema object. + * @returns {object} - Dictionary mapping property name to collected schemas[] + */ +const getPropertySchemasByName = schema => { + let propertyDictionary = {}; + + if (!isObject(schema)) { + return propertyDictionary; + } + + if (isObject(schema.properties)) { + for (const propertyName of Object.keys(schema.properties)) { + propertyDictionary[propertyName] = [schema.properties[propertyName]]; + } + } + + for (const applicatorType of ['allOf', 'oneOf', 'anyOf']) { + if (Array.isArray(schema[applicatorType])) { + for (const applicatorSchema of schema[applicatorType]) { + propertyDictionary = mergeDictionaries( + propertyDictionary, + getPropertySchemasByName(applicatorSchema) + ); + } + } + } + + return propertyDictionary; +}; + +function mergeDictionaries(...dictionaries) { + const newDictionary = {}; + + for (const dictionary of dictionaries) { + for (const propertyName of Object.keys(dictionary)) { + if (!Array.isArray(newDictionary[propertyName])) { + newDictionary[propertyName] = []; + } + newDictionary[propertyName].push(...dictionary[propertyName]); + } + } + + return newDictionary; +} + +module.exports = getPropertySchemasByName; diff --git a/packages/ruleset/src/utils/index.js b/packages/ruleset/src/utils/index.js index 6cad71835..bf0c1f500 100644 --- a/packages/ruleset/src/utils/index.js +++ b/packages/ruleset/src/utils/index.js @@ -3,6 +3,7 @@ module.exports = { checkCompositeSchemaForProperty: require('./check-composite-schema-for-property'), getCompositeSchemaAttribute: require('./get-composite-schema-attribute'), getPropertyNamesForSchema: require('./get-property-names-for-schema'), + getPropertySchemasByName: require('./get-property-schemas-by-name'), isDeprecated: require('./is-deprecated'), isEmptyObjectSchema: require('./is-empty-object-schema'), isObject: require('./is-object'), diff --git a/packages/ruleset/test/composed-schema-restrictions.test.js b/packages/ruleset/test/composed-schema-restrictions.test.js index cb1c63402..c07fece72 100644 --- a/packages/ruleset/test/composed-schema-restrictions.test.js +++ b/packages/ruleset/test/composed-schema-restrictions.test.js @@ -5,7 +5,7 @@ const rule = composedSchemaRestrictions; const ruleId = 'composed-schema-restrictions'; const expectedSeverity = severityCodes.warning; const expectedMsgObjSchema = `A schema within an object schema's oneOf/anyOf list must be an object schema`; -const expectedMsgProp = `Duplicate property with incompatible type defined in schema within a oneOf/anyOf list:`; +const expectedMsgProp = `SDK generation may fail due to incompatible types for property across composite object schema:`; describe('Spectral rule: schema-description', () => { describe('Should not yield errors', () => { @@ -284,7 +284,7 @@ describe('Spectral rule: schema-description', () => { expect(results[0].message).toBe(expectedMsgObjSchema); expect(results[0].severity).toBe(expectedSeverity); expect(results[0].path.join('.')).toBe( - 'paths./v1/drinks.get.responses.200.content.application/json.schema.allOf.1.properties.drinks.items.additionalProperties.anyOf.0' + 'paths./v1/drinks.get.responses.200.content.application/json.schema.allOf.1.properties.drinks.items.additionalProperties' ); }); @@ -319,7 +319,7 @@ describe('Spectral rule: schema-description', () => { expect(results[0].message).toBe(`${expectedMsgProp} drink_size`); expect(results[0].severity).toBe(expectedSeverity); expect(results[0].path.join('.')).toBe( - 'paths./v1/drinks.get.responses.200.content.application/json.schema.allOf.1.properties.drinks.items.oneOf.0' + 'paths./v1/drinks.get.responses.200.content.application/json.schema.allOf.1.properties.drinks.items' ); }); @@ -353,7 +353,7 @@ describe('Spectral rule: schema-description', () => { expect(results[0].message).toBe(`${expectedMsgProp} bad_prop`); expect(results[0].severity).toBe(expectedSeverity); expect(results[0].path.join('.')).toBe( - 'paths./v1/drinks.get.responses.200.content.application/json.schema.allOf.1.properties.drinks.items.oneOf.1' + 'paths./v1/drinks.get.responses.200.content.application/json.schema.allOf.1.properties.drinks.items' ); }); @@ -399,7 +399,7 @@ describe('Spectral rule: schema-description', () => { expect(results[0].message).toBe(`${expectedMsgProp} bad_prop`); expect(results[0].severity).toBe(expectedSeverity); expect(results[0].path.join('.')).toBe( - 'paths./v1/drinks.get.responses.200.content.application/json.schema.allOf.1.properties.drinks.items.oneOf.1' + 'paths./v1/drinks.get.responses.200.content.application/json.schema.allOf.1.properties.drinks.items' ); }); @@ -465,7 +465,7 @@ describe('Spectral rule: schema-description', () => { expect(results[0].message).toBe(`${expectedMsgProp} bad_prop`); expect(results[0].severity).toBe(expectedSeverity); expect(results[0].path.join('.')).toBe( - 'paths./v1/drinks.get.responses.200.content.application/json.schema.allOf.1.properties.drinks.items.oneOf.1' + 'paths./v1/drinks.get.responses.200.content.application/json.schema.allOf.1.properties.drinks.items' ); }); @@ -516,7 +516,7 @@ describe('Spectral rule: schema-description', () => { expect(results[0].message).toBe(`${expectedMsgProp} object_prop`); expect(results[0].severity).toBe(expectedSeverity); expect(results[0].path.join('.')).toBe( - 'paths./v1/drinks.get.responses.200.content.application/json.schema.allOf.1.properties.drinks.items.oneOf.0' + 'paths./v1/drinks.get.responses.200.content.application/json.schema.allOf.1.properties.drinks.items' ); }); @@ -567,7 +567,7 @@ describe('Spectral rule: schema-description', () => { expect(results[0].message).toBe(`${expectedMsgProp} object_prop`); expect(results[0].severity).toBe(expectedSeverity); expect(results[0].path.join('.')).toBe( - 'paths./v1/drinks.get.responses.200.content.application/json.schema.allOf.1.properties.drinks.items.oneOf.0' + 'paths./v1/drinks.get.responses.200.content.application/json.schema.allOf.1.properties.drinks.items' ); }); @@ -618,7 +618,7 @@ describe('Spectral rule: schema-description', () => { expect(results[0].message).toBe(`${expectedMsgProp} object_prop`); expect(results[0].severity).toBe(expectedSeverity); expect(results[0].path.join('.')).toBe( - 'paths./v1/drinks.get.responses.200.content.application/json.schema.allOf.1.properties.drinks.items.oneOf.0' + 'paths./v1/drinks.get.responses.200.content.application/json.schema.allOf.1.properties.drinks.items' ); }); }); From 727bad89de15c80156f4a3fce8f47399d14dce76 Mon Sep 17 00:00:00 2001 From: Dan Hudlow Date: Thu, 10 Nov 2022 23:10:51 -0600 Subject: [PATCH 4/4] refactor(schema-type): make schema-type much more sophisticated --- packages/ruleset/src/functions/schema-type.js | 86 +++++++++++++---- packages/ruleset/src/rules/schema-type.js | 5 +- .../src/utils/get-all-composed-schemas.js | 46 ++++++++++ .../utils/get-property-names-for-schema.js | 33 +++---- .../src/utils/get-property-schemas-by-name.js | 49 +++------- packages/ruleset/src/utils/get-schema-type.js | 92 +++++++++++++++---- packages/ruleset/src/utils/index.js | 1 + 7 files changed, 215 insertions(+), 97 deletions(-) create mode 100644 packages/ruleset/src/utils/get-all-composed-schemas.js diff --git a/packages/ruleset/src/functions/schema-type.js b/packages/ruleset/src/functions/schema-type.js index 089135545..2f835957c 100644 --- a/packages/ruleset/src/functions/schema-type.js +++ b/packages/ruleset/src/functions/schema-type.js @@ -1,31 +1,77 @@ -const { mergeAllOfSchemaProperties, validateSubschemas } = require('../utils'); +const { + SchemaType, + JSONSchemaType, + JSONSchemaFormat, + checkCompositeSchemaForConstraint, + getAllComposedSchemas, + getSchemaType, + validateNestedSchemas +} = require('../utils'); -module.exports = function(schema, _opts, { path }) { - return validateSubschemas(schema, path, schemaType); +module.exports = function(schema, options, { path }) { + return validateNestedSchemas(schema, path, schemaType); }; function schemaType(schema, path) { - // If we're looking at an allOf list element schema, then - // bail out as this would not necessarily provide the full - // definition of a schema or schema property. - if (path[path.length - 2] === 'allOf') { + const type = getSchemaType(schema); + let message = null; + + if (type === SchemaType.UNKNOWN) { + message = + 'Schema does not have a clear, consistent combination of `type` and `format`.'; + } else if (hasAmbiguousType(schema, type)) { + message = '`type` is undefined for a variation of the schema.'; + } else if (hasContradictoryType(schema, type)) { + message = '`type` is contradictory in the schema composition.'; + } else if (type in JSONSchemaFormat) { + if (hasAmbiguousFormat(schema, type)) { + message = '`format` is undefined for a variation of the schema.'; + } else if (hasContradictoryFormat(schema, type)) { + message = '`format` is contradictory in the schema composition.'; + } + } + + if (message !== null) { + return [{ message, path }]; + } else { return []; } +} - const mergedSchema = mergeAllOfSchemaProperties(schema); +const hasAmbiguousType = (schema, type) => { + return !checkCompositeSchemaForConstraint( + schema, + s => s.type === JSONSchemaType[type] + ); +}; + +const hasContradictoryType = (schema, type) => { + const schemas = getAllComposedSchemas(schema); - if (!schemaHasType(mergedSchema)) { - return [ - { - message: 'Schema should have a non-empty `type` field.', - path - } - ]; + for (const s of schemas) { + if ('type' in s && s.type !== JSONSchemaType[type]) { + return true; + } } - return []; -} + return false; +}; -function schemaHasType(s) { - return s && s.type && s.type.toString().trim().length; -} +const hasAmbiguousFormat = (schema, type) => { + return !checkCompositeSchemaForConstraint( + schema, + s => s.format === JSONSchemaFormat[type] + ); +}; + +const hasContradictoryFormat = (schema, type) => { + const schemas = getAllComposedSchemas(schema); + + for (const s of schemas) { + if ('format' in s && s.format !== JSONSchemaFormat[type]) { + return true; + } + } + + return false; +}; diff --git a/packages/ruleset/src/rules/schema-type.js b/packages/ruleset/src/rules/schema-type.js index 43836ae63..eaddef932 100644 --- a/packages/ruleset/src/rules/schema-type.js +++ b/packages/ruleset/src/rules/schema-type.js @@ -3,11 +3,10 @@ const { schemaType } = require('../functions'); const { schemas } = require('../collections'); module.exports = { - description: - 'Schemas and schema properties should have a non-empty `type` field. **This rule is disabled by default.**', + description: 'Schemas must have an explicit and consistent type', message: '{{error}}', given: schemas, - severity: 'off', + severity: 'warn', formats: [oas2, oas3], resolved: true, then: { diff --git a/packages/ruleset/src/utils/get-all-composed-schemas.js b/packages/ruleset/src/utils/get-all-composed-schemas.js new file mode 100644 index 000000000..1e291a710 --- /dev/null +++ b/packages/ruleset/src/utils/get-all-composed-schemas.js @@ -0,0 +1,46 @@ +const isObject = require('./is-object'); + +/** + * Returns an array of all schemas composed into a schema, optionally filtered by a lambda function, + * and optionally including the schema itself. This function is useful if you need to see across + * all composed schemas without caring about the exact implication of the schema (such as whether it + * represents a necessary constraint as in `allOf` or a possible constraint such as in `oneOf`). + * + * @param {object} schema - Simple or composite OpenAPI 3.0 schema object. + * @param {function} schemaFilter(schema, applicators) - Lambda filter for schemas + * @param {boolean} includeSelf - Option to include the schema itself (defaults to `true`) + * @returns {array} - Array of composed schemas + */ +const getAllComposedSchemas = ( + schema, + schemaFilter = () => true, + includeSelf = true, + applicators = [] +) => { + const schemas = []; + + if (!isObject(schema)) { + return schemas; + } + + if (includeSelf && schemaFilter(schema, applicators)) { + schemas.push(schema); + } + + for (const applicatorType of ['allOf', 'oneOf', 'anyOf']) { + if (Array.isArray(schema[applicatorType])) { + for (const applicatorSchema of schema[applicatorType]) { + schemas.push( + ...getAllComposedSchemas(applicatorSchema, schemaFilter, true, [ + ...applicators, + applicatorType + ]) + ); + } + } + } + + return [...new Set(schemas)]; // de-duplicate +}; + +module.exports = getAllComposedSchemas; diff --git a/packages/ruleset/src/utils/get-property-names-for-schema.js b/packages/ruleset/src/utils/get-property-names-for-schema.js index 9189e1c8b..e058ee768 100644 --- a/packages/ruleset/src/utils/get-property-names-for-schema.js +++ b/packages/ruleset/src/utils/get-property-names-for-schema.js @@ -1,35 +1,24 @@ +const getAllComposedSchemas = require('./get-all-composed-schemas'); const isObject = require('./is-object'); /** * Returns an array of property names for a simple or composite schema, * optionally filtered by a lambda function. * - * @param {object} schema - Simple or composite OpenAPI 3.0 schema object. - * @param {array} path - Path array for the provided schema. - * @param {function} validate - Validate function. - * @returns {array} - Array of validation errors. + * @param {object} schema - Simple or composite OpenAPI 3.0 schema object + * @param {function} schemaFilter(propertyName, propertySchema) - Lambda filter for properties + * @returns {array} - Array of property names defined for a schema */ const getPropertyNamesForSchema = (schema, propertyFilter = () => true) => { const propertyNames = []; + const schemas = getAllComposedSchemas(schema); - if (!isObject(schema)) { - return propertyNames; - } - - if (isObject(schema.properties)) { - for (const propertyName of Object.keys(schema.properties)) { - if (propertyFilter(propertyName, schema.properties[propertyName])) { - propertyNames.push(propertyName); - } - } - } - - for (const applicatorType of ['allOf', 'oneOf', 'anyOf']) { - if (Array.isArray(schema[applicatorType])) { - for (const applicatorSchema of schema[applicatorType]) { - propertyNames.push( - ...getPropertyNamesForSchema(applicatorSchema, propertyFilter) - ); + for (const s of schemas) { + if (isObject(s.properties)) { + for (const propertyName of Object.keys(s.properties)) { + if (propertyFilter(propertyName, s.properties[propertyName])) { + propertyNames.push(propertyName); + } } } } diff --git a/packages/ruleset/src/utils/get-property-schemas-by-name.js b/packages/ruleset/src/utils/get-property-schemas-by-name.js index 7c9549ede..346c8c0d0 100644 --- a/packages/ruleset/src/utils/get-property-schemas-by-name.js +++ b/packages/ruleset/src/utils/get-property-schemas-by-name.js @@ -1,52 +1,29 @@ +const getAllComposedSchemas = require('./get-all-composed-schemas'); const isObject = require('./is-object'); /** * Returns a dictionary object of property schemas in arrays keyed by property name for a simple or * composite schema. * - * @param {object} schema - Simple or composite OpenAPI 3.0 schema object. + * @param {object} schema - Simple or composite OpenAPI 3.0 schema object * @returns {object} - Dictionary mapping property name to collected schemas[] */ const getPropertySchemasByName = schema => { - let propertyDictionary = {}; - - if (!isObject(schema)) { - return propertyDictionary; - } - - if (isObject(schema.properties)) { - for (const propertyName of Object.keys(schema.properties)) { - propertyDictionary[propertyName] = [schema.properties[propertyName]]; - } - } - - for (const applicatorType of ['allOf', 'oneOf', 'anyOf']) { - if (Array.isArray(schema[applicatorType])) { - for (const applicatorSchema of schema[applicatorType]) { - propertyDictionary = mergeDictionaries( - propertyDictionary, - getPropertySchemasByName(applicatorSchema) - ); + const dictionary = {}; + const schemas = getAllComposedSchemas(schema); + + for (const s of schemas) { + if (isObject(s.properties)) { + for (const propertyName of Object.keys(s.properties)) { + if (!Array.isArray(dictionary[propertyName])) { + dictionary[propertyName] = []; + } + dictionary[propertyName].push(s.properties[propertyName]); } } } - return propertyDictionary; + return dictionary; }; -function mergeDictionaries(...dictionaries) { - const newDictionary = {}; - - for (const dictionary of dictionaries) { - for (const propertyName of Object.keys(dictionary)) { - if (!Array.isArray(newDictionary[propertyName])) { - newDictionary[propertyName] = []; - } - newDictionary[propertyName].push(...dictionary[propertyName]); - } - } - - return newDictionary; -} - module.exports = getPropertySchemasByName; diff --git a/packages/ruleset/src/utils/get-schema-type.js b/packages/ruleset/src/utils/get-schema-type.js index fb49c554a..7d89ef2c6 100644 --- a/packages/ruleset/src/utils/get-schema-type.js +++ b/packages/ruleset/src/utils/get-schema-type.js @@ -28,8 +28,37 @@ const SchemaType = { UNKNOWN: Symbol('unknown') }; +const JSONSchemaType = { + [SchemaType.ARRAY]: 'array', + [SchemaType.BINARY]: 'string', + [SchemaType.BOOLEAN]: 'boolean', + [SchemaType.BYTE]: 'string', + [SchemaType.DATE]: 'string', + [SchemaType.DATE_TIME]: 'string', + [SchemaType.DOUBLE]: 'number', + [SchemaType.ENUMERATION]: 'string', + [SchemaType.FLOAT]: 'number', + [SchemaType.INT32]: 'integer', + [SchemaType.INT64]: 'integer', + [SchemaType.INTEGER]: 'integer', + [SchemaType.NUMBER]: 'number', + [SchemaType.OBJECT]: 'object', + [SchemaType.STRING]: 'string' +}; + +const JSONSchemaFormat = { + [SchemaType.BINARY]: 'binary', + [SchemaType.BYTE]: 'byte', + [SchemaType.DATE]: 'date', + [SchemaType.DATE_TIME]: 'date-time', + [SchemaType.DOUBLE]: 'double', + [SchemaType.FLOAT]: 'float', + [SchemaType.INT32]: 'int32', + [SchemaType.INT64]: 'int64' +}; + /** - * Returns a symbol from SchemaType based on the most specific schema type detected for a schema. + *] Returns a symbol from SchemaType based on the most specific schema type detected for a schema. * * This function is a heuristic and does not attempt to account for contradictions, schemas which * give no consistent indication of type, or OAS 3.1 schemas which use a type array. It also does @@ -108,7 +137,7 @@ const getSchemaType = schema => { const isArraySchema = schema => { return checkCompositeSchemaForConstraint(schema, s => { if ('type' in s) { - return s.type === 'array'; // ignores the possibility of type arrays in OAS 3.1 + return s.type === JSONSchemaType[SchemaType.ARRAY]; // ignores the possibility of type arrays in OAS 3.1 } else { return isObject(s.items); } @@ -124,7 +153,9 @@ const isArraySchema = schema => { const isBinarySchema = schema => { return checkCompositeSchemaForConstraint( schema, - s => s.type === 'string' && s.format === 'binary' + s => + s.type === JSONSchemaType[SchemaType.BINARY] && + s.format === JSONSchemaFormat[SchemaType.BINARY] ); }; @@ -135,7 +166,10 @@ const isBinarySchema = schema => { * @returns {boolean} */ const isBooleanSchema = schema => { - return checkCompositeSchemaForConstraint(schema, s => s.type === 'boolean'); + return checkCompositeSchemaForConstraint( + schema, + s => s.type === JSONSchemaType[SchemaType.BOOLEAN] + ); }; /** @@ -147,7 +181,9 @@ const isBooleanSchema = schema => { const isByteSchema = schema => { return checkCompositeSchemaForConstraint( schema, - s => s.type === 'string' && s.format === 'byte' + s => + s.type === JSONSchemaType[SchemaType.BYTE] && + s.format === JSONSchemaFormat[SchemaType.BYTE] ); }; @@ -160,7 +196,9 @@ const isByteSchema = schema => { const isDateSchema = schema => { return checkCompositeSchemaForConstraint( schema, - s => s.type === 'string' && s.format === 'date' + s => + s.type === JSONSchemaType[SchemaType.DATE] && + s.format === JSONSchemaFormat[SchemaType.DATE] ); }; @@ -173,7 +211,9 @@ const isDateSchema = schema => { const isDateTimeSchema = schema => { return checkCompositeSchemaForConstraint( schema, - s => s.type === 'string' && s.format === 'date-time' + s => + s.type === JSONSchemaType[SchemaType.DATE] && + s.format === JSONSchemaFormat[SchemaType.DATE_TIME] ); }; @@ -187,7 +227,9 @@ const isDoubleSchema = schema => { // also ignores `type` and `format` defined in separately composited schemas return checkCompositeSchemaForConstraint( schema, - s => s.type === 'number' && s.format === 'double' + s => + s.type === JSONSchemaType[SchemaType.DOUBLE] && + s.format === JSONSchemaFormat[SchemaType.DOUBLE] ); }; @@ -200,7 +242,8 @@ const isDoubleSchema = schema => { const isEnumerationSchema = schema => { return checkCompositeSchemaForConstraint( schema, - s => s.type === 'string' && Array.isArray(s.enum) + s => + s.type === JSONSchemaType[SchemaType.ENUMERATION] && Array.isArray(s.enum) ); }; @@ -214,7 +257,9 @@ const isFloatSchema = schema => { // also ignores `type` and `format` defined in separately composited schemas return checkCompositeSchemaForConstraint( schema, - s => s.type === 'number' && s.format === 'float' + s => + s.type === JSONSchemaType[SchemaType.FLOAT] && + s.format === JSONSchemaFormat[SchemaType.FLOAT] ); }; @@ -227,7 +272,9 @@ const isFloatSchema = schema => { const isInt32Schema = schema => { return checkCompositeSchemaForConstraint( schema, - s => s.type === 'integer' && s.format === 'int32' + s => + s.type === JSONSchemaType[SchemaType.INT32] && + s.format === JSONSchemaFormat[SchemaType.INT32] ); }; @@ -240,7 +287,9 @@ const isInt32Schema = schema => { const isInt64Schema = schema => { return checkCompositeSchemaForConstraint( schema, - s => s.type === 'integer' && s.format === 'int64' + s => + s.type === JSONSchemaType[SchemaType.INT64] && + s.format === JSONSchemaFormat[SchemaType.INT64] ); }; @@ -251,7 +300,10 @@ const isInt64Schema = schema => { * @returns {boolean} */ const isIntegerSchema = schema => { - return checkCompositeSchemaForConstraint(schema, s => s.type === 'integer'); + return checkCompositeSchemaForConstraint( + schema, + s => s.type === JSONSchemaType[SchemaType.INTEGER] + ); }; /** @@ -261,7 +313,10 @@ const isIntegerSchema = schema => { * @returns {boolean} */ const isNumberSchema = schema => { - return checkCompositeSchemaForConstraint(schema, s => s.type === 'number'); + return checkCompositeSchemaForConstraint( + schema, + s => s.type === JSONSchemaType[SchemaType.NUMBER] + ); }; /** @@ -273,7 +328,7 @@ const isNumberSchema = schema => { const isObjectSchema = schema => { return checkCompositeSchemaForConstraint(schema, s => { if ('type' in s) { - return s.type === 'object'; // ignores the possibility of type arrays in OAS 3.1 + return s.type === JSONSchemaType[SchemaType.OBJECT]; // ignores the possibility of type arrays in OAS 3.1 } else { return ( isObject(s.properties) || @@ -291,7 +346,10 @@ const isObjectSchema = schema => { * @returns {boolean} */ const isStringSchema = schema => { - return checkCompositeSchemaForConstraint(schema, s => s.type === 'string'); + return checkCompositeSchemaForConstraint( + schema, + s => s.type === JSONSchemaType[SchemaType.STRING] + ); }; /** @@ -319,6 +377,8 @@ function isPrimitiveSchema(s) { module.exports = { SchemaType, + JSONSchemaType, + JSONSchemaFormat, getSchemaType, isArraySchema, isBinarySchema, diff --git a/packages/ruleset/src/utils/index.js b/packages/ruleset/src/utils/index.js index bf0c1f500..9bc680ece 100644 --- a/packages/ruleset/src/utils/index.js +++ b/packages/ruleset/src/utils/index.js @@ -1,6 +1,7 @@ module.exports = { checkCompositeSchemaForConstraint: require('./check-composite-schema-for-constraint'), checkCompositeSchemaForProperty: require('./check-composite-schema-for-property'), + getAllComposedSchemas: require('./get-all-composed-schemas'), getCompositeSchemaAttribute: require('./get-composite-schema-attribute'), getPropertyNamesForSchema: require('./get-property-names-for-schema'), getPropertySchemasByName: require('./get-property-schemas-by-name'),
composed-schema-restrictionserrorVerifies that schema compositions involving oneOf and anyOf comply with the -restrictions imposed by the SDK generator.warnVerifies that schema compositions involving oneOf and anyOf comply +with best practices associated with SDK generation. oas3
Description:This rule examines each schema that inclues a oneOf/anyOf composition and verifies that -it complies with the restrictions imposed by the SDK generator. The restrictions are: +This rule examines each schema that includes a oneOf/anyOf composition and verifies that +it complies with SDK generation best practices. +

These best practices include the following restrictions:

  • Any object schema containing a oneOf/anyOf composition must include only object schemas within the oneOf/anyOf list.
  • The union of the properties defined by the main schema and its -oneOf/anyOf sub-schemas is examined to detect any like-named properties defined by two or more of the schemas. -Like-named properties that are found to be of different types will trigger an error.
    • +oneOf/anyOf sub-schemas is examined to detect any like-named (common) properties defined by two or more of the schemas. +These common properties must be defined with the same datatype.
Severity:errorwarn
OAS Versions:
Description: A response schema should be defined as a reference to a named schema instead of defined as an inline schema. -This is a best practice because the SDK generator will use the schema reference when determining the operation's return type +This is a best practice because IBM's SDK generator will use the schema reference when determining the operation's return type within the generated SDK code. -

The SDK generator will refactor any inline response schemas that it finds by moving them to the components.schemas -section of the API definition and then replacing them with a reference. However, the names computed by the SDK generator are +

IBM's SDK generator will refactor any inline response schemas that it finds by moving them to the components.schemas +section of the API definition and then replacing them with a reference. However, the names computed by the generator are not optimal (e.g. GetThingResponse), so the recommendation is for API authors to define the response schema as a $ref.