Make Name field match what service returns#44118
Conversation
Next Steps to MergeNext steps that must be taken to merge this PR:
Comment generated by summarize-checks workflow run. |
API Change CheckAPIView identified API level changes in this PR and created the following API reviews
Comment generated by After APIView workflow run. |
ravimeda
left a comment
There was a problem hiding this comment.
ARM API Review
Posting findings from the ARM API Reviewer agent (critic-verified, 1 iteration, converged) against commit 8412021. See the inline comment for finding 1. Findings 2–4 below concern a line and a file outside this PR's diff (models.tsp line 192 and the generated skus.json), so they are posted here as a top-level comment.
2. [NEW] 🔴 Blocking [XmsIdentifierValidation (R4041)] specification/compute/resource-manager/Microsoft.Compute/Compute/ComputeSku/models.tsp — line 192 — @identifiers(#["name"]) references a renamed property.
The zoneDetails?: ResourceSkuZoneDetails[] array (line 193, model ResourceSkuLocationInfo) declares @identifiers(#["name"]), pointing at ResourceSkuZoneDetails.name. This PR renamed that property to Name, so after the swagger regenerates, the emitted x-ms-identifiers: ["name"] on zoneDetails will reference a non-existent property → XmsIdentifierValidation failure. If the rename is kept, update to @identifiers(#["Name"]). If the @encodedName fix in finding 1 is adopted, the property stays name so this decorator is valid — verify the emitted x-ms-identifiers resolves to the wire name. (The second @identifiers(#["name"]) at line 222 targets ResourceSkuCapabilities.name, unchanged — not affected.)
3. [NEW] 🔴 Blocking [TypeSpec generated output must match source] specification/compute/resource-manager/Microsoft.Compute/Compute/stable/2021-07-01/skus.json — ResourceSkuZoneDetails, line 413 — Generated swagger not regenerated.
The PR changes only models.tsp; the committed skus.json still declares name. TypeSpec source and generated OpenAPI are out of sync, so the TypeSpec Validation CI check (which regenerates and diffs) will fail. Regenerate the ComputeSku project and commit the updated skus.json.
4. [NEW] 🟠 Warning [Breaking changes guidelines] specification/compute/resource-manager/Microsoft.Compute/Compute/stable/2021-07-01/skus.json — ResourceSkuZoneDetails.name, line 413 — In-place breaking change to a released stable version.
Renaming a property in the already-shipped stable 2021-07-01 is a wire-contract breaking change that OAD will flag once the swagger regenerates, and released versions are normally immutable. The service-correctness rationale supports an in-place fix, but the breaking-change review/suppression flow must be followed and ARM should confirm an in-place edit (vs. a new version) is acceptable.
Worth surfacing — the underlying tension: The service returns PascalCase Name, so any faithful spec carries a PascalCase wire name. @encodedName keeps the TypeSpec/SDK property camelCase, but the emitted swagger property is still Name, which may itself need a swagger-level naming suppression in LintDiff. This tension can only be managed (encodedName + breaking-change approval + possibly a suppression), not eliminated in the spec — the truly correct long-term fix is service-side (return name).
8412021 to
6b09604
Compare
|
Good comments @ravimeda, I've fixed them and pushed. Also rebased so this is based on latest main to get rid of the "out of date branch" comment |
|
Here's an example raw REST response that shows the current response mismatches with the documented API shape: |
6b09604 to
8d33b47
Compare
|
Here's an AI report of this issue's impact in the various SDKs: Azure SDK Case-Sensitivity Report:
|
| SDK | Read mechanism | Match | Result when service returns Name |
|---|---|---|---|
| Go | switch key { case "name": } |
exact string | ❌ empty (len==0) |
| C# | JsonProperty.NameEquals("name"u8) |
ordinal / case-sensitive bytes | ❌ null; value dumped into raw additionalBinaryDataProperties |
| Python | _data.get("name") dict lookup |
case-sensitive key | ❌ None |
Bottom line: none of Go, C#, or Python are case-insensitive here. The correct fix is exactly what PR #44118 does — align the spec wire name to Name (keeping the client-facing name name), which regenerates all three SDKs to read the key the service actually returns.
ARM (Control Plane) API Specification Update Pull Request
Tip
Overwhelmed by all this guidance? See the
Getting helpsection at the bottom of this PR description.PR review workflow diagram
Please understand this diagram before proceeding. It explains how to get your PR approved & merged.
Purpose of this PR
What's the purpose of this PR? Check the specific option that applies. This is mandatory!
Due diligence checklist
To merge this PR, you must go through the following checklist and confirm you understood
and followed the instructions by checking all the boxes:
ARM resource provider contract and
REST guidelines (estimated time: 4 hours).
I understand this is required before I can proceed to the diagram Step 2, "ARM API changes review", for this PR.
Additional information
Viewing API changes
For convenient view of the API changes made by this PR, refer to the URLs provided in the table
in the
Generated ApiViewcomment added to this PR. You can use ApiView to show API versions diff.Suppressing failures
If one or multiple validation error/warning suppression(s) is detected in your PR, please follow the
suppressions guide to get approval.
Getting help
Purpose of this PRandDue diligence checklist.write accessper aka.ms/azsdk/access#request-access-to-rest-api-or-sdk-repositoriesNext Steps to Mergecomment. It will appear within few minutes of submitting this PR and will continue to be up-to-date with current PR state.and https://aka.ms/ci-fix.
queuedstate, please add a comment with contents/azp run.This should result in a new comment denoting a
PR validation pipelinehas started and the checks should be updated after few minutes.