Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: work-in-progress proposed refactor of composed schema restrictions #506

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
107 changes: 100 additions & 7 deletions docs/ibm-cloud-rules.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -178,6 +179,13 @@ within the operation's path string, which should also match the plural form of t
<td>oas2, oas3</td>
</tr>
<tr>
<td><a href="#rule-composed-schema-restrictions">composed-schema-restrictions</a></td>
<td>warn</td>
<td>Verifies that schema compositions involving <code>oneOf</code> and <code>anyOf</code> comply
with best practices associated with SDK generation.</td>
<td>oas3</td>
</tr>
<tr>
<td><a href="#rule-consecutive-path-param-segments">consecutive-path-param-segments</a></td>
<td>error</td>
<td>Checks each path string in the API definition to detect the presence of two or more consecutive
Expand Down Expand Up @@ -1321,6 +1329,91 @@ paths:
</table>


### Rule: composed-schema-restrictions
<table>
<tr>
<td><b>Rule id:</b></td>
<td><b>composed-schema-restrictions</b></td>
</tr>
<tr>
<td valign=top><b>Description:</b></td>
<td>This rule examines each schema that includes a oneOf/anyOf composition and verifies that
it complies with SDK generation best practices.
<p>These best practices include the following restrictions:
<ul>
<li>Any object schema containing a oneOf/anyOf composition must include only object schemas within
the oneOf/anyOf list.</li>
<li>The union of the properties defined by the main schema and its
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.</li>
</ul>
</td>
</tr>
<tr>
<td><b>Severity:</b></td>
<td>warn</td>
</tr>
<tr>
<td><b>OAS Versions:</b></td>
<td>oas3</td>
</tr>
<tr>
<td valign=top><b>Non-compliant example:<b></td>
<td>
<pre>
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 &lt;&lt;&lt; incompatible type
some_other_property:
type: integer
</pre>
</td>
</tr>
<tr>
<td valign=top><b>Compliant example:</b></td>
<td>
<pre>
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: integer
</pre>
</td>
</tr>
</table>


### Rule: consecutive-path-param-segments
<table>
<tr>
Expand Down Expand Up @@ -2245,12 +2338,12 @@ it is a best practice to define the schema as a named schema within the <code>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).
<p>The use of a schema $ref is preferred instead of a nested object schema, because the SDK generator will
<p>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 <code>components.schemas</code>
If IBM's SDK generator encounters a nested object schema, it must refactor it by moving it to the <code>components.schemas</code>
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.
</td>
</tr>
Expand Down Expand Up @@ -2404,10 +2497,10 @@ paths:
<tr>
<td valign=top><b>Description:</b></td>
<td>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.
<p>The SDK generator will refactor any inline response schemas that it finds by moving them to the <code>components.schemas</code>
section of the API definition and then replacing them with a reference. However, the names computed by the SDK generator are
<p>IBM's SDK generator will refactor any inline response schemas that it finds by moving them to the <code>components.schemas</code>
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.
</td>
</tr>
Expand Down
10 changes: 6 additions & 4 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions packages/ruleset/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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": {
Expand Down
89 changes: 89 additions & 0 deletions packages/ruleset/src/functions/composed-schema-restrictions.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
const {
validateSubschemas,
getPropertySchemasByName,
getSchemaType,
isObjectSchema,
isArraySchema,
isEnumerationSchema,
isObject,
SchemaType
} = require('../utils');

module.exports = function(schema, options, { path }) {
return validateSubschemas(schema, path, composedSchemaRestrictions);
};

/**
* 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) {
const errors = [];

// Only object schemas have properties
if (isObjectSchema(schema)) {
// Collects all composed property schemas indexed by the name of the property they define
Copy link
Member

@dpopp07 dpopp07 Oct 18, 2022

Choose a reason for hiding this comment

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

Would be easier to read (for me, at least) if this comment elaborated slightly more on the data structure returned by getPropertySchemasByName. Something to let the reader know the map values are lists of schemas all defining properties of common names.

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
});
}
}
}

return errors;
}

/**
* 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 {*} 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 schemaCompatibilityReducer(left, right) {
return getComparandum(left) === getComparandum(right) ? left : false;
}

/**
* 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 getComparandum(schema) {
if (!isObject(schema) || getSchemaType(schema) === SchemaType.UNKNOWN) {
// not compatible with any other schema
return Symbol();
}
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);
}
}
1 change: 1 addition & 0 deletions packages/ruleset/src/functions/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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'),
Expand Down
18 changes: 10 additions & 8 deletions packages/ruleset/src/functions/inline-schema-rules.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ const {
isJsonMimeType,
isArraySchema,
isEmptyObjectSchema,
isPrimitiveType,
isPrimitiveSchema,
isRefSiblingSchema,
validateSubschemas
} = require('../utils');
Expand Down Expand Up @@ -35,7 +35,7 @@ function inlineResponseSchema(schema, options, { path }) {
if (
!schema.$ref &&
isJsonMimeType(mimeType) &&
!isPrimitiveType(schema) &&
!isPrimitiveSchema(schema) &&
!arrayItemsAreRefOrPrimitive(schema) &&
!isRefSiblingSchema(schema)
) {
Expand Down Expand Up @@ -72,7 +72,7 @@ function inlineRequestSchema(schema, options, { path }) {
if (
!schema.$ref &&
isJsonMimeType(mimeType) &&
!isPrimitiveType(schema) &&
!isPrimitiveSchema(schema) &&
!arrayItemsAreRefOrPrimitive(schema) &&
!isRefSiblingSchema(schema)
) {
Expand Down Expand Up @@ -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);
}

/**
Expand All @@ -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)
Expand Down Expand Up @@ -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));
}
1 change: 1 addition & 0 deletions packages/ruleset/src/ibm-oas.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
16 changes: 16 additions & 0 deletions packages/ruleset/src/rules/composed-schema-restrictions.js
Original file line number Diff line number Diff line change
@@ -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 SDK generation best practices',
message: '{{error}}',
given: schemas,
severity: 'warn',
formats: [oas3],
resolved: true,
then: {
function: composedSchemaRestrictions
}
};
1 change: 1 addition & 0 deletions packages/ruleset/src/rules/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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'),
Expand Down
Loading