diff --git a/.chronus/changes/arm-patch-body-schema-2026-05-05-02-30-00.md b/.chronus/changes/arm-patch-body-schema-2026-05-05-02-30-00.md new file mode 100644 index 0000000000..d3d6b13a55 --- /dev/null +++ b/.chronus/changes/arm-patch-body-schema-2026-05-05-02-30-00.md @@ -0,0 +1,14 @@ +--- +changeKind: feature +packages: + - "@azure-tools/typespec-azure-resource-manager" +--- + +Extend the `arm-resource-patch` linter rule to validate PATCH request body schemas. New diagnostics are emitted when: + +- A PATCH request body property is required and not read-only. +- A PATCH request body property has a default value (defaults are never applied to PATCH requests). +- A PATCH request body property is not updateable and not read-only. +- A PATCH operation specifies an explicit `content-type` other than `application/merge-patch+json` (or the implicit `application/json`). + +Codefixes are provided to remove default values, mark properties as optional, and remove non-updateable properties from PATCH bodies. diff --git a/packages/typespec-azure-resource-manager/src/rules/arm-resource-patch.ts b/packages/typespec-azure-resource-manager/src/rules/arm-resource-patch.ts index fbbcccc311..d64154a7c0 100644 --- a/packages/typespec-azure-resource-manager/src/rules/arm-resource-patch.ts +++ b/packages/typespec-azure-resource-manager/src/rules/arm-resource-patch.ts @@ -1,18 +1,29 @@ import { + CodeFix, DiagnosticMessages, + EnumMember, LinterRuleContext, Model, ModelProperty, Operation, Program, createRule, + defineCodeFix, getEffectiveModelType, + getLifecycleVisibilityEnum, getProperty, + getSourceLocation, + getTypeName, + getVisibilityForClass, + hasVisibility, isErrorType, isType, paramMessage, } from "@typespec/compiler"; +import { SyntaxKind } from "@typespec/compiler/ast"; import { + getContentTypes, + getHeaderFieldName, getOperationVerb, isBody, isBodyRoot, @@ -22,7 +33,7 @@ import { } from "@typespec/http"; import { getArmResource } from "../resource.js"; -import { getSourceModel, isInternalTypeSpec } from "./utils.js"; +import { getSourceModel, getSourceProperty, isInternalTypeSpec } from "./utils.js"; export const patchOperationsRule = createRule({ name: "arm-resource-patch", @@ -33,8 +44,19 @@ export const patchOperationsRule = createRule({ default: "The request body of a PATCH must be a model with a subset of resource properties", missingTags: "Resource PATCH must contain the 'tags' property.", modelSuperset: paramMessage`Resource PATCH models must be a subset of the resource type. The following properties: [${"name"}] do not exist in resource Model '${"resourceModel"}'.`, + notUpdateableInPatch: paramMessage`Property '${"propertyName"}' is in the PATCH request body but is not updateable on the resource. Make this property updateable on the resource or remove it from the PATCH request.`, + requiredInPatch: paramMessage`Property '${"propertyName"}' is required in the PATCH request body. PATCH request body properties must all be optional or readOnly.`, + defaultInPatch: paramMessage`Property '${"propertyName"}' has a default value in the PATCH request body. PATCH request body properties that are not present in the request body leave the value unchanged; they do not result in any default value being assigned.`, + nonMergePatchContentType: paramMessage`PATCH operation '${"operationName"}' specifies a content-type '${"contentType"}' other than 'application/merge-patch+json'.`, }, create(context) { + // Resolve the lifecycle visibility class members once per rule activation + // so we don't re-resolve them on every property check. + const lifecycle = getLifecycleVisibilityEnum(context.program); + const lifecycleMembers: LifecycleMembers = { + read: lifecycle.members.get("Read"), + update: lifecycle.members.get("Update"), + }; return { operation: (operation: Operation) => { if (!isInternalTypeSpec(context.program, operation)) { @@ -42,7 +64,7 @@ export const patchOperationsRule = createRule({ if (verb === "patch") { const resourceType = getResourceModel(context.program, operation); if (resourceType) { - checkPatchModel(context, operation, resourceType); + checkPatchModel(context, operation, resourceType, lifecycleMembers); } } } @@ -51,10 +73,16 @@ export const patchOperationsRule = createRule({ }, }); +interface LifecycleMembers { + read: EnumMember | undefined; + update: EnumMember | undefined; +} + function checkPatchModel( context: LinterRuleContext, operation: Operation, resourceType: Model, + lifecycleMembers: LifecycleMembers, ) { const patchModel = getPatchModel(context.program, operation); if (patchModel === undefined) { @@ -94,9 +122,315 @@ function checkPatchModel( }, target: operation, }); + + // Check each property in the PATCH body for additional issues. + checkPatchBodyProperties(context, patchModel, lifecycleMembers); + } + + // Check the request content-type header (if explicit). + checkPatchContentType(context, operation); +} + +function checkPatchBodyProperties( + context: LinterRuleContext, + patchModel: ModelProperty[], + lifecycleMembers: LifecycleMembers, +) { + const readOnlyCache = new Map(); + + for (const property of patchModel) { + if ( + !property.optional && + !isReadOnly(context.program, property, lifecycleMembers, readOnlyCache) + ) { + context.reportDiagnostic({ + messageId: "requiredInPatch", + format: { propertyName: property.name }, + target: property, + codefixes: [createMakeOptionalCodeFix(property)], + }); + } + + // Default values on PATCH body properties are not meaningful. + if (property.defaultValue !== undefined) { + context.reportDiagnostic({ + messageId: "defaultInPatch", + format: { propertyName: property.name }, + target: property, + codefixes: [createRemoveDefaultCodeFix(property)], + }); + } + + // Non-updateable properties should not appear in PATCH body. The check is + // applied recursively to nested model and Record property types. + // Recursion-related state is created fresh per top-level property so that + // a cycle-resolution false on one traversal does not leak as a stale + // cached "false" into a sibling property's traversal. + if ( + isNotUpdateable(context.program, property, { + lifecycleMembers, + readOnlyCache, + modelHasNonUpdateableCache: new Map(), + inProgressModels: new Set(), + }) + ) { + context.reportDiagnostic({ + messageId: "notUpdateableInPatch", + format: { propertyName: property.name }, + target: property, + codefixes: [createRemovePropertyCodeFix(property)], + }); + } } } +interface NotUpdateableState { + /** Resolved Lifecycle visibility members; computed once at rule activation. */ + lifecycleMembers: LifecycleMembers; + /** + * Pure per-source-property cache; safe to share across the entire traversal. + * Keyed by the source property's fully-qualified name (namespace + model + + * property) which is guaranteed to be unique in TypeSpec, since identity + * comparisons on `ModelProperty` objects are not reliable across recursion. + */ + readOnlyCache: Map; + /** + * Per-top-level-property cache of the recursive "does this model contain a + * non-updateable property" answer. Keyed by the model's fully-qualified + * name. Recreated for every top-level patch property because cycle-breaking + * inside one traversal can otherwise cache a too-conservative `false` for a + * model whose true answer depends on a different entry path. + */ + modelHasNonUpdateableCache: Map; + /** + * Models currently being visited in the active recursion stack, keyed by + * fully-qualified name. Used purely to break cycles — entries are added on + * entry to `modelHasNonUpdateable` and removed on exit, so each top-level + * call sees a clean stack. + */ + inProgressModels: Set; +} + +/** + * Returns true when the source resource property's lifecycle visibility is + * exactly `{Lifecycle.Read}` by itself. Such properties are filtered out by + * visibility transforms during PATCH request serialization, so they are + * allowed (even if required) in the PATCH body model. + */ +function isReadOnly( + program: Program, + property: ModelProperty, + lifecycleMembers: LifecycleMembers, + cache: Map, +): boolean { + const sourceProperty = getSourceProperty(property); + const cacheKey = getTypeName(sourceProperty); + const cached = cache.get(cacheKey); + if (cached !== undefined) return cached; + const readMember = lifecycleMembers.read; + if (readMember === undefined) { + cache.set(cacheKey, false); + return false; + } + const lifecycle = readMember.enum; + const visibility = getVisibilityForClass(program, sourceProperty, lifecycle); + const result = visibility.size === 1 && visibility.has(readMember); + cache.set(cacheKey, result); + return result; +} + +/** + * Returns true when the source resource property's lifecycle visibility makes + * it eligible to appear in a PATCH request body. A property is allowed if its + * visibility either: + * + * - includes `Lifecycle.Update` (this covers default visibility — which has + * all lifecycle modifiers — as well as `@visibility(Lifecycle.Update)` + * alone or combined with any other modifier), OR + * - is exactly `{Lifecycle.Read}` by itself (such properties are filtered out + * of the request body by visibility transforms during serialization). + * + * Other visibilities (for example `@visibility(Lifecycle.Create)` only or + * `@visibility(Lifecycle.Create, Lifecycle.Read)`) are not allowed. + */ +function isAllowedInPatchByVisibility( + program: Program, + property: ModelProperty, + lifecycleMembers: LifecycleMembers, + readOnlyCache: Map, +): boolean { + const updateMember = lifecycleMembers.update; + if (updateMember !== undefined) { + const sourceProperty = getSourceProperty(property); + if (hasVisibility(program, sourceProperty, updateMember)) { + return true; + } + } + return isReadOnly(program, property, lifecycleMembers, readOnlyCache); +} + +/** + * Returns true when the source resource property's visibility is not allowed + * in a PATCH body, OR when any of its transitively-nested complex keyed + * properties (model types and `Record` value types) is itself not + * updateable. + */ +function isNotUpdateable( + program: Program, + property: ModelProperty, + state: NotUpdateableState, +): boolean { + if ( + !isAllowedInPatchByVisibility(program, property, state.lifecycleMembers, state.readOnlyCache) + ) { + return true; + } + + // Recurse into complex keyed property types: bare model types (excluding + // arrays) and the value type of records when that value type is a model. + const nested = getNestedModelToCheck(property.type); + if (nested !== undefined && modelHasNonUpdateable(program, nested, state)) { + return true; + } + + return false; +} + +/** + * Returns true when `model` (or any of its transitively-nested complex keyed + * property types) has a property that is not updateable. Cycles are broken by + * treating an in-progress model as not contributing any new non-updateable + * findings on the back-edge — the property that started the cycle is still + * checked by its own enclosing call, so any non-updateable property is still + * detected from at least one path. + */ +function modelHasNonUpdateable(program: Program, model: Model, state: NotUpdateableState): boolean { + const cacheKey = getTypeName(model); + const cached = state.modelHasNonUpdateableCache.get(cacheKey); + if (cached !== undefined) return cached; + if (state.inProgressModels.has(cacheKey)) return false; + state.inProgressModels.add(cacheKey); + try { + for (const nestedProperty of model.properties.values()) { + if (isNotUpdateable(program, nestedProperty, state)) { + state.modelHasNonUpdateableCache.set(cacheKey, true); + return true; + } + } + state.modelHasNonUpdateableCache.set(cacheKey, false); + return false; + } finally { + state.inProgressModels.delete(cacheKey); + } +} + +/** + * Returns the nested `Model` to recurse into for the `notUpdateableInPatch` + * check, or `undefined` when the property's type is not a complex keyed type. + * + * - Plain model types are returned directly (excluding arrays which are + * `Model<"Array">` in TypeSpec). + * - For `Record` types, the value type `T` is returned when it is a model. + */ +function getNestedModelToCheck(type: ModelProperty["type"]): Model | undefined { + if (type.kind !== "Model") return undefined; + // `Array` is represented as a Model whose name is "Array"; skip it. + if (type.name === "Array") return undefined; + if (type.name === "Record") { + const valueType = type.indexer?.value; + if (valueType && valueType.kind === "Model" && valueType.name !== "Array") { + return valueType; + } + return undefined; + } + return type; +} + +function checkPatchContentType( + context: LinterRuleContext, + operation: Operation, +) { + const program = context.program; + for (const property of operation.parameters.properties.values()) { + if (!isHeader(program, property)) continue; + const headerName = getHeaderFieldName(program, property); + if (headerName?.toLowerCase() !== "content-type") continue; + const [contentTypes] = getContentTypes(property); + if (contentTypes.length === 0) continue; + const allowed = new Set(["application/merge-patch+json", "application/json"]); + const offending = contentTypes.find((ct) => !allowed.has(ct)); + if (offending !== undefined) { + context.reportDiagnostic({ + messageId: "nonMergePatchContentType", + format: { operationName: operation.name, contentType: offending }, + target: property, + }); + } + } +} + +function createMakeOptionalCodeFix(property: ModelProperty): CodeFix { + return defineCodeFix({ + id: "make-patch-property-optional", + label: `Make property '${property.name}' optional`, + fix: (context) => { + const node = property.node; + if (!node || node.kind !== SyntaxKind.ModelProperty) return undefined; + const idLocation = getSourceLocation(node.id); + return context.appendText(idLocation, "?"); + }, + }); +} + +function createRemoveDefaultCodeFix(property: ModelProperty): CodeFix { + return defineCodeFix({ + id: "remove-patch-property-default", + label: `Remove default value from property '${property.name}'`, + fix: (context) => { + const node = property.node; + if (!node || node.kind !== SyntaxKind.ModelProperty || node.default === undefined) + return undefined; + const valueLocation = getSourceLocation(node.value); + const defaultLocation = getSourceLocation(node.default); + return context.replaceText( + { file: valueLocation.file, pos: valueLocation.end, end: defaultLocation.end }, + "", + ); + }, + }); +} + +function createRemovePropertyCodeFix(property: ModelProperty): CodeFix { + return defineCodeFix({ + id: "remove-patch-property", + label: `Remove property '${property.name}' from PATCH body`, + fix: (context) => { + const node = property.node; + if (!node || node.kind !== SyntaxKind.ModelProperty) return undefined; + const location = getSourceLocation(node); + const fileText = location.file.text; + // Extend the range to include any trailing terminator (`;` or `,`) and the rest + // of the line (including the newline) so we don't leave an empty line behind. + let end = location.end; + if (end < fileText.length && (fileText[end] === ";" || fileText[end] === ",")) { + end += 1; + } + // Consume trailing horizontal whitespace and a single newline (if present). + while (end < fileText.length && (fileText[end] === " " || fileText[end] === "\t")) { + end += 1; + } + if (fileText[end] === "\r") end += 1; + if (fileText[end] === "\n") end += 1; + // Also consume leading whitespace on the same line so the line is fully removed. + let pos = location.pos; + while (pos > 0 && (fileText[pos - 1] === " " || fileText[pos - 1] === "\t")) { + pos -= 1; + } + return context.replaceText({ file: location.file, pos, end }, ""); + }, + }); +} + function getResourceModel(program: Program, operation: Operation): Model | undefined { const returnType = operation.returnType; if (returnType.kind === "Union") { diff --git a/packages/typespec-azure-resource-manager/test/rules/patch-operations.test.ts b/packages/typespec-azure-resource-manager/test/rules/patch-operations.test.ts index 8e53d205d7..ba7114a858 100644 --- a/packages/typespec-azure-resource-manager/test/rules/patch-operations.test.ts +++ b/packages/typespec-azure-resource-manager/test/rules/patch-operations.test.ts @@ -177,3 +177,958 @@ it("emit diagnostic when there is no request body", async () => { message: "The request body of a PATCH must be a model with a subset of resource properties", }); }); + +it("emits requiredInPatch diagnostic when a PATCH body property is required", async () => { + await tester + .expect( + ` + @armProviderNamespace + namespace Microsoft.Foo; + + model FooResource is TrackedResource { + @visibility(Lifecycle.Read) + @key("foo") + @segment("foo") + @path + name: string; + ...ManagedServiceIdentityProperty; + } + + model MyPatch { + tags: Record; + } + + @armResourceOperations + #suppress "deprecated" "test" + interface FooResources + extends ResourceRead, ResourceCreate, ResourceDelete { + @armResourceUpdate(FooResource) + #suppress "@typespec/http/deprecated-implicit-optionality" "For test" + @patch(#{implicitOptionality: true}) myFooUpdate(...ResourceInstanceParameters, @bodyRoot body: MyPatch) : ArmResponse | ErrorResponse; + } + + enum ResourceState { + Succeeded, + Canceled, + Failed + } + + model FooProperties { + displayName?: string; + provisioningState: ResourceState; + } + `, + ) + .toEmitDiagnostics([ + { + code: "@azure-tools/typespec-azure-resource-manager/arm-resource-patch", + message: + "Property 'tags' is required in the PATCH request body. PATCH request body properties must all be optional or readOnly.", + }, + ]); +}); + +it("emits defaultInPatch diagnostic when a PATCH body property has a default value", async () => { + await tester + .expect( + ` + @armProviderNamespace + namespace Microsoft.Foo; + + model FooResource is TrackedResource { + @visibility(Lifecycle.Read) + @key("foo") + @segment("foo") + @path + name: string; + } + + model MyPatch { + tags?: Record = #{}; + } + + @armResourceOperations + #suppress "deprecated" "test" + interface FooResources + extends ResourceRead, ResourceCreate, ResourceDelete { + @armResourceUpdate(FooResource) + #suppress "@typespec/http/deprecated-implicit-optionality" "For test" + @patch(#{implicitOptionality: true}) myFooUpdate(...ResourceInstanceParameters, @bodyRoot body: MyPatch) : ArmResponse | ErrorResponse; + } + + enum ResourceState { + Succeeded, + Canceled, + Failed + } + + model FooProperties { + provisioningState?: ResourceState; + } + `, + ) + .toEmitDiagnostics([ + { + code: "@azure-tools/typespec-azure-resource-manager/arm-resource-patch", + message: + "Property 'tags' has a default value in the PATCH request body. PATCH request body properties that are not present in the request body leave the value unchanged; they do not result in any default value being assigned.", + }, + ]); +}); + +it("does not emit notUpdateableInPatch when a PATCH body property is read-only (Lifecycle.Read only) on the resource", async () => { + await tester + .expect( + ` + @armProviderNamespace + namespace Microsoft.Foo; + + model FooResource is TrackedResource { + @key("foo") + @segment("foo") + @path + name: string; + } + + model FooProperties { + @visibility(Lifecycle.Read) + readOnlyProp?: string; + + displayName?: string; + } + + model MyPatch { + ...FooProperties; + tags?: Record; + } + + @armResourceOperations + #suppress "deprecated" "test" + interface FooResources + extends ResourceRead, ResourceCreate, ResourceDelete { + @armResourceUpdate(FooResource) + #suppress "@typespec/http/deprecated-implicit-optionality" "For test" + @patch(#{implicitOptionality: true}) myFooUpdate(...ResourceInstanceParameters, @bodyRoot body: MyPatch) : ArmResponse | ErrorResponse; + } + `, + ) + .toBeValid(); +}); + +it("emits notUpdateableInPatch diagnostic when a PATCH body property is not updateable (e.g. Lifecycle.Create only) on the resource", async () => { + await tester + .expect( + ` + @armProviderNamespace + namespace Microsoft.Foo; + + model FooResource is TrackedResource { + @key("foo") + @segment("foo") + @path + name: string; + } + + model FooProperties { + @visibility(Lifecycle.Create) + createOnlyProp?: string; + + displayName?: string; + } + + model MyPatch { + ...FooProperties; + tags?: Record; + } + + @armResourceOperations + #suppress "deprecated" "test" + interface FooResources + extends ResourceRead, ResourceCreate, ResourceDelete { + @armResourceUpdate(FooResource) + #suppress "@typespec/http/deprecated-implicit-optionality" "For test" + @patch(#{implicitOptionality: true}) myFooUpdate(...ResourceInstanceParameters, @bodyRoot body: MyPatch) : ArmResponse | ErrorResponse; + } + `, + ) + .toEmitDiagnostics([ + { + code: "@azure-tools/typespec-azure-resource-manager/arm-resource-patch", + message: + "Property 'createOnlyProp' is in the PATCH request body but is not updateable on the resource. Make this property updateable on the resource or remove it from the PATCH request.", + }, + ]); +}); + +it("emits nonMergePatchContentType diagnostic when content-type is not merge-patch+json", async () => { + await tester + .expect( + ` + @armProviderNamespace + namespace Microsoft.Foo; + + model FooResource is TrackedResource { + @visibility(Lifecycle.Read) + @key("foo") + @segment("foo") + @path + name: string; + } + + model FooProperties { + displayName?: string; + } + + model MyPatch { + tags?: Record; + } + + @armResourceOperations + #suppress "deprecated" "test" + interface FooResources + extends ResourceRead, ResourceCreate, ResourceDelete { + @armResourceUpdate(FooResource) + #suppress "@typespec/http/deprecated-implicit-optionality" "For test" + @patch(#{implicitOptionality: true}) myFooUpdate( + ...ResourceInstanceParameters, + @header("content-type") contentType: "application/xml", + @bodyRoot body: MyPatch, + ) : ArmResponse | ErrorResponse; + } + `, + ) + .toEmitDiagnostics([ + { + code: "@azure-tools/typespec-azure-resource-manager/arm-resource-patch", + message: + "PATCH operation 'myFooUpdate' specifies a content-type 'application/xml' other than 'application/merge-patch+json'.", + }, + ]); +}); + +it("emits nonMergePatchContentType diagnostic with the offending content-type for text/plain", async () => { + await tester + .expect( + ` + @armProviderNamespace + namespace Microsoft.Foo; + + model FooResource is TrackedResource { + @visibility(Lifecycle.Read) + @key("foo") + @segment("foo") + @path + name: string; + } + + model FooProperties { + displayName?: string; + } + + model MyPatch { + tags?: Record; + } + + @armResourceOperations + #suppress "deprecated" "test" + interface FooResources + extends ResourceRead, ResourceCreate, ResourceDelete { + @armResourceUpdate(FooResource) + #suppress "@typespec/http/deprecated-implicit-optionality" "For test" + @patch(#{implicitOptionality: true}) myFooUpdate( + ...ResourceInstanceParameters, + @header("content-type") contentType: "text/plain", + @bodyRoot body: MyPatch, + ) : ArmResponse | ErrorResponse; + } + `, + ) + .toEmitDiagnostics([ + { + code: "@azure-tools/typespec-azure-resource-manager/arm-resource-patch", + message: + "PATCH operation 'myFooUpdate' specifies a content-type 'text/plain' other than 'application/merge-patch+json'.", + }, + ]); +}); + +it("does not emit nonMergePatchContentType when content-type is application/merge-patch+json", async () => { + await tester + .expect( + ` + @armProviderNamespace + namespace Microsoft.Foo; + + model FooResource is TrackedResource { + @visibility(Lifecycle.Read) + @key("foo") + @segment("foo") + @path + name: string; + } + + model FooProperties { + displayName?: string; + } + + model MyPatch { + tags?: Record; + } + + @armResourceOperations + #suppress "deprecated" "test" + interface FooResources + extends ResourceRead, ResourceCreate, ResourceDelete { + @armResourceUpdate(FooResource) + #suppress "@typespec/http/deprecated-implicit-optionality" "For test" + @patch(#{implicitOptionality: true}) myFooUpdate( + ...ResourceInstanceParameters, + @header("content-type") contentType: "application/merge-patch+json", + @bodyRoot body: MyPatch, + ) : ArmResponse | ErrorResponse; + } + `, + ) + .toBeValid(); +}); + +it("does not emit diagnostics when the 'properties' property of a tracked resource is included as an optional property in the PATCH body", async () => { + await tester + .expect( + ` + @armProviderNamespace + namespace Microsoft.Foo; + + model FooResource is TrackedResource { + @key("foo") + @segment("foo") + @path + name: string; + } + + enum ResourceState { + Succeeded, + Canceled, + Failed, + } + + model FooProperties { + displayName?: string; + + @visibility(Lifecycle.Read) + provisioningState?: ResourceState; + } + + model MyPatch { + properties?: FooProperties; + tags?: Record; + } + + @armResourceOperations + #suppress "deprecated" "test" + interface FooResources + extends ResourceRead, ResourceCreate, ResourceDelete { + @armResourceUpdate(FooResource) + #suppress "@typespec/http/deprecated-implicit-optionality" "For test" + @patch(#{implicitOptionality: true}) myFooUpdate(...ResourceInstanceParameters, @bodyRoot body: MyPatch) : ArmResponse | ErrorResponse; + } + `, + ) + .toBeValid(); +}); + +it("emits notUpdateableInPatch when a PATCH body property has visibility (Lifecycle.Create, Lifecycle.Read) on the resource", async () => { + await tester + .expect( + ` + @armProviderNamespace + namespace Microsoft.Foo; + + model FooResource is TrackedResource { + @key("foo") + @segment("foo") + @path + name: string; + } + + model FooProperties { + @visibility(Lifecycle.Create, Lifecycle.Read) + createReadProp?: string; + + displayName?: string; + } + + model MyPatch { + ...FooProperties; + tags?: Record; + } + + @armResourceOperations + #suppress "deprecated" "test" + interface FooResources + extends ResourceRead, ResourceCreate, ResourceDelete { + @armResourceUpdate(FooResource) + #suppress "@typespec/http/deprecated-implicit-optionality" "For test" + @patch(#{implicitOptionality: true}) myFooUpdate(...ResourceInstanceParameters, @bodyRoot body: MyPatch) : ArmResponse | ErrorResponse; + } + `, + ) + .toEmitDiagnostics([ + { + code: "@azure-tools/typespec-azure-resource-manager/arm-resource-patch", + message: + "Property 'createReadProp' is in the PATCH request body but is not updateable on the resource. Make this property updateable on the resource or remove it from the PATCH request.", + }, + ]); +}); + +it("does not emit requiredInPatch when a required PATCH body property maps back to a resource property with visibility Lifecycle.Read by itself", async () => { + await tester + .expect( + ` + @armProviderNamespace + namespace Microsoft.Foo; + + model FooResource is TrackedResource { + @key("foo") + @segment("foo") + @path + name: string; + } + + model FooProperties { + @visibility(Lifecycle.Read) + readOnlyRequired: string; + + displayName?: string; + } + + model MyPatch { + ...FooProperties; + tags?: Record; + } + + @armResourceOperations + #suppress "deprecated" "test" + interface FooResources + extends ResourceRead, ResourceCreate, ResourceDelete { + @armResourceUpdate(FooResource) + #suppress "@typespec/http/deprecated-implicit-optionality" "For test" + @patch(#{implicitOptionality: true}) myFooUpdate(...ResourceInstanceParameters, @bodyRoot body: MyPatch) : ArmResponse | ErrorResponse; + } + `, + ) + .toBeValid(); +}); + +it("emits notUpdateableInPatch recursively into nested model properties", async () => { + await tester + .expect( + ` + @armProviderNamespace + namespace Microsoft.Foo; + + model FooResource is TrackedResource { + @key("foo") + @segment("foo") + @path + name: string; + } + + model NestedProps { + @visibility(Lifecycle.Create) + createOnlyNested?: string; + + otherNested?: string; + } + + model FooProperties { + nested?: NestedProps; + + displayName?: string; + } + + model MyPatch { + nested?: NestedProps; + tags?: Record; + } + + @armResourceOperations + #suppress "deprecated" "test" + interface FooResources + extends ResourceRead, ResourceCreate, ResourceDelete { + @armResourceUpdate(FooResource) + #suppress "@typespec/http/deprecated-implicit-optionality" "For test" + @patch(#{implicitOptionality: true}) myFooUpdate(...ResourceInstanceParameters, @bodyRoot body: MyPatch) : ArmResponse | ErrorResponse; + } + `, + ) + .toEmitDiagnostics([ + { + code: "@azure-tools/typespec-azure-resource-manager/arm-resource-patch", + message: + "Property 'nested' is in the PATCH request body but is not updateable on the resource. Make this property updateable on the resource or remove it from the PATCH request.", + }, + ]); +}); + +it("emits notUpdateableInPatch recursively into Record property value types", async () => { + await tester + .expect( + ` + @armProviderNamespace + namespace Microsoft.Foo; + + model FooResource is TrackedResource { + @key("foo") + @segment("foo") + @path + name: string; + } + + model NestedProps { + @visibility(Lifecycle.Create) + createOnlyNested?: string; + + otherNested?: string; + } + + model FooProperties { + items?: Record; + + displayName?: string; + } + + model MyPatch { + items?: Record; + tags?: Record; + } + + @armResourceOperations + #suppress "deprecated" "test" + interface FooResources + extends ResourceRead, ResourceCreate, ResourceDelete { + @armResourceUpdate(FooResource) + #suppress "@typespec/http/deprecated-implicit-optionality" "For test" + @patch(#{implicitOptionality: true}) myFooUpdate(...ResourceInstanceParameters, @bodyRoot body: MyPatch) : ArmResponse | ErrorResponse; + } + `, + ) + .toEmitDiagnostics([ + { + code: "@azure-tools/typespec-azure-resource-manager/arm-resource-patch", + message: + "Property 'items' is in the PATCH request body but is not updateable on the resource. Make this property updateable on the resource or remove it from the PATCH request.", + }, + ]); +}); + +it("does not infinite-loop on cyclic model graphs in the recursive notUpdateableInPatch check", async () => { + await tester + .expect( + ` + @armProviderNamespace + namespace Microsoft.Foo; + + model FooResource is TrackedResource { + @key("foo") + @segment("foo") + @path + name: string; + } + + model NodeA { + b?: NodeB; + value?: string; + } + + model NodeB { + a?: NodeA; + value?: string; + } + + model FooProperties { + graph?: NodeA; + + displayName?: string; + } + + model MyPatch { + graph?: NodeA; + tags?: Record; + } + + @armResourceOperations + #suppress "deprecated" "test" + interface FooResources + extends ResourceRead, ResourceCreate, ResourceDelete { + @armResourceUpdate(FooResource) + #suppress "@typespec/http/deprecated-implicit-optionality" "For test" + @patch(#{implicitOptionality: true}) myFooUpdate(...ResourceInstanceParameters, @bodyRoot body: MyPatch) : ArmResponse | ErrorResponse; + } + `, + ) + .toBeValid(); +}); + +it("does not emit notUpdateableInPatch for default-visibility properties (no @visibility decorator)", async () => { + // Default lifecycle visibility for a property with no @visibility decorator + // includes both Lifecycle.Create and Lifecycle.Update (in fact, all members + // of the Lifecycle enum), so such properties are allowed in PATCH bodies. + await tester + .expect( + ` + @armProviderNamespace + namespace Microsoft.Foo; + + model FooResource is TrackedResource { + @key("foo") + @segment("foo") + @path + name: string; + } + + model FooProperties { + defaultVisProp?: string; + anotherDefault?: int32; + displayName?: string; + } + + model MyPatch { + ...FooProperties; + tags?: Record; + } + + @armResourceOperations + #suppress "deprecated" "test" + interface FooResources + extends ResourceRead, ResourceCreate, ResourceDelete { + @armResourceUpdate(FooResource) + #suppress "@typespec/http/deprecated-implicit-optionality" "For test" + @patch(#{implicitOptionality: true}) myFooUpdate(...ResourceInstanceParameters, @bodyRoot body: MyPatch) : ArmResponse | ErrorResponse; + } + `, + ) + .toBeValid(); +}); + +it("does not emit notUpdateableInPatch for explicit @visibility(Lifecycle.Create, Lifecycle.Update) properties", async () => { + await tester + .expect( + ` + @armProviderNamespace + namespace Microsoft.Foo; + + model FooResource is TrackedResource { + @key("foo") + @segment("foo") + @path + name: string; + } + + model FooProperties { + @visibility(Lifecycle.Create, Lifecycle.Update) + createUpdateProp?: string; + + displayName?: string; + } + + model MyPatch { + ...FooProperties; + tags?: Record; + } + + @armResourceOperations + #suppress "deprecated" "test" + interface FooResources + extends ResourceRead, ResourceCreate, ResourceDelete { + @armResourceUpdate(FooResource) + #suppress "@typespec/http/deprecated-implicit-optionality" "For test" + @patch(#{implicitOptionality: true}) myFooUpdate(...ResourceInstanceParameters, @bodyRoot body: MyPatch) : ArmResponse | ErrorResponse; + } + `, + ) + .toBeValid(); +}); + +it("applies the make-patch-property-optional codefix to a required PATCH body property", async () => { + await tester + .expect( + ` + @armProviderNamespace + namespace Microsoft.Foo; + + model FooResource is TrackedResource { + @visibility(Lifecycle.Read) + @key("foo") + @segment("foo") + @path + name: string; + } + + model MyPatch { + tags: Record; + } + + @armResourceOperations + #suppress "deprecated" "test" + interface FooResources + extends ResourceRead, ResourceCreate, ResourceDelete { + @armResourceUpdate(FooResource) + #suppress "@typespec/http/deprecated-implicit-optionality" "For test" + @patch(#{implicitOptionality: true}) myFooUpdate(...ResourceInstanceParameters, @bodyRoot body: MyPatch) : ArmResponse | ErrorResponse; + } + + model FooProperties { + displayName?: string; + } + `, + ) + .applyCodeFix("make-patch-property-optional").toEqual(` + @armProviderNamespace + namespace Microsoft.Foo; + + model FooResource is TrackedResource { + @visibility(Lifecycle.Read) + @key("foo") + @segment("foo") + @path + name: string; + } + + model MyPatch { + tags?: Record; + } + + @armResourceOperations + #suppress "deprecated" "test" + interface FooResources + extends ResourceRead, ResourceCreate, ResourceDelete { + @armResourceUpdate(FooResource) + #suppress "@typespec/http/deprecated-implicit-optionality" "For test" + @patch(#{implicitOptionality: true}) myFooUpdate(...ResourceInstanceParameters, @bodyRoot body: MyPatch) : ArmResponse | ErrorResponse; + } + + model FooProperties { + displayName?: string; + } + `); +}); + +it("applies the remove-patch-property-default codefix to a PATCH body property with a default value", async () => { + await tester + .expect( + ` + @armProviderNamespace + namespace Microsoft.Foo; + + model FooResource is TrackedResource { + @visibility(Lifecycle.Read) + @key("foo") + @segment("foo") + @path + name: string; + } + + model MyPatch { + displayName?: string = "default"; + tags?: Record; + } + + @armResourceOperations + #suppress "deprecated" "test" + interface FooResources + extends ResourceRead, ResourceCreate, ResourceDelete { + @armResourceUpdate(FooResource) + #suppress "@typespec/http/deprecated-implicit-optionality" "For test" + @patch(#{implicitOptionality: true}) myFooUpdate(...ResourceInstanceParameters, @bodyRoot body: MyPatch) : ArmResponse | ErrorResponse; + } + + model FooProperties { + displayName?: string; + } + `, + ) + .applyCodeFix("remove-patch-property-default").toEqual(` + @armProviderNamespace + namespace Microsoft.Foo; + + model FooResource is TrackedResource { + @visibility(Lifecycle.Read) + @key("foo") + @segment("foo") + @path + name: string; + } + + model MyPatch { + displayName?: string; + tags?: Record; + } + + @armResourceOperations + #suppress "deprecated" "test" + interface FooResources + extends ResourceRead, ResourceCreate, ResourceDelete { + @armResourceUpdate(FooResource) + #suppress "@typespec/http/deprecated-implicit-optionality" "For test" + @patch(#{implicitOptionality: true}) myFooUpdate(...ResourceInstanceParameters, @bodyRoot body: MyPatch) : ArmResponse | ErrorResponse; + } + + model FooProperties { + displayName?: string; + } + `); +}); + +it("applies the remove-patch-property codefix to a non-updateable PATCH body property", async () => { + await tester + .expect( + ` + @armProviderNamespace + namespace Microsoft.Foo; + + model FooResource is TrackedResource { + @key("foo") + @segment("foo") + @path + name: string; + } + + model FooProperties { + @visibility(Lifecycle.Create) + createOnlyProp?: string; + + displayName?: string; + } + + model MyPatch { + ...FooProperties; + tags?: Record; + } + + @armResourceOperations + #suppress "deprecated" "test" + interface FooResources + extends ResourceRead, ResourceCreate, ResourceDelete { + @armResourceUpdate(FooResource) + #suppress "@typespec/http/deprecated-implicit-optionality" "For test" + @patch(#{implicitOptionality: true}) myFooUpdate(...ResourceInstanceParameters, @bodyRoot body: MyPatch) : ArmResponse | ErrorResponse; + } + `, + ) + .applyCodeFix("remove-patch-property").toEqual(` + @armProviderNamespace + namespace Microsoft.Foo; + + model FooResource is TrackedResource { + @key("foo") + @segment("foo") + @path + name: string; + } + + model FooProperties { + + displayName?: string; + } + + model MyPatch { + ...FooProperties; + tags?: Record; + } + + @armResourceOperations + #suppress "deprecated" "test" + interface FooResources + extends ResourceRead, ResourceCreate, ResourceDelete { + @armResourceUpdate(FooResource) + #suppress "@typespec/http/deprecated-implicit-optionality" "For test" + @patch(#{implicitOptionality: true}) myFooUpdate(...ResourceInstanceParameters, @bodyRoot body: MyPatch) : ArmResponse | ErrorResponse; + } + `); +}); + +it("does not emit notUpdateableInPatch for @visibility(Lifecycle.Update)-only properties", async () => { + // Per the visibility rubric, a property is allowed in the PATCH body if its + // visibility includes Lifecycle.Update — even when Lifecycle.Update is the + // only modifier applied. + await tester + .expect( + ` + @armProviderNamespace + namespace Microsoft.Foo; + + model FooResource is TrackedResource { + @key("foo") + @segment("foo") + @path + name: string; + } + + model FooProperties { + @visibility(Lifecycle.Update) + updateOnlyProp?: string; + + displayName?: string; + } + + model MyPatch { + ...FooProperties; + tags?: Record; + } + + @armResourceOperations + #suppress "deprecated" "test" + interface FooResources + extends ResourceRead, ResourceCreate, ResourceDelete { + @armResourceUpdate(FooResource) + #suppress "@typespec/http/deprecated-implicit-optionality" "For test" + @patch(#{implicitOptionality: true}) myFooUpdate(...ResourceInstanceParameters, @bodyRoot body: MyPatch) : ArmResponse | ErrorResponse; + } + `, + ) + .toBeValid(); +}); + +it("does not emit notUpdateableInPatch for @visibility(Lifecycle.Read, Lifecycle.Update) properties (Read+Update without Create)", async () => { + // Per the visibility rubric, a property whose visibility includes + // Lifecycle.Read but not Lifecycle.Create is allowed in PATCH bodies. Here + // Lifecycle.Update is also present, so the property is updateable. + await tester + .expect( + ` + @armProviderNamespace + namespace Microsoft.Foo; + + model FooResource is TrackedResource { + @key("foo") + @segment("foo") + @path + name: string; + } + + model FooProperties { + @visibility(Lifecycle.Read, Lifecycle.Update) + readUpdateProp?: string; + + displayName?: string; + } + + model MyPatch { + ...FooProperties; + tags?: Record; + } + + @armResourceOperations + #suppress "deprecated" "test" + interface FooResources + extends ResourceRead, ResourceCreate, ResourceDelete { + @armResourceUpdate(FooResource) + #suppress "@typespec/http/deprecated-implicit-optionality" "For test" + @patch(#{implicitOptionality: true}) myFooUpdate(...ResourceInstanceParameters, @bodyRoot body: MyPatch) : ArmResponse | ErrorResponse; + } + `, + ) + .toBeValid(); +}); diff --git a/website/src/content/docs/docs/libraries/azure-resource-manager/rules/arm-resource-patch.md b/website/src/content/docs/docs/libraries/azure-resource-manager/rules/arm-resource-patch.md index 86a7c8aefb..16708dd165 100644 --- a/website/src/content/docs/docs/libraries/azure-resource-manager/rules/arm-resource-patch.md +++ b/website/src/content/docs/docs/libraries/azure-resource-manager/rules/arm-resource-patch.md @@ -6,7 +6,12 @@ title: "arm-resource-patch" @azure-tools/typespec-azure-resource-manager/arm-resource-patch ``` -Validate ARM PATCH operations. The request body of a PATCH must be a model with a subset of the resource properties. The PATCH body must not contain properties that do not exist on the resource. +Validate ARM PATCH operations. The request body of a PATCH must be a model with a subset of the resource properties. The PATCH body must not contain properties that do not exist on the resource. In addition, the rule validates that: + +- All properties in the PATCH request body are optional (PATCH supports partial updates) or read-only (not included in requests). +- No PATCH request body property has a default value (a property absent from a PATCH request leaves the existing value unchanged; defaults are not applied). +- Every PATCH request body property maps back to a resource property that is updateable or readOnly. +- The `content-type` header (when explicitly specified) is `application/merge-patch+json` (or the implicit `application/json`). #### ❌ Incorrect @@ -19,6 +24,8 @@ model MyBadPatch { name?: string; ...Foundations.ArmTagsProperty; blah?: string; // does not exist on FooResource + required: string; // not optional + withDefault?: string = "x"; // has a default value } ```