feat(azure-core): add three Azure REST API Guidelines linter rules - #5055
feat(azure-core): add three Azure REST API Guidelines linter rules#5055xirzec wants to merge 3 commits into
Conversation
Prototype encoding vNext data-plane guidelines as deterministic lint rules: - `no-version-in-route` (versioning-no-version-in-path): rejects literal version segments such as `/v1/` or `/v2` in resolved operation paths. - `api-version-date-format` (versioning-date-based-versioning): requires the values of the `@versioned` enum to be a real `YYYY-MM-DD` date with an optional `-preview` suffix. - `no-dollar-prefixed-query-params` (collections-query-options-no-dollar-sign): rejects OData-style `\$`-prefixed wire names for the seven standard collection query options. All three are enabled in the data-plane ruleset. `api-version-date-format` is disabled for resource-manager, which has its own conflicting version format rule that permits suffixes other than `-preview`. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 07c7eb54-a85a-494c-b2bc-4dd967523a02
|
All changed packages have been documented.
Show changes
|
commit: |
|
You can try these changes here
|
Both rules could report a single offending declaration many times. no-dollar-prefixed-query-params walked each operation's resolved query parameters, so a shared parameter model spread into N operations produced N diagnostics for one declaration. It now resolves each property back through sourceProperty to the declaration it was copied from, reports there, and reports each declaration only once. Against azure-rest-api-specs this takes the rule from 295 diagnostics to 81; Batch alone goes from 180 to 36, all in one file. no-version-in-route reported both an operation and every is alias of it, because an alias inherits the route. It now skips an alias whose version segment comes from its source operation, so the diagnostic lands on the declaration that owns the route. An alias that declares its own versioned @route is still reported, and an alias of an excluded (library) operation is still reported since the source is never visited. This takes Purview from 178 diagnostics to 89. Neither change relaxes what the rules detect: distinct declarations that happen to share a name are still reported separately. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 07c7eb54-a85a-494c-b2bc-4dd967523a02
Investigating also turned up a real bug in two of the rules — they reported the same declaration once per consuming operation (Batch: 180 diagnostics for 36 declarations) and double-reported Suppressions go to
Happy to expand on any of it — severity semantics, the per-cluster breakdown, or why "only lint new APIs" isn't expressible in the linter model. |
There was a problem hiding this comment.
are those new rules? is there any conflict with the ongoing lintdiff migration work or is this part of it?
@catalinaperalta @markcowl
There was a problem hiding this comment.
Yeah, this is from me asking Copilot to look at any gaps from our REST guidelines. Happy to avoid duplicating any work that Cat and Mark have in flight.
Align the rule id with the repo's linter naming convention (use the `use-<preferred-thing>` prefix for rules that point to a preferred TypeSpec pattern), per reviewer feedback. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 07c7eb54-a85a-494c-b2bc-4dd967523a02
markcowl
left a comment
There was a problem hiding this comment.
I don't think any of these rules meet the bar for linting rules. These some more like authoring guidance.
| @@ -0,0 +1,57 @@ | |||
| The standard collection query options (`filter`, `orderby`, `skip`, `top`, `maxpagesize`, `select`, and `expand`) come from the OData standard, where they are spelled with a `$` prefix. Azure services use the same names, but without the `$`. | |||
There was a problem hiding this comment.
This is not true for management plane - this should be narrowly focused on data plane services.
| /** | ||
| * Matches a path segment that looks like an API version, e.g. `v1`, `V2`, `v1.0`, `v2.1.3`. | ||
| */ | ||
| const versionSegmentPattern = /^v\d+(\.\d+)*$/i; |
There was a problem hiding this comment.
I am skeptical of this rule. We are unsure if any path like this represents an api-version and not something else. I think we should reserve these rules for real, impactful violations that are likely
| @@ -0,0 +1,60 @@ | |||
| import { createRule, fileRef, listServices, paramMessage } from "@typespec/compiler"; | |||
There was a problem hiding this comment.
This is duplicative of the rule that already exists in ARM. However, unlike in ARM, this is not always true for data plane APIs, which is why this wasn't added for data planes. I would suggest not adding this rule, and if you do, moving the ARM rule into azure-core instead.
Summary
This is the first batch of a broader effort to encode mechanical statements from the Azure REST API Guidelines (vNext data-plane) as deterministic linter rules, rather than relying on manual (or LLM-based) review to catch them.
Three new
@azure-tools/typespec-azure-corerules, allwarningseverity (consistent with every other rule in the package — in fact the compiler typesLinterRuleDefinitionBase.severityas the literal"warning", so a linter rule cannot be anything else):data-planeresource-managerno-version-in-routeversioning-no-version-in-pathuse-date-api-versionversioning-date-based-versioningno-dollar-prefixed-query-paramscollections-query-options-no-dollar-signValidation — zero false positives on in-repo specs
Every spec in the repo was compiled against the real rulesets (not a synthetic harness) and checked for diagnostics from the three new rules:
packages/samples/specs/data-plane/*(5 specs)data-planepackages/samples/specs/resource-manager/*(6 specs)resource-managerpackages/azure-http-specs/specs/azure/core/*data-planepackages/azure-http-specs/specs/azure/resource-manager/*(10 specs)resource-managerFiring was separately verified end-to-end against a deliberately-violating spec, so the zeroes above are "no violations present", not "rule silently disabled".
Other checks:
Azure.Core/Azure.ResourceManagerlibrary types are exempt).typespec-azure-coresuite green.typespec-azure-rulesetssuite (incl.validate-rules-defined): 4/4.pnpm lint,pnpm format,pnpm cspell,regen-docsall clean.Why these aren't duplicates of
casing-style— wire names vs. TypeSpec identifiersWorth calling out explicitly, because it looks like overlap at first glance.
casing-styleinspects TypeSpec identifiers only —model.name,property.name,operation.name, and namespace/interface names. It never looks at the string arguments of@query(...),@header(...), or@route(...).That means this is completely invisible to the existing rule:
The TypeSpec identifier is
filter— perfectly camelCase, no complaint. The wire name the customer actually types into a URL is$filter.no-dollar-prefixed-query-paramsreadsHttpProperty.options.name(the resolved wire name), which is a capability the linter did not previously have. Same story forno-version-in-route, which reads the resolvedHttpOperation.pathrather than the raw@routestring, so version segments contributed by a namespace- or interface-level@routeare also caught.This is a new axis of checking and is where I expect most of the remaining mechanical guidelines to live (header casing, query-param casing, JSON field casing).
Ruleset matrix and reasoning
Guidelines.mdvNext is the data-plane contract. ARM has its own, separately-governed contract, so a data-plane guideline is not automatically an ARM guideline. Two of the three rules are therefore deliberately disabled inresource-manager.ts:no-dollar-prefixed-query-params→ disabled for ARM. ARM ships the OData spelling by design, in this very repo:Enabling the rule for ARM would flag every ARM service that uses the library's own standard parameters. (The rule already exempts the library declarations themselves via
isExcludedCoreType, but a service spreadingArmTopParameterinto its own operation would still be flagged — which is exactly the unactionable diagnostic we want to avoid.)use-date-api-version→ disabled for ARM. ARM already hasarm-resource-invalid-version-format, whose pattern is/^[\d]{4}-[\d]{2}-[\d]{2}(-[\w]+(\.\d+)?)?$/— it permits arbitrary suffixes such as-alpha.1and-beta.2. The data-plane guideline permits only-preview. These are in genuine conflict, and ARM's existing rule wins on ARM.no-version-in-route→ enabled for ARM. This is a deliberate decision, not an oversight. Reasoning:api-version-query-parameter based; a literal/v1/path segment is wrong under both contracts, so this is a shared constraint rather than a data-plane-only one./subscriptions/{id}/resourceGroups/{rg}/providers/{RP}/{type}/{name}); there is no legitimate place for a version segment.v<digits>segment appears in any ARM library template (packages/typespec-azure-resource-manager/lib/**), any ARM sample, or any ARM spec inazure-http-specs— all confirmed by compilation, not just grep.arm-resource-path-segment-invalid-chars, only constrains the character set and casing of a segment; it would happily acceptv2.Residual risk: I can only verify against specs in this repo. If a real ARM RP in
azure-rest-api-specscarries a nested resource-type segment that happens to matchv<digits>,int:azure-specswill surface it, and scoping the rule to data-plane only is a one-line change toresource-manager.ts. Happy to flip it pre-emptively if reviewers prefer.Rule details
no-version-in-routeReports operations whose resolved HTTP path contains a literal API-version segment.
Intentionally narrow to keep false positives at zero: it matches only
v<digits>[.<digits>]*(case-insensitive), and skips{...}path-parameter segments so a parameter that happens to be namedv1is not flagged. It does not flag date-shaped path segments, even though the guideline arguably covers them — that felt like it would need more real-world evidence before turning on.use-date-api-versionValidates the values of the enum referenced by
@versioned.A shape regex alone is insufficient —
2021-13-45and2023-02-29both match\d{4}-\d{2}-\d{2}. The rule does a real calendar round-trip viaDate.UTCand emits a distinctinvalidDatemessage so the diagnostic explains why the value is rejected rather than just restating the pattern.no-dollar-prefixed-query-paramsReports
$-prefixed wire names for the seven standard collection query options (filter,orderby,skip,top,maxpagesize,select,expand), and suggests the unprefixed name in the message.Handles both the explicit-
@query("...")form and the backtick-identifier form (where the wire name comes from the property name). Non-standard$-prefixed names (e.g.$custom) are intentionally left alone — the guideline is about the OData collection options specifically.Update: External Integration results and duplicate-reporting fix
The
int:azure-specscheck surfaced violations in existing specs. All are true positives — external-standard compatibility (OCI Distribution Spec for ACR's/v2/, Apache Atlas for Purview's/atlas/v2/), deliberate OData (Batch, Search), and already-shipped api-versions (Key Vault7.5/7.6). Renaming any of them would be a breaking change to released SDKs, so they need suppressions rather than fixes. A companion suppression PR againstazure-rest-api-specstypespec-nextis required before this merges; it has not been created yet, pending reviewer input on its shape.Investigating that also surfaced a genuine bug in two of the rules: they reported the same declaration many times. Both are fixed in this PR.
no-version-in-routeno-dollar-prefixed-query-paramsuse-date-api-versionno-dollar-prefixed-query-paramsnow resolves each property back throughsourcePropertyand reports each declaration once, instead of once per consuming operation (Batch: 180 diagnostics → 36 declarations, all in one file).no-version-in-routeno longer double-reportsis-aliases that inherit their route (Purview: 178 → 89).Neither change relaxes detection — distinct declarations sharing a name are still reported separately, an alias that declares its own versioned
@routeis still reported, and an alias of an excluded library operation is still reported. 5 regression tests added.📋 Full analysis — severity semantics, per-cluster fixability, precedent, and the
typespec-nextvsmainsuppression flow — is in this comment, which also asks two specific questions for reviewers.Notes for reviewers
.mddoc with ✅/❌ examples, an## Impactsection, and a## Suppressionsection explaining when suppression is legitimate and (for the two ARM-disabled rules) why they don't apply to ARM.linter.ts's diff is larger than three lines becausepnpm create:linter-rulere-sorts the entire import block.feature).