-
Notifications
You must be signed in to change notification settings - Fork 90
[Swagger Linter Migration] GuidUsage #5336
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
base: main
Are you sure you want to change the base?
Changes from all commits
605c34a
45aeaff
04ca361
674ddf4
abcf9d4
44bab68
23de97b
afd2b72
5dded4a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| changeKind: feature | ||
| packages: | ||
| - "@azure-tools/typespec-azure-resource-manager" | ||
| --- | ||
|
|
||
| Add the `no-uuid` ARM lint rule, migrated from the Swagger `GuidUsage` validator rule. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| changeKind: internal | ||
| packages: | ||
| - "@azure-tools/typespec-azure-rulesets" | ||
| --- | ||
|
|
||
| Register the ARM `no-uuid` lint rule as disabled in the resource manager ruleset. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| Avoid UUID-typed schemas in Azure Resource Manager APIs unless their use has explicit Azure API review approval. | ||
|
|
||
| ## Impact | ||
|
|
||
| - **Area:** API, SDK | ||
|
|
||
| UUIDs are difficult for customers to create, recognize, and troubleshoot. Prefer stable, human-readable identifiers that follow the resource's naming constraints. UUID wire types also become language-specific UUID types in generated SDKs, which can make an API harder to use consistently across languages. | ||
|
|
||
| The rule checks UUID model properties, HTTP parameters, request and response bodies, response headers, custom scalar aliases, and container types. It also checks UUID formats applied directly with `@format("uuid")`. | ||
|
|
||
| ## Incorrect | ||
|
|
||
| ```tsp | ||
| @armProviderNamespace | ||
| namespace Microsoft.Contoso; | ||
|
|
||
| model WidgetProperties { | ||
| id: Azure.Core.uuid; | ||
| } | ||
| ``` | ||
|
|
||
| ## Correct | ||
|
|
||
| ```tsp | ||
| @armProviderNamespace | ||
| namespace Microsoft.Contoso; | ||
|
|
||
| model WidgetProperties { | ||
| id: string; | ||
| } | ||
| ``` | ||
|
|
||
| If a UUID is required, obtain Azure API review approval and suppress the rule at the authored declaration with the approval context. | ||
|
|
||
| ## LintDiff Equivalent | ||
|
|
||
| This rule corresponds to the Swagger validator rule [GuidUsage](https://github.com/Azure/azure-openapi-validator/blob/6243cb01c16c7535cd3b8df6f45fbeb3c095ed7f/docs/guid-usage.md). |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,190 @@ | ||
| import { | ||
| type ArrayModelType, | ||
| type Model, | ||
| type ModelProperty, | ||
| type Namespace, | ||
| type Operation, | ||
| type Program, | ||
| type RecordModelType, | ||
| type Scalar, | ||
| type Type, | ||
| createRule, | ||
| fileRef, | ||
| getFormat, | ||
| getLocationContext, | ||
| isArrayModelType, | ||
| isRecordModelType, | ||
| } from "@typespec/compiler"; | ||
| import { $ } from "@typespec/compiler/typekit"; | ||
| import { getAllHttpServices } from "@typespec/http"; | ||
|
|
||
| import { getArmProviderNamespace } from "../namespace.js"; | ||
| import { getArmResources } from "../resource.js"; | ||
|
|
||
| export const noUuidRule = createRule({ | ||
| name: "no-uuid", | ||
| docs: fileRef.fromPackageRoot("src/rules/no-uuid.md"), | ||
| description: | ||
| "ARM APIs should avoid UUID-typed schemas unless they have explicit Azure API review approval.", | ||
| severity: "warning", | ||
| url: "https://azure.github.io/typespec-azure/docs/libraries/azure-resource-manager/rules/no-uuid", | ||
| messages: { | ||
| default: | ||
| "UUID usage is not recommended. If UUIDs are required in your service, get sign-off from the Azure API review board.", | ||
| }, | ||
| create(context) { | ||
| const reportedTargets = new Set<ModelProperty | Operation>(); | ||
| const uuidScalar = $(context.program).type.resolve("Azure.Core.uuid", "Scalar"); | ||
| const [services] = getAllHttpServices(context.program); | ||
| const armServices = services.filter((service) => | ||
| getArmProviderNamespace(context.program, service.namespace), | ||
| ); | ||
| const resourceKeyByOperation = getResourceKeyByOperation(context.program); | ||
|
|
||
| return { | ||
| modelProperty: (property) => { | ||
|
msyyc marked this conversation as resolved.
|
||
| if (!isInArmService(property.model?.namespace, armServices)) { | ||
| return; | ||
| } | ||
|
|
||
| if ( | ||
| getFormat(context.program, property) === "uuid" || | ||
| containsUuid(context.program, uuidScalar, property.type) | ||
| ) { | ||
| reportTarget(context, property, reportedTargets); | ||
| } | ||
| }, | ||
| operation: (operation) => { | ||
| const namespace = operation.interface?.namespace ?? operation.namespace; | ||
| if ( | ||
| isInArmService(namespace, armServices) && | ||
| containsUuid(context.program, uuidScalar, operation.returnType) | ||
| ) { | ||
| reportTarget(context, operation, reportedTargets); | ||
| } | ||
| }, | ||
| root: () => { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do you need to make your own traverse here? can't you just use the linter engine, this seems like this should be a pretty simple rule
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The extra traversal is intentional because this rule is trying to cover the resolved HTTP projection of the API while reporting on actionable authored TypeSpec targets. The regular linter traversal handles ordinary declarations, but it does not fully cover generated ARM HTTP shapes with a useful diagnostic location. For example: model Widget is TrackedResource<WidgetProperties> {
...ResourceNameParameter<
Resource = Widget,
KeyName = "widgetName",
SegmentName = "widgets",
Type = Azure.Core.uuid
>;
}
@armResourceOperations
interface Widgets {
get is ArmResourceRead<Widget>;
}This emits an HTTP path parameter with The same projection traversal covers direct Some of the recursive type inspection may still be refactorable, but replacing the resolved HTTP traversal with only semantic listeners would narrow the migrated rule's established coverage or change its diagnostic targets.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hhm really don't like that by that reasoning every single rule should do their own traversal due to the same limitation. I think if this is critical and can't be hacked in the diagnostic target resolution function right now we need to figure out a better way built in for this.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't find a better solution. Let us hold on this PR for now and see if other PRs have similar request for traversal or we could find other solution. CC catalinaperalta
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you not just check modelProperty type(and any other types you want to check) and report on the model property? The typespec engine should report template instantiation trace if it happens in a template
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Timothee Guerin (@timotheeguerin) Yes, this works for the ordinary cases, and I refactored the rule in afd2b72 to use There is one ARM-specific exception. An instantiated This is not just theoretical in the migration corpus: there are 9 UUID resource-name declarations across 6 projects (8 across 5 successfully compiled projects; Quota was excluded by its compile failure), producing 35 Swagger operation-parameter findings. Dropping the case would therefore create a known gap in 5 of the 46 projects detected by the final TypeSpec corpus run. (detailed report is here: https://github.com/Azure/typespec-azure/blob/feature/lintdiff-migration-new/packages/typespec-lintdiff/test/fixtures/GuidUsage/migration.md) As a compromise, the only remaining HTTP projection logic now finds resolved ARM resource-key parameters and maps those library-owned generated parameters to their authored operations. Everything else uses the standard linter traversal. I also added coverage for |
||
| for (const service of armServices) { | ||
| for (const httpOperation of service.operations) { | ||
| const operation = httpOperation.operation; | ||
| const resourceKey = resourceKeyByOperation.get(operation); | ||
| if (resourceKey === undefined) { | ||
| continue; | ||
| } | ||
|
|
||
| const parameter = httpOperation.parameters.parameters.find( | ||
| (parameter) => parameter.param.name === resourceKey, | ||
| ); | ||
| if ( | ||
| parameter !== undefined && | ||
| (getFormat(context.program, parameter.param) === "uuid" || | ||
| containsUuid(context.program, uuidScalar, parameter.param.type)) | ||
| ) { | ||
| reportTarget(context, operation, reportedTargets); | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| }; | ||
| }, | ||
| }); | ||
|
|
||
| function getResourceKeyByOperation(program: Program): Map<Operation, string> { | ||
| const result = new Map<Operation, string>(); | ||
| for (const resource of getArmResources(program)) { | ||
| if (resource.keyName === undefined) { | ||
| continue; | ||
| } | ||
|
|
||
| const operations = [ | ||
| ...Object.values(resource.operations.lifecycle), | ||
| ...Object.values(resource.operations.lists), | ||
| ...Object.values(resource.operations.actions), | ||
| ]; | ||
| for (const operation of operations) { | ||
| if (operation !== undefined) { | ||
| result.set(operation.operation, resource.keyName); | ||
| } | ||
| } | ||
| } | ||
| return result; | ||
| } | ||
|
|
||
| function isWithinNamespace(namespace: Namespace, ancestor: Namespace): boolean { | ||
| for (let current: Namespace | undefined = namespace; current; current = current.namespace) { | ||
| if (current === ancestor) { | ||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| function isInArmService( | ||
| namespace: Namespace | undefined, | ||
| services: readonly { namespace: Namespace }[], | ||
| ): boolean { | ||
| return ( | ||
| namespace !== undefined && | ||
| services.some((service) => isWithinNamespace(namespace, service.namespace)) | ||
| ); | ||
| } | ||
|
|
||
| function containsUuid( | ||
| program: Program, | ||
| uuidScalar: Scalar | undefined, | ||
| type: Type, | ||
| seen = new Set<Type>(), | ||
| ): boolean { | ||
| if (seen.has(type)) { | ||
| return false; | ||
| } | ||
|
|
||
| seen.add(type); | ||
|
|
||
| switch (type.kind) { | ||
| case "Scalar": | ||
| return ( | ||
| type === uuidScalar || | ||
| getFormat(program, type) === "uuid" || | ||
| (type.baseScalar !== undefined && containsUuid(program, uuidScalar, type.baseScalar, seen)) | ||
| ); | ||
| case "Model": | ||
| if (isContainerModel(type)) { | ||
| return containsUuid(program, uuidScalar, type.indexer.value, seen); | ||
| } | ||
| if (getLocationContext(program, type).type === "project") { | ||
| return false; | ||
| } | ||
| // Project model properties are visited by the linter. Recurse only through library wrappers | ||
| // such as ArmResponse<T>, whose instantiated payload property cannot be reported directly. | ||
| return [...type.properties.values()].some( | ||
| (property) => | ||
| getLocationContext(program, property).type !== "project" && | ||
| (getFormat(program, property) === "uuid" || | ||
| containsUuid(program, uuidScalar, property.type, new Set(seen))), | ||
| ); | ||
| case "Tuple": | ||
| return type.values.some((value) => containsUuid(program, uuidScalar, value, new Set(seen))); | ||
| case "Union": | ||
| return [...type.variants.values()].some((variant) => | ||
| containsUuid(program, uuidScalar, variant.type, new Set(seen)), | ||
| ); | ||
| default: | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| function reportTarget( | ||
| context: Parameters<typeof noUuidRule.create>[0], | ||
| target: ModelProperty | Operation, | ||
| reportedTargets: Set<ModelProperty | Operation>, | ||
| ): void { | ||
| if (!reportedTargets.has(target)) { | ||
| reportedTargets.add(target); | ||
| context.reportDiagnostic({ target }); | ||
| } | ||
| } | ||
|
|
||
| function isContainerModel(model: Model): model is ArrayModelType | RecordModelType { | ||
| return isArrayModelType(model) || isRecordModelType(model); | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.