diff --git a/tools/spectral/ipa/__tests__/IPA113ResetMethodNotOnReadonlySingleton.test.js b/tools/spectral/ipa/__tests__/IPA113ResetMethodNotOnReadonlySingleton.test.js index 49a40c6c32..e0414069c9 100644 --- a/tools/spectral/ipa/__tests__/IPA113ResetMethodNotOnReadonlySingleton.test.js +++ b/tools/spectral/ipa/__tests__/IPA113ResetMethodNotOnReadonlySingleton.test.js @@ -32,6 +32,47 @@ testRule('xgen-IPA-113-reset-method-not-on-readonly-singleton', [ }, errors: [], }, + { + name: 'valid reset when base path returns list response shape', + document: { + paths: { + '/resource/{exampleId}/listSingleton': { + get: { + responses: { + 200: { + content: { + 'application/json': { + schema: { + type: 'object', + properties: { + results: { + type: 'array', + readOnly: true, + items: { + type: 'object', + properties: { + id: { type: 'string', readOnly: true }, + status: { type: 'string', readOnly: true }, + }, + }, + }, + totalCount: { type: 'integer', readOnly: true }, + }, + required: ['results'], + }, + }, + }, + }, + }, + }, + }, + '/resource/{exampleId}/listSingleton:reset': { + post: {}, + }, + }, + }, + errors: [], + }, { name: 'invalid reset on read-only singleton', document: { diff --git a/tools/spectral/ipa/__tests__/IPA113SingletonHasUpdateMethod.test.js b/tools/spectral/ipa/__tests__/IPA113SingletonHasUpdateMethod.test.js index 703e702043..9ce598006f 100644 --- a/tools/spectral/ipa/__tests__/IPA113SingletonHasUpdateMethod.test.js +++ b/tools/spectral/ipa/__tests__/IPA113SingletonHasUpdateMethod.test.js @@ -95,7 +95,164 @@ testRule('xgen-IPA-113-singleton-should-have-update-method', [ errors: [], }, { - name: 'read-only singleton with List response', + name: 'read-only singleton with referenced nested schema does not require update method', + document: { + paths: { + '/resource/{exampleId}/readOnlySingleton': { + get: { + responses: { + 200: { + content: { + 'application/json': { + schema: { + $ref: '#/components/schemas/ReadOnlySingleton', + }, + }, + }, + }, + }, + }, + }, + }, + components: { + schemas: { + ReadOnlySingleton: { + type: 'object', + properties: { + status: { type: 'string', readOnly: true }, + metadata: { + $ref: '#/components/schemas/ReadOnlyMetadata', + }, + }, + }, + ReadOnlyMetadata: { + type: 'object', + properties: { + createdAt: { type: 'string', readOnly: true }, + updatedAt: { type: 'string', readOnly: true }, + }, + }, + }, + }, + }, + errors: [], + }, + { + name: 'singleton with writable property in referenced nested schema requires update method', + document: { + paths: { + '/resource/{exampleId}/writableSingleton': { + get: { + responses: { + 200: { + content: { + 'application/json': { + schema: { + $ref: '#/components/schemas/WritableSingleton', + }, + }, + }, + }, + }, + }, + }, + }, + components: { + schemas: { + WritableSingleton: { + type: 'object', + properties: { + status: { type: 'string', readOnly: true }, + metadata: { + $ref: '#/components/schemas/WritableMetadata', + }, + }, + }, + WritableMetadata: { + type: 'object', + properties: { + createdAt: { type: 'string', readOnly: true }, + displayName: { type: 'string' }, + }, + }, + }, + }, + }, + errors: [ + { + code: 'xgen-IPA-113-singleton-should-have-update-method', + message: + 'Singleton resources should define the Update method. If this is not a singleton resource, please implement all CRUDL methods.', + path: ['paths', '/resource/{exampleId}/writableSingleton'], + severity: DiagnosticSeverity.Error, + }, + ], + }, + ...['allOf', 'anyOf', 'oneOf'].flatMap((composition) => + [ + { description: 'read-only branches', withProperties: false, writable: false }, + { description: 'a writable branch', withProperties: false, writable: true }, + { description: 'read-only branches alongside properties', withProperties: true, writable: false }, + { description: 'a writable branch alongside read-only properties', withProperties: true, writable: true }, + ].map(({ description, withProperties, writable }) => ({ + name: `singleton with referenced nested ${composition} containing ${description}`, + document: { + paths: { + '/resource/{exampleId}/singleton': { + get: { + responses: { + 200: { + content: { + 'application/json': { + schema: { $ref: '#/components/schemas/Singleton' }, + }, + }, + }, + }, + }, + }, + }, + components: { + schemas: { + Singleton: { + type: 'object', + properties: { + metadata: { $ref: '#/components/schemas/Metadata' }, + }, + }, + Metadata: { + ...(withProperties ? { properties: { id: { type: 'string', readOnly: true } } } : {}), + [composition]: [ + { $ref: '#/components/schemas/ReadOnlyMetadataPart' }, + { $ref: '#/components/schemas/OtherMetadataPart' }, + ], + }, + ReadOnlyMetadataPart: { + type: 'object', + properties: { createdAt: { type: 'string', readOnly: true } }, + }, + OtherMetadataPart: { + type: 'object', + properties: { displayName: { type: 'string', readOnly: !writable } }, + }, + }, + }, + }, + errors: writable + ? [ + { + code: 'xgen-IPA-113-singleton-should-have-update-method', + message: + 'Singleton resources should define the Update method. If this is not a singleton resource, please implement all CRUDL methods.', + path: ['paths', '/resource/{exampleId}/singleton'], + severity: DiagnosticSeverity.Error, + }, + ] + : [], + })) + ), + { + name: 'list response shape does not prove singleton is read-only', document: { paths: { '/resource/{exampleId}/readOnlyListSingleton': { @@ -109,6 +266,7 @@ testRule('xgen-IPA-113-singleton-should-have-update-method', [ properties: { results: { type: 'array', + readOnly: true, items: { type: 'object', properties: { @@ -118,8 +276,9 @@ testRule('xgen-IPA-113-singleton-should-have-update-method', [ }, }, }, - totalCount: { type: 'integer' }, + totalCount: { type: 'integer', readOnly: true }, }, + required: ['results'], }, }, }, @@ -129,7 +288,15 @@ testRule('xgen-IPA-113-singleton-should-have-update-method', [ }, }, }, - errors: [], + errors: [ + { + code: 'xgen-IPA-113-singleton-should-have-update-method', + message: + 'Singleton resources should define the Update method. If this is not a singleton resource, please implement all CRUDL methods.', + path: ['paths', '/resource/{exampleId}/readOnlyListSingleton'], + severity: DiagnosticSeverity.Error, + }, + ], }, { name: 'writable singleton with List response', diff --git a/tools/spectral/ipa/__tests__/IPA132OperationMustBeAReadOnlyResource.test.js b/tools/spectral/ipa/__tests__/IPA132OperationMustBeAReadOnlyResource.test.js index ce8bfbeaf1..2ef2278b94 100644 --- a/tools/spectral/ipa/__tests__/IPA132OperationMustBeAReadOnlyResource.test.js +++ b/tools/spectral/ipa/__tests__/IPA132OperationMustBeAReadOnlyResource.test.js @@ -2,7 +2,7 @@ import testRule from './__helpers__/testRule'; import { DiagnosticSeverity } from '@stoplight/types'; const READ_ONLY_SCHEMA_ERROR_MESSAGE = - 'The Operation resource must be read-only. All properties of the GET response schema must be marked as readOnly: true.'; + 'The Operation resource must be read-only. All properties of the GET response schema must be marked as readOnly: true or contain only read-only properties.'; const readOnlyGet = { responses: { diff --git a/tools/spectral/ipa/__tests__/ReadOnlySchemaTraversal.test.js b/tools/spectral/ipa/__tests__/ReadOnlySchemaTraversal.test.js new file mode 100644 index 0000000000..470fb8656a --- /dev/null +++ b/tools/spectral/ipa/__tests__/ReadOnlySchemaTraversal.test.js @@ -0,0 +1,95 @@ +import { DiagnosticSeverity } from '@stoplight/types'; +import testRule from './__helpers__/testRule'; +import { documentForResponseSchema, listResponseCases, readOnlySchemaCases } from './__helpers__/readOnlySchemaCases'; + +const singletonPath = '/resource/{exampleId}/singleton'; +const resourceCases = [ + ...readOnlySchemaCases, + ...listResponseCases, + ...['allOf', 'anyOf', 'oneOf'].map((composition) => ({ + description: `a referenced list response wrapped in ${composition}`, + schema: { [composition]: [{ $ref: '#/components/schemas/ListResponse' }] }, + expected: false, + })), +]; + +const singletonRule = 'xgen-IPA-113-singleton-should-have-update-method'; +testRule( + singletonRule, + resourceCases.map(({ description, schema, expected }) => ({ + name: `singleton with ${description}`, + document: documentForResponseSchema(schema, singletonPath), + errors: expected + ? [] + : [ + { + code: singletonRule, + message: 'Singleton resources should define the Update method.', + path: ['paths', singletonPath], + severity: DiagnosticSeverity.Error, + }, + ], + })) +); + +const updateRule = 'xgen-IPA-107-readonly-resource-should-not-have-update-method'; +testRule( + updateRule, + resourceCases.map(({ description, schema, expected }) => ({ + name: `PATCH on a singleton with ${description}`, + document: documentForResponseSchema(schema, singletonPath, { patch: {} }), + errors: expected + ? [ + { + code: updateRule, + message: 'Read-only resources must not define the Update method.', + path: ['paths', singletonPath, 'patch'], + severity: DiagnosticSeverity.Error, + }, + ] + : [], + })) +); + +const resetRule = 'xgen-IPA-113-reset-method-not-on-readonly-singleton'; +testRule( + resetRule, + resourceCases.map(({ description, schema, expected }) => { + const document = documentForResponseSchema(schema, singletonPath); + document.paths[`${singletonPath}:reset`] = { post: {} }; + return { + name: `reset on a singleton with ${description}`, + document, + errors: expected + ? [ + { + code: resetRule, + message: 'Read-only singleton resources must not define a :reset custom method.', + path: ['paths', `${singletonPath}:reset`], + severity: DiagnosticSeverity.Error, + }, + ] + : [], + }; + }) +); + +const operationRule = 'xgen-IPA-132-operation-must-be-a-read-only-resource'; +const operationPath = '/resource/{exampleId}/operations/{operationId}'; +testRule( + operationRule, + readOnlySchemaCases.map(({ description, schema, expected }) => ({ + name: `Operation response with ${description}`, + document: documentForResponseSchema(schema, operationPath), + errors: expected + ? [] + : [ + { + code: operationRule, + message: 'The Operation resource must be read-only.', + path: ['paths', operationPath, 'get'], + severity: DiagnosticSeverity.Warning, + }, + ], + })) +); diff --git a/tools/spectral/ipa/__tests__/__helpers__/readOnlySchemaCases.js b/tools/spectral/ipa/__tests__/__helpers__/readOnlySchemaCases.js new file mode 100644 index 0000000000..2e45c5726c --- /dev/null +++ b/tools/spectral/ipa/__tests__/__helpers__/readOnlySchemaCases.js @@ -0,0 +1,290 @@ +const readOnlyObject = { + type: 'object', + properties: { id: { type: 'string', readOnly: true } }, +}; + +export const readOnlySchemaCases = [ + { + description: 'an empty schema without a read-only annotation', + schema: {}, + expected: false, + }, + { + description: 'an open empty object', + schema: { type: 'object', properties: {} }, + expected: false, + }, + ...[undefined, {}].map((properties) => ({ + description: `a closed empty object with ${properties ? 'empty' : 'omitted'} properties`, + schema: { type: 'object', ...(properties ? { properties } : {}), additionalProperties: false }, + expected: true, + })), + { + description: 'an empty response object with a root read-only annotation', + schema: { type: 'object', readOnly: true }, + expected: false, + }, + ...[true, {}, { type: 'string' }].flatMap((additionalProperties) => [ + { + description: `read-only named fields with writable additionalProperties ${JSON.stringify(additionalProperties)}`, + schema: { ...readOnlyObject, additionalProperties }, + expected: false, + }, + { + description: `a response object with a root read-only annotation and additionalProperties ${JSON.stringify(additionalProperties)}`, + schema: { ...readOnlyObject, readOnly: true, additionalProperties }, + expected: false, + }, + ]), + { + description: 'a response object with a root read-only annotation and writable fields', + schema: { + type: 'object', + readOnly: true, + properties: { name: { type: 'string' } }, + additionalProperties: { type: 'string' }, + }, + expected: false, + }, + { + description: 'a response object with a root read-only annotation and a writable oneOf branch', + schema: { + type: 'object', + readOnly: true, + oneOf: [{ type: 'object', properties: { name: { type: 'string' } } }], + }, + expected: false, + }, + { + description: 'a response object with a root read-only annotation and only read-only fields', + schema: { ...readOnlyObject, readOnly: true }, + expected: true, + }, + { + description: 'an explicitly read-only object-valued property with writable named and additional fields', + schema: { + type: 'object', + properties: { + metadata: { + type: 'object', + readOnly: true, + properties: { name: { type: 'string' } }, + additionalProperties: { type: 'string' }, + }, + }, + }, + expected: true, + }, + { + description: 'an explicitly read-only empty object-valued property', + schema: { type: 'object', properties: { metadata: { type: 'object', readOnly: true } } }, + expected: true, + }, + { + description: 'a nested object with read-only named fields and writable dictionary entries', + schema: { + type: 'object', + properties: { + metadata: { ...readOnlyObject, additionalProperties: { type: 'string' } }, + }, + }, + expected: false, + }, + { + description: 'a dictionary of explicitly read-only values', + schema: { type: 'object', additionalProperties: { type: 'string', readOnly: true } }, + expected: true, + }, + { + description: 'a dictionary of objects containing only read-only fields', + schema: { type: 'object', additionalProperties: readOnlyObject }, + expected: true, + }, + { + description: 'a dictionary of objects containing writable fields', + schema: { + type: 'object', + additionalProperties: { + type: 'object', + properties: { id: { type: 'string', readOnly: true }, name: { type: 'string' } }, + }, + }, + expected: false, + }, + { + description: 'a dictionary of open empty objects', + schema: { type: 'object', additionalProperties: { type: 'object' } }, + expected: false, + }, + { + description: 'read-only named fields alongside read-only dictionary entries', + schema: { ...readOnlyObject, additionalProperties: readOnlyObject }, + expected: true, + }, + { + description: 'writable named fields alongside read-only dictionary entries', + schema: { + type: 'object', + properties: { name: { type: 'string' } }, + additionalProperties: readOnlyObject, + }, + expected: false, + }, + { + description: 'an array without an items schema', + schema: { type: 'array' }, + expected: false, + }, + { + description: 'an array with only object-specific constraints', + schema: { type: 'array', properties: readOnlyObject.properties, additionalProperties: false }, + expected: false, + }, + { + description: 'an object with only an array-specific constraint', + schema: { type: 'object', items: readOnlyObject }, + expected: false, + }, + { + description: 'an enum without field definitions', + schema: { enum: ['ONE', 'TWO'] }, + expected: false, + }, + ...['allOf', 'anyOf', 'oneOf'].flatMap((composition) => [ + { + description: `read-only fields with a required-only ${composition} branch`, + schema: { ...readOnlyObject, [composition]: [{ required: ['id'] }] }, + expected: true, + }, + { + description: `read-only fields with an object-type-only ${composition} branch`, + schema: { ...readOnlyObject, [composition]: [{ type: 'object' }] }, + expected: true, + }, + { + description: `read-only array items with an array-type-only ${composition} branch`, + schema: { type: 'array', items: readOnlyObject, [composition]: [{ type: 'array' }] }, + expected: true, + }, + { + description: `writable fields with a required-only ${composition} branch`, + schema: { type: 'object', properties: { id: { type: 'string' } }, [composition]: [{ required: ['id'] }] }, + expected: false, + }, + { + description: `a ${composition} containing only constraints`, + schema: { [composition]: [{ type: 'object', required: ['id'] }] }, + expected: false, + }, + { + description: `a ${composition} containing read-only fields and a constraint-only branch`, + schema: { [composition]: [readOnlyObject, { required: ['id'] }] }, + expected: composition === 'allOf', + }, + { + description: `a ${composition} containing read-only fields and an unconstrained branch`, + schema: { [composition]: [readOnlyObject, {}] }, + expected: composition === 'allOf', + }, + { + description: `a ${composition} containing array items and an array-type-only branch`, + schema: { [composition]: [{ items: readOnlyObject }, { type: 'array' }] }, + expected: composition === 'allOf', + }, + { + description: `a ${composition} containing read-only fields and a writable primitive`, + schema: { [composition]: [readOnlyObject, { type: 'string' }] }, + expected: false, + }, + { + description: `empty properties alongside read-only ${composition} branches`, + schema: { properties: {}, [composition]: [readOnlyObject] }, + expected: true, + }, + ]), +]; + +export const listResponseSchema = { + type: 'object', + properties: { + results: { + type: 'array', + readOnly: true, + items: { type: 'object', properties: { name: { type: 'string' } } }, + }, + totalCount: { type: 'integer', readOnly: true }, + }, +}; + +export const listResponseCases = [ + { description: 'an inline list response', schema: listResponseSchema, expected: false }, + ...['allOf', 'anyOf', 'oneOf'].map((composition) => ({ + description: `a list response wrapped in ${composition}`, + schema: { [composition]: [listResponseSchema] }, + expected: false, + })), + { + description: 'list fields split across allOf branches', + schema: { + allOf: [ + { type: 'object', properties: { results: listResponseSchema.properties.results } }, + { type: 'object', properties: { totalCount: listResponseSchema.properties.totalCount } }, + ], + }, + expected: false, + }, + { + description: 'list results and a required constraint split across allOf branches', + schema: { + allOf: [ + { type: 'object', properties: { results: listResponseSchema.properties.results } }, + { required: ['results'] }, + ], + }, + expected: false, + }, + ...['anyOf', 'oneOf'].flatMap((composition) => [ + { + description: `list metadata shared with a ${composition} results branch`, + schema: { + properties: { totalCount: listResponseSchema.properties.totalCount }, + [composition]: [{ properties: { results: listResponseSchema.properties.results } }, readOnlyObject], + }, + expected: false, + }, + { + description: `non-list fields in separate ${composition} alternatives`, + schema: { + [composition]: [ + { properties: { results: listResponseSchema.properties.results } }, + { properties: { totalCount: listResponseSchema.properties.totalCount } }, + ], + }, + expected: true, + }, + ]), + { + description: 'a resource containing a nested list rather than being a list itself', + schema: { type: 'object', properties: { history: listResponseSchema } }, + expected: true, + }, +]; + +export function documentForResponseSchema(schema, resourcePath, methods = {}) { + return { + paths: { + [resourcePath]: { + ...methods, + get: { + responses: { + 200: { + description: 'OK', + content: { 'application/json': { schema: { $ref: '#/components/schemas/Response' } } }, + }, + }, + }, + }, + }, + components: { schemas: { Response: schema, ListResponse: listResponseSchema } }, + }; +} diff --git a/tools/spectral/ipa/__tests__/utils/resourceEvaluation.test.js b/tools/spectral/ipa/__tests__/utils/resourceEvaluation.test.js index f92a809fb7..75360385b9 100644 --- a/tools/spectral/ipa/__tests__/utils/resourceEvaluation.test.js +++ b/tools/spectral/ipa/__tests__/utils/resourceEvaluation.test.js @@ -9,6 +9,7 @@ import { isSingletonResource, resourceBelongsToSingleParent, } from '../../rulesets/functions/utils/resourceEvaluation'; +import { listResponseCases, listResponseSchema, readOnlySchemaCases } from '../__helpers__/readOnlySchemaCases'; const resource = { '/resource': { @@ -396,6 +397,7 @@ describe('tools/spectral/ipa/rulesets/functions/utils/resourceEvaluation.js', () describe('allPropertiesAreReadOnly', () => { const testCases = [ + ...readOnlySchemaCases, { description: 'schema with all properties readOnly', schema: { @@ -445,6 +447,82 @@ describe('tools/spectral/ipa/rulesets/functions/utils/resourceEvaluation.js', () }, expected: true, }, + { + description: 'schema with unmarked nested object containing only readOnly properties', + schema: { + type: 'object', + properties: { + metadata: { + type: 'object', + properties: { + createdBy: { type: 'string', readOnly: true }, + updatedBy: { type: 'string', readOnly: true }, + }, + }, + }, + }, + expected: true, + }, + { + description: 'schema with nested object containing a writable property', + schema: { + type: 'object', + properties: { + metadata: { + type: 'object', + properties: { + createdBy: { type: 'string', readOnly: true }, + displayName: { type: 'string' }, + }, + }, + }, + }, + expected: false, + }, + { + description: 'schema with nested array containing only readOnly properties', + schema: { + type: 'object', + properties: { + entries: { + type: 'array', + items: { + type: 'object', + properties: { + id: { type: 'string', readOnly: true }, + status: { type: 'string', readOnly: true }, + }, + }, + }, + }, + }, + expected: true, + }, + { + description: 'schema with nested allOf containing only readOnly properties', + schema: { + type: 'object', + properties: { + metadata: { + allOf: [ + { + type: 'object', + properties: { + createdBy: { type: 'string', readOnly: true }, + }, + }, + { + type: 'object', + properties: { + updatedBy: { type: 'string', readOnly: true }, + }, + }, + ], + }, + }, + }, + expected: true, + }, { description: 'schema with array items all readOnly', schema: { @@ -490,28 +568,27 @@ describe('tools/spectral/ipa/rulesets/functions/utils/resourceEvaluation.js', () expected: false, }, { - description: 'singleton List response with all readOnly items', + description: 'schema with readOnly array property containing writable item schema', schema: { type: 'object', properties: { - results: { + auditEvents: { type: 'array', + readOnly: true, items: { type: 'object', properties: { id: { type: 'string', readOnly: true }, - name: { type: 'string', readOnly: true }, - status: { type: 'string', readOnly: true }, + message: { type: 'string' }, }, }, }, - totalCount: { type: 'integer' }, }, }, expected: true, }, { - description: 'singleton List response with some non-readOnly items', + description: 'list response shape with writable metadata is not treated specially', schema: { type: 'object', properties: { @@ -538,9 +615,149 @@ describe('tools/spectral/ipa/rulesets/functions/utils/resourceEvaluation.js', () expect(allPropertiesAreReadOnly(testCase.schema)).toEqual(testCase.expected); }); }); + + describe.each(['allOf', 'anyOf', 'oneOf'])('%s compositions', (composition) => { + const readOnlySchema = { + type: 'object', + properties: { id: { type: 'string', readOnly: true } }, + }; + const writableSchema = { + type: 'object', + properties: { displayName: { type: 'string' } }, + }; + + it.each([ + { + description: 'all branches are read-only, including a shared schema', + schema: { [composition]: [readOnlySchema, readOnlySchema] }, + expected: true, + }, + { + description: 'a writable branch follows a read-only branch', + schema: { [composition]: [readOnlySchema, writableSchema] }, + expected: false, + }, + { + description: 'a writable branch precedes a read-only branch', + schema: { [composition]: [writableSchema, readOnlySchema] }, + expected: false, + }, + { + description: 'read-only properties coexist with read-only branches', + schema: { ...readOnlySchema, [composition]: [readOnlySchema] }, + expected: true, + }, + { + description: 'read-only properties coexist with a writable branch', + schema: { ...readOnlySchema, [composition]: [writableSchema] }, + expected: false, + }, + { + description: 'writable properties coexist with a read-only branch', + schema: { ...writableSchema, [composition]: [readOnlySchema] }, + expected: false, + }, + { + description: 'read-only array items coexist with a writable branch', + schema: { type: 'array', items: readOnlySchema, [composition]: [{ type: 'array', items: writableSchema }] }, + expected: false, + }, + { + description: 'the composition is empty', + schema: { [composition]: [] }, + expected: false, + }, + ])('returns $expected when $description', ({ schema, expected }) => { + expect(allPropertiesAreReadOnly({ type: 'object', properties: { metadata: schema } })).toEqual(expected); + }); + + it('returns false for a circular branch after a read-only branch', () => { + const schema = { [composition]: [readOnlySchema] }; + schema[composition].push(schema); + + expect(allPropertiesAreReadOnly(schema)).toEqual(false); + }); + }); + + it('checks sibling composition keywords even when allOf is read-only', () => { + const schema = { + allOf: [{ type: 'object', properties: { id: { type: 'string', readOnly: true } } }], + anyOf: [{ type: 'object', properties: { displayName: { type: 'string' } } }], + }; + + expect(allPropertiesAreReadOnly(schema)).toEqual(false); + }); + + it('returns false for an unmarked circular schema', () => { + const schema = { + type: 'object', + properties: { + id: { type: 'string', readOnly: true }, + }, + }; + schema.properties.parent = schema; + + expect(allPropertiesAreReadOnly(schema)).toEqual(false); + }); + + it('returns false for a circular dictionary value', () => { + const schema = { type: 'object' }; + schema.additionalProperties = schema; + + expect(allPropertiesAreReadOnly(schema)).toEqual(false); + }); + + it('honors an explicit read-only annotation on a circular dictionary', () => { + const schema = { type: 'object', readOnly: true }; + schema.additionalProperties = schema; + + expect(allPropertiesAreReadOnly({ properties: { metadata: schema } })).toEqual(true); + }); + + it('checks a shared schema independently in named and additional properties', () => { + const value = { type: 'object', properties: { id: { type: 'string', readOnly: true } } }; + const schema = { type: 'object', properties: { metadata: value }, additionalProperties: value }; + + expect(allPropertiesAreReadOnly(schema)).toEqual(true); + }); + + it('does not treat an unresolved reference as a neutral constraint', () => { + const schema = { + properties: { id: { type: 'string', readOnly: true } }, + allOf: [{ $ref: '#/components/schemas/Unknown' }], + }; + + expect(allPropertiesAreReadOnly(schema)).toEqual(false); + }); }); describe('isReadOnlyResource', () => { + function resourceForSchema(schema) { + return { + '/resource/{id}/singleton': { + get: { responses: { 200: { content: { 'application/json': { schema } } } } }, + }, + }; + } + + it.each(listResponseCases)('returns $expected for $description', ({ schema, expected }) => { + expect(isReadOnlyResource(resourceForSchema(schema))).toEqual(expected); + }); + + it('handles a cycle while detecting a list response in another branch', () => { + const schema = { readOnly: true, allOf: [] }; + schema.allOf.push(schema, listResponseSchema); + + expect(isReadOnlyResource(resourceForSchema(schema))).toEqual(false); + }); + + it('handles a read-only circular schema without list fields', () => { + const schema = { readOnly: true, allOf: [] }; + schema.allOf.push(schema); + + expect(isReadOnlyResource(resourceForSchema({ properties: { metadata: schema } }))).toEqual(true); + }); + const testCases = [ { description: 'read-only resource', @@ -567,6 +784,33 @@ describe('tools/spectral/ipa/rulesets/functions/utils/resourceEvaluation.js', () resourcePathItems: readOnlySingleton, expected: true, }, + { + description: 'singleton with list response shape', + resourcePathItems: { + '/resource/{id}/listSingleton': { + get: { + responses: { + 200: { + content: { + 'application/json': { + schema: { + type: 'object', + properties: { + links: { type: 'array', readOnly: true, items: { type: 'object' } }, + results: { type: 'array', readOnly: true, items: { type: 'object' } }, + totalCount: { type: 'integer', readOnly: true }, + }, + required: ['results'], + }, + }, + }, + }, + }, + }, + }, + }, + expected: false, + }, { description: 'resource with xgen-IPA-104-resource-has-GET exception', resourcePathItems: { diff --git a/tools/spectral/ipa/rulesets/IPA-106.yaml b/tools/spectral/ipa/rulesets/IPA-106.yaml index f3defb6fc2..aa8ca9581a 100644 --- a/tools/spectral/ipa/rulesets/IPA-106.yaml +++ b/tools/spectral/ipa/rulesets/IPA-106.yaml @@ -122,7 +122,7 @@ rules: ##### Implementation details Rule checks for the following conditions: - Applies to POST methods on resource collection paths - - Checks if the resource is a read-only resource (all properties in GET response have readOnly:true) + - Checks if the resource is a read-only resource (all properties in the GET response are marked as readOnly or contain only read-only properties) - If a resource does not have a standard GET method, it is not considered read-only (cannot determine the resource schema) - Fails if a Create method is defined on a read-only resource - Operation objects with `x-xgen-IPA-exception` for this rule are excluded from validation diff --git a/tools/spectral/ipa/rulesets/IPA-107.yaml b/tools/spectral/ipa/rulesets/IPA-107.yaml index 23f39556b2..104cad848a 100644 --- a/tools/spectral/ipa/rulesets/IPA-107.yaml +++ b/tools/spectral/ipa/rulesets/IPA-107.yaml @@ -122,7 +122,7 @@ rules: ##### Implementation details Rule checks for the following conditions: - Applies to PUT/PATCH methods on all resource paths - - Checks if the resource is a read-only resource (all properties in GET response have readOnly:true) + - Checks if the resource is a read-only resource (all properties in the GET response are marked as readOnly or contain only read-only properties) - If a resource does not have a standard GET method, it is not considered read-only (cannot determine the resource schema) - Fails if an Update method is defined on a read-only resource - Operation objects with `x-xgen-IPA-exception` for this rule are excluded from validation diff --git a/tools/spectral/ipa/rulesets/IPA-108.yaml b/tools/spectral/ipa/rulesets/IPA-108.yaml index 66c677e6e6..3d23c00a1e 100644 --- a/tools/spectral/ipa/rulesets/IPA-108.yaml +++ b/tools/spectral/ipa/rulesets/IPA-108.yaml @@ -62,7 +62,7 @@ rules: ##### Implementation details Rule checks for the following conditions: - Applies to DELETE methods on single resource paths and singleton resources - - Checks if the resource is a read-only resource (all properties in GET response have readOnly:true) + - Checks if the resource is a read-only resource (all properties in the GET response are marked as readOnly or contain only read-only properties) - If a resource does not have a standard GET method, it is not considered read-only (cannot determine the resource schema) - Fails if a Delete method is defined on a read-only resource - Operation objects with `x-xgen-IPA-exception` for this rule are excluded from validation diff --git a/tools/spectral/ipa/rulesets/IPA-113.yaml b/tools/spectral/ipa/rulesets/IPA-113.yaml index eaa966aaa7..cdfdc3d5b8 100644 --- a/tools/spectral/ipa/rulesets/IPA-113.yaml +++ b/tools/spectral/ipa/rulesets/IPA-113.yaml @@ -50,7 +50,13 @@ rules: ##### Implementation details Rule checks for the following conditions: - Applies only to singleton resources - - Excludes read-only singleton resources (where all properties in the GET response schema are marked as readOnly; for List responses, all properties in the items schema must be readOnly) + - Excludes read-only singleton resources (where the GET response schema contains only read-only fields) + - An explicit `readOnly: true` annotation on an object-valued property covers that property, including its additional properties; an annotation on the response schema itself does not establish a read-only resource + - Otherwise, checks declared properties, array items, and explicit additionalProperties schemas recursively; writable dictionary entries prevent read-only inference + - Omitted additionalProperties does not override read-only declared fields, but an empty or constraint-only schema alone does not prove read-only status + - An explicitly closed empty object (`additionalProperties: false`) has no writable fields; use of that keyword is validated separately by IPA-118 + - Checks all composition branches; constraint-only branches may accompany read-only field definitions but cannot establish read-only status on their own + - List response envelopes, including those defined through composition, do not establish that a singleton is read-only - Checks that the resource has the PUT and/or PATCH methods defined message: '{{error}} https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-113-singleton-should-have-update-method' severity: error @@ -142,7 +148,8 @@ rules: - Applies only to paths ending with :reset - Verifies that the parent singleton resource is not read-only - Uses existing isReadOnlyResource() helper function - - Fails if the singleton resource has all properties marked as readOnly: true + - Fails if all properties of the singleton resource are marked as readOnly or contain only read-only properties + - List response envelopes, including those defined through composition, do not establish that a singleton is read-only message: '{{error}} https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-113-reset-method-not-on-readonly-singleton' severity: error given: '$.paths[*]' diff --git a/tools/spectral/ipa/rulesets/IPA-132.yaml b/tools/spectral/ipa/rulesets/IPA-132.yaml index 9261e1f93e..c7deaf3e90 100644 --- a/tools/spectral/ipa/rulesets/IPA-132.yaml +++ b/tools/spectral/ipa/rulesets/IPA-132.yaml @@ -52,7 +52,7 @@ rules: xgen-IPA-132-operation-must-be-a-read-only-resource: description: | Operations endpoints are read-only. They may only define the get method, and all properties - of the Operation resource must be readOnly. + of the Operation resource must be read-only. ##### Implementation details Rule checks for the following conditions: @@ -62,7 +62,7 @@ rules: - The path item must not define any HTTP method other than `get` - On the single Operation endpoint (`.../operations/{operationId}`), where the Get method is defined, all properties of every 2xx response schema of the `get` method must be - marked as `readOnly: true` + marked as `readOnly: true` or contain only read-only properties - Paths with `x-xgen-IPA-exception` for this rule are excluded from validation message: '{{error}} https://mdb.link/mongodb-atlas-openapi-validation#xgen-IPA-132-operation-must-be-a-read-only-resource' diff --git a/tools/spectral/ipa/rulesets/README.md b/tools/spectral/ipa/rulesets/README.md index 48aeb59ba0..17b585323d 100644 --- a/tools/spectral/ipa/rulesets/README.md +++ b/tools/spectral/ipa/rulesets/README.md @@ -365,7 +365,7 @@ Read-only resources must not define the Create method. ##### Implementation details Rule checks for the following conditions: - Applies to POST methods on resource collection paths - - Checks if the resource is a read-only resource (all properties in GET response have readOnly:true) + - Checks if the resource is a read-only resource (all properties in the GET response are marked as readOnly or contain only read-only properties) - If a resource does not have a standard GET method, it is not considered read-only (cannot determine the resource schema) - Fails if a Create method is defined on a read-only resource - Operation objects with `x-xgen-IPA-exception` for this rule are excluded from validation @@ -484,7 +484,7 @@ Read-only resources must not define the Update method. ##### Implementation details Rule checks for the following conditions: - Applies to PUT/PATCH methods on all resource paths - - Checks if the resource is a read-only resource (all properties in GET response have readOnly:true) + - Checks if the resource is a read-only resource (all properties in the GET response are marked as readOnly or contain only read-only properties) - If a resource does not have a standard GET method, it is not considered read-only (cannot determine the resource schema) - Fails if an Update method is defined on a read-only resource - Operation objects with `x-xgen-IPA-exception` for this rule are excluded from validation @@ -577,7 +577,7 @@ Read-only resources must not define the Delete method. ##### Implementation details Rule checks for the following conditions: - Applies to DELETE methods on single resource paths and singleton resources - - Checks if the resource is a read-only resource (all properties in GET response have readOnly:true) + - Checks if the resource is a read-only resource (all properties in the GET response are marked as readOnly or contain only read-only properties) - If a resource does not have a standard GET method, it is not considered read-only (cannot determine the resource schema) - Fails if a Delete method is defined on a read-only resource - Operation objects with `x-xgen-IPA-exception` for this rule are excluded from validation @@ -891,7 +891,13 @@ Singleton resources should define the Update method. Validation for the presence ##### Implementation details Rule checks for the following conditions: - Applies only to singleton resources - - Excludes read-only singleton resources (where all properties in the GET response schema are marked as readOnly; for List responses, all properties in the items schema must be readOnly) + - Excludes read-only singleton resources (where the GET response schema contains only read-only fields) + - An explicit `readOnly: true` annotation on an object-valued property covers that property, including its additional properties; an annotation on the response schema itself does not establish a read-only resource + - Otherwise, checks declared properties, array items, and explicit additionalProperties schemas recursively; writable dictionary entries prevent read-only inference + - Omitted additionalProperties does not override read-only declared fields, but an empty or constraint-only schema alone does not prove read-only status + - An explicitly closed empty object (`additionalProperties: false`) has no writable fields; use of that keyword is validated separately by IPA-118 + - Checks all composition branches; constraint-only branches may accompany read-only field definitions but cannot establish read-only status on their own + - List response envelopes, including those defined through composition, do not establish that a singleton is read-only - Checks that the resource has the PUT and/or PATCH methods defined #### xgen-IPA-113-reset-method-must-use-POST @@ -964,7 +970,8 @@ Rule checks for the following conditions: - Applies only to paths ending with :reset - Verifies that the parent singleton resource is not read-only - Uses existing isReadOnlyResource() helper function - - Fails if the singleton resource has all properties marked as readOnly: true + - Fails if all properties of the singleton resource are marked as readOnly or contain only read-only properties + - List response envelopes, including those defined through composition, do not establish that a singleton is read-only #### xgen-IPA-113-reset-method-valid-operation-id @@ -1469,7 +1476,7 @@ Rule checks for the following conditions: ![warn](https://img.shields.io/badge/warning-yellow) Operations endpoints are read-only. They may only define the get method, and all properties -of the Operation resource must be readOnly. +of the Operation resource must be read-only. ##### Implementation details Rule checks for the following conditions: @@ -1479,7 +1486,7 @@ Rule checks for the following conditions: - The path item must not define any HTTP method other than `get` - On the single Operation endpoint (`.../operations/{operationId}`), where the Get method is defined, all properties of every 2xx response schema of the `get` method must be - marked as `readOnly: true` + marked as `readOnly: true` or contain only read-only properties - Paths with `x-xgen-IPA-exception` for this rule are excluded from validation #### xgen-IPA-132-operations-endpoint-must-be-a-leaf-resource diff --git a/tools/spectral/ipa/rulesets/functions/IPA132OperationMustBeAReadOnlyResource.js b/tools/spectral/ipa/rulesets/functions/IPA132OperationMustBeAReadOnlyResource.js index 5a7d789a30..fa64d8d093 100644 --- a/tools/spectral/ipa/rulesets/functions/IPA132OperationMustBeAReadOnlyResource.js +++ b/tools/spectral/ipa/rulesets/functions/IPA132OperationMustBeAReadOnlyResource.js @@ -5,7 +5,7 @@ import { isOperationsPath, isSingleOperationPath } from './utils/longRunningOper const VALID_METHOD = 'get'; const HTTP_METHODS = ['get', 'put', 'post', 'delete', 'options', 'head', 'patch', 'trace']; const READ_ONLY_SCHEMA_ERROR_MESSAGE = - 'The Operation resource must be read-only. All properties of the GET response schema must be marked as readOnly: true.'; + 'The Operation resource must be read-only. All properties of the GET response schema must be marked as readOnly: true or contain only read-only properties.'; /** * Checks that an Operations endpoint defined by IPA-132 is a read-only resource: its path items diff --git a/tools/spectral/ipa/rulesets/functions/utils/resourceEvaluation.js b/tools/spectral/ipa/rulesets/functions/utils/resourceEvaluation.js index 607c76bf91..38907044e9 100644 --- a/tools/spectral/ipa/rulesets/functions/utils/resourceEvaluation.js +++ b/tools/spectral/ipa/rulesets/functions/utils/resourceEvaluation.js @@ -242,51 +242,93 @@ export function removePrefix(path) { } /** - * Checks if all properties in a schema have readOnly: true. + * Checks if all properties in a schema are read-only. * * @param {Object} schema - The schema to check + * @param {Set} visiting - Schemas in the current traversal path * @returns {boolean} true if all properties are readOnly, false otherwise */ -export function allPropertiesAreReadOnly(schema) { +export function allPropertiesAreReadOnly(schema, visiting = new Set()) { + return getReadOnlyState(schema, visiting, false) === true; +} + +/** + * Returns true for read-only fields, false for writable/unknown fields, and undefined for + * constraints that do not define fields. A constraint-only schema cannot prove read-only + * status on its own, but can accompany field definitions in a composition. + */ +function getReadOnlyState(schema, visiting, isField) { if (!schema || typeof schema !== 'object') { return false; } - if (schema.properties) { - if (schema.properties.results && schema.properties.results.type === 'array' && schema.properties.results.items) { - return allPropertiesAreReadOnly(schema.properties.results.items); + // OpenAPI readOnly applies to a field, not to an entire response component. + if (isField && schema.readOnly === true) { + return true; + } + + if (visiting.has(schema)) { + return false; + } + visiting.add(schema); + + try { + if (schema.$ref || (schema.type && schema.type !== 'object' && schema.type !== 'array')) { + return false; } - for (const [, propSchema] of Object.entries(schema.properties)) { - if (propSchema.readOnly !== true) { + let hasReadOnlyFields = false; + if (schema.properties && schema.type !== 'array') { + const properties = Object.values(schema.properties); + if (!properties.every((property) => getReadOnlyState(property, visiting, true) === true)) { return false; } + hasReadOnlyFields = properties.length > 0; } - return Object.keys(schema.properties).length > 0; - } - if (schema.items) { - return allPropertiesAreReadOnly(schema.items); - } + if (schema.items && schema.type !== 'object') { + if (getReadOnlyState(schema.items, visiting, true) !== true) { + return false; + } + hasReadOnlyFields = true; + } - if (Array.isArray(schema.allOf)) { - return schema.allOf.every((subSchema) => allPropertiesAreReadOnly(subSchema)); - } + // Infer from declared fields, retaining the convention that omitted additionalProperties + // does not override read-only named fields. Explicit dictionaries must be checked. + if (schema.additionalProperties !== undefined && schema.type !== 'array') { + if ( + schema.additionalProperties !== false && + getReadOnlyState(schema.additionalProperties, visiting, true) !== true + ) { + return false; + } + hasReadOnlyFields = true; + } - if (Array.isArray(schema.anyOf)) { - return schema.anyOf.some((subSchema) => allPropertiesAreReadOnly(subSchema)); - } + for (const composition of ['allOf', 'anyOf', 'oneOf']) { + const branches = schema[composition]; + if (!Array.isArray(branches)) { + continue; + } + const states = branches.map((subSchema) => getReadOnlyState(subSchema, visiting, isField)); + if (states.length === 0 || states.includes(false)) { + return false; + } + // allOf shares field definitions between branches. Alternatives must each establish + // read-only fields unless a sibling definition already constrains the whole schema. + hasReadOnlyFields ||= composition === 'allOf' ? states.includes(true) : states.every((state) => state === true); + } - if (Array.isArray(schema.oneOf)) { - return schema.oneOf.some((subSchema) => allPropertiesAreReadOnly(subSchema)); + return hasReadOnlyFields ? true : undefined; + } finally { + visiting.delete(schema); } - - return false; } /** * Checks if a resource is a read-only resource. - * A read-only resource has all properties in its GET response schema marked as readOnly: true. + * A read-only resource has all properties in its GET response schema marked as readOnly: true + * or composed only of read-only properties. * * @param {Object} resourcePathItems - All path items for the resource to be evaluated * @returns {boolean} true if the resource is read-only, false otherwise @@ -351,6 +393,12 @@ export function isReadOnlyResource(resourcePathItems) { continue; } + // List responses are containers around resources, not resource schemas themselves. + // They cannot prove that a singleton or resource is read-only. + if (isListResponseSchema(mediaTypeObj.schema)) { + return false; + } + if (!allPropertiesAreReadOnly(mediaTypeObj.schema)) { return false; } @@ -358,3 +406,49 @@ export function isReadOnlyResource(resourcePathItems) { return true; } + +function isListResponseSchema(schema) { + return getResponseShapes(schema, new Set()).has(RESULTS_ARRAY | PAGINATION_METADATA); +} + +const RESULTS_ARRAY = 1; +const PAGINATION_METADATA = 2; + +// Track possible response shapes so allOf can combine fields split across branches without +// combining fields from mutually exclusive anyOf/oneOf alternatives. There are only four shapes. +function getResponseShapes(schema, visiting) { + if (!schema || typeof schema !== 'object' || visiting.has(schema)) { + return new Set([0]); + } + + const properties = schema?.properties; + const results = properties?.results; + const resultsShape = results?.type === 'array' && results.items ? RESULTS_ARRAY : 0; + const metadataShape = + properties?.links || properties?.totalCount || schema.required?.includes('results') ? PAGINATION_METADATA : 0; + let shapes = new Set([resultsShape | metadataShape]); + + visiting.add(schema); + try { + for (const composition of ['allOf', 'anyOf', 'oneOf']) { + if (!Array.isArray(schema[composition]) || schema[composition].length === 0) { + continue; + } + const branches = schema[composition].map((branch) => getResponseShapes(branch, visiting)); + if (composition === 'allOf') { + for (const branch of branches) { + shapes = combineResponseShapes(shapes, branch); + } + } else { + shapes = combineResponseShapes(shapes, new Set(branches.flatMap((branch) => [...branch]))); + } + } + return shapes; + } finally { + visiting.delete(schema); + } +} + +function combineResponseShapes(left, right) { + return new Set([...left].flatMap((a) => [...right].map((b) => a | b))); +}