-
Notifications
You must be signed in to change notification settings - Fork 244
Extend azsdk_typespec_generate_authoring_plan with SDK breaking change detection #15077
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
Changes from all commits
6f15a8b
cb61d4e
9282c28
aba775e
e28a6ac
4e559f9
6aaa2a0
fd94087
f5c88bb
ffd22e0
4fc25f9
2f765fe
e30d182
6245127
6a2934f
2e4b1bd
ad372b0
56d5529
893f51b
8408631
0a456b9
dba5eb5
d9461be
c00b498
90a1e23
d965d8a
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,264 @@ | ||
| # SDK Breaking Change Patterns | ||
|
|
||
| This document catalogs known TypeSpec changes that cause SDK breaking changes, organized by detection source, per-language impact, and recommended `client.tsp` mitigations. It is used by `azsdk_typespec_generate_authoring_plan` to warn developers about SDK impact during TypeSpec authoring. | ||
|
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 this be a instruction or skill that people could invoke at any time to analyze their code? Not sure whether we shall push user to analyze it every time they author something.
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 detection is effectively zero-cost. The authoring plan tool already makes exactly one HTTP call to the KB service per invocation (TypeSpecAuthoringTool.cs:170-171). Breaking change patterns are retrieved server-side via RAG, not an additional round-trip. Making it a separate on-demand skill would actually add overhead (an extra tool invocation) where currently there is none. The patterns are also self-filtering - they only match changes to existing TypeSpec (renames, removals, type splits). A user writing new definitions from scratch won't see breaking change warnings because the patterns don't match. |
||
|
|
||
|
chunyu3 marked this conversation as resolved.
|
||
| ## How to Use This Document | ||
|
|
||
| **Important:** The patterns below are **SDK breaking changes**, not API breaking changes. These TypeSpec changes may be perfectly valid from an API/wire-protocol perspective, but they cause breaking changes in the generated SDK surface (e.g., class renames, removed methods) that affect downstream SDK consumers. API breaking changes (changes to the wire contract) should be caught by TypeSpec API-level validation, not this document. | ||
|
|
||
| When generating an authoring plan for TypeSpec changes, check if the planned changes match any pattern below. If so, include an **SDK IMPACT** warning in the plan with the recommended mitigation. The developer can then apply the `client.tsp` fix in the same session, before merge. | ||
|
|
||
| ## Pattern Catalog | ||
|
|
||
| ### 1. Enum Changed to Extensible Union | ||
|
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. We’ve discussed this before: on the TypeSpec side, we should not allow this kind of change. Instead of modifying an existing API, we should use version decorators to deprecate the old version and introduce a new one.
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. In this case, yes, we can use version decorate to insure that extensible union only start from a special version, as following: after: The pattern still be enum -> union
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. Addressed in 6245127 — added a note that this change should use version decorators ( |
||
|
|
||
| **Detection:** TypeSpec diff shows `enum` keyword replaced by `union` or extensible enum pattern. | ||
|
|
||
| **Note:** This change should generally use version decorators (`@added`) to introduce the union in a new API version rather than modifying the existing enum in place. If version decorators are used correctly, the SDK will handle versioning automatically. However, if the change does occur (e.g., during TypeSpec restructuring), the SDK impact is: | ||
|
|
||
| **Per-Language Impact:** | ||
| - **Java:** ❌ Breaking — enum ordinal values change, deserialization breaks | ||
| - **.NET:** ❌ Breaking — cannot be fully fixed by `client.tsp`; may require .NET customization code | ||
| - **Python:** ✅ Usually safe — duck typing, string comparison | ||
| - **Go:** ❌ Breaking — type assertion fails on new union type | ||
|
|
||
| **Mitigation:** | ||
|
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. If this pattern is not allowed, I don't think we shall teach people how to mitigate it.
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. Updated in 6245127 - added a note that this change should use version decorators (@added) to introduce the union in a new API version. The pattern is kept because if it does occur (e.g., during TypeSpec restructuring), developers need to understand the SDK impact. The mitigation is provided as a safety net, not as encouragement to make the change. |
||
| ```typespec | ||
| // In client.tsp — Define a new enum with all original enum values, then use @@alternateType to map to the new enum type for affected languages. | ||
| enum FooStatusEnum | ||
| { | ||
| // ... all values from original FooStatus Enum | ||
| } | ||
| @@alternateType(MyService.FooStatus, FooStatusEnum, "java"); | ||
| @@alternateType(MyService.FooStatus, FooStatusEnum, "go"); | ||
| ``` | ||
|
|
||
| **Note (.NET):** `@@alternateType` may not fully resolve this for .NET. .NET may need customization code (partial classes) to preserve backward compatibility. | ||
|
|
||
| --- | ||
|
|
||
| ### 2. Model Renamed | ||
|
|
||
| **Detection:** TypeSpec diff shows a model definition's name changed while its structure remains the same or similar. This is distinct from Pattern 5 (Model Removed) — a rename has a clear old→new mapping, while a removal has no replacement. | ||
|
|
||
| This pattern covers two scenarios: | ||
| 1. **Explicit rename:** A model is renamed across API versions (detectable via `@renamedFrom` decorator or paired model removal + addition in TypeSpec diff) | ||
| 2. **Migration naming divergence:** During Swagger→TypeSpec migration, TypeSpec default naming conventions produce different SDK class names than the old Swagger-generated SDK had (e.g., TypeSpec `AccessMode` generates `AccessMode` in C#, but the Swagger SDK used `ContainerAppAccessMode`) | ||
|
|
||
| Both scenarios use the same mitigation (`@@clientName`). | ||
|
|
||
| **How to distinguish from Pattern 5:** If a model disappears from the changelog AND a new model with similar properties appears, this is a rename (Pattern 4). If a model disappears with no replacement, that's a removal (Pattern 5). | ||
|
|
||
|
Comment on lines
+48
to
+51
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. I think we need to aware this comment, why |
||
| **TypeSpec pattern** | ||
| ``` | ||
| @renamedFrom(Versions.v2, "OldModelName") | ||
| model NewModelName { | ||
| prop: string | ||
| } | ||
| ``` | ||
|
|
||
| **Per-Language Impact:** | ||
| - **All languages:** ❌ Breaking — client code references the old name | ||
| - **.NET:** .NET may keep the old model available via customization code (partial classes) rather than `client.tsp` | ||
|
|
||
| **Mitigation:** | ||
| ```typespec | ||
| // In client.tsp — preserve the old name for all languages | ||
| @@clientName(MyService.NewModelName, "OldModelName"); | ||
|
|
||
| // Or scope to specific languages if only some need backward compat | ||
| @@clientName(MyService.NewModelName, "OldModelName", "java"); | ||
| @@clientName(MyService.NewModelName, "OldModelName", "python"); | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ### 3. Model Removed | ||
|
|
||
| **Detection:** TypeSpec diff shows model definition deleted with no replacement. If a replacement model with similar properties exists, this is a rename — see Pattern 4 (Model Renamed) instead. | ||
|
|
||
|
chunyu3 marked this conversation as resolved.
|
||
| **Per-Language Impact:** | ||
| - **All languages:** ❌ Breaking — any code referencing the model fails to compile | ||
| - **Go:** Cannot be resolved through client customizations if model is truly deleted | ||
|
|
||
| **Mitigation:** | ||
| ```typespec | ||
| // In main.tsp — version gate with @removed instead of deleting | ||
| @removed(Versions.v2026_07_01) | ||
| model DeprecatedModel { | ||
| // ... | ||
| } | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ### 4. Property Type Changed | ||
|
|
||
| **Detection:** TypeSpec diff shows property type changed (e.g., `int32` → `duration`, `string` → custom model). | ||
|
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. Not all type change can be mitigate by client.tsp
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. Addressed in 6245127 — added caveat: "Not all type changes can be mitigated by |
||
|
|
||
| **Per-Language Changelog Pattern:** | ||
| - **Go:** `Type of 'Test.Prop' has been changed from '*string' to '*int32'` | ||
| - **Python:** Property type change visible in changelog | ||
| - **Java/.NET:** Getter return type changes | ||
|
|
||
| **Per-Language Impact:** | ||
| - **Java:** ❌ Breaking — deserialization and getter return type change | ||
| - **.NET:** ❌ Breaking — analyzer AZC0030 flags type mismatches; use `@@alternateType` with `Azure.ResourceManager.CommonTypes.ArmResourceIdentifier` for ID properties | ||
| - **Python:** ⚠️ May break — mypy type check fails | ||
| - **Go:** ❌ Breaking — cannot be resolved through client customizations | ||
|
|
||
| **Note:** Not all type changes can be mitigated by `client.tsp`. If the type change is fundamental (e.g., switching between incompatible model hierarchies), the spec change itself may need to be reconsidered. | ||
|
|
||
| **Mitigation:** | ||
| ```typespec | ||
| // In client.tsp — use @@alternateType to keep old type for affected languages | ||
| @@alternateType(MyService.MyModel.timeout, int32, "java"); | ||
|
|
||
| // For .NET — use CommonTypes for ARM resource IDs | ||
| @@alternateType(MyService.MyModel.resourceId, Azure.ResourceManager.CommonTypes.ArmResourceIdentifier, "csharp"); | ||
| ``` | ||
|
|
||
| **Note (Go):** Property type changes cannot be resolved through `client.tsp` customizations in Go. The spec change itself must be reconsidered if Go backward compatibility is required. | ||
|
|
||
| --- | ||
|
|
||
| ### 5. Property Renamed | ||
|
|
||
| **Detection:** TypeSpec diff shows property name changed, or changelog shows renamed getter/setter. | ||
|
|
||
| **Per-Language Changelog Pattern:** | ||
| - **Go:** `Field 'A' of struct 'Test' has been removed` / `New field 'B' in struct 'Test'` | ||
| - **Python:** Paired removal/addition of model properties | ||
| - **Java/.NET:** Getter/setter name changes | ||
|
|
||
| **Per-Language Impact:** | ||
| - **All languages:** ❌ Breaking — getter/setter name changes, customization code drifts | ||
| - **Java/Python:** Additional risk — customization files (`*Customization.java`, `*_patch.py`) may reference old name | ||
|
|
||
| **Mitigation:** | ||
| ```typespec | ||
| // In client.tsp — preserve old property name | ||
| @@clientName(MyService.MyModel.newPropertyName, "oldPropertyName"); | ||
|
|
||
| // Scope per language if needed | ||
| @@clientName(MyService.MyModel.newPropertyName, "oldPropertyName", "go"); | ||
| @@clientName(MyService.MyModel.newPropertyName, "oldPropertyName", "python"); | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ### 6. Operation Renamed | ||
|
|
||
| **Detection:** TypeSpec diff shows operation name changed, or changelog shows paired method removal + addition. This is distinct from Pattern 10 (Operation Removed) — a rename has a clear old→new mapping, while a removal has no replacement. | ||
|
|
||
| **How to distinguish from Pattern 8:** If a method disappears from the changelog AND a new method with similar parameters/return type appears, this is a rename (Pattern 6). If a method disappears with no replacement, that's a removal (Pattern 8). | ||
|
|
||
| **Per-Language Changelog Pattern:** | ||
| - **Python:** `Removed operation StorageTaskAssignmentOperations.list` / `Added operation StorageTaskAssignmentOperations.storage_task_assignment_list` | ||
| - **Go:** `Function 'A' has been removed` / `New function '*xxx.B(xxx) *xxx'` | ||
| - **Java/.NET:** Method name changes in changelog | ||
|
|
||
| **Per-Language Impact:** | ||
| - **All languages:** ❌ Breaking — method names change in generated clients | ||
|
|
||
| **Mitigation:** | ||
| ```typespec | ||
| // In client.tsp — restore the original operation name | ||
| @@clientName(MyService.StorageTaskAssignment.storageTaskAssignmentList, "list", "python"); | ||
| @@clientName(MyService.MyInterface.B, "A", "go"); | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ### 7. Operation Signature Changed (Parameters Added/Removed/Reordered) | ||
|
|
||
| **Detection:** TypeSpec diff shows operation parameters added/removed/retyped, or changelog shows parameter changes. | ||
|
|
||
| **Per-Language Changelog Pattern:** | ||
| - **Python:** `Method 'IotDpsResourceOperations.get' re-ordered its parameters from ['self', 'provisioning_service_name', 'resource_group_name'] to ['self', 'resource_group_name', 'provisioning_service_name']` | ||
| - **Go:** `Function '*xxx.Test' parameter(s) have been changed from '(string)' to '(string, int32)'` | ||
| - **Java/.NET:** Method signature changes | ||
|
|
||
| **Per-Language Impact:** | ||
| - **All languages:** ❌ Breaking — method signatures change in generated clients | ||
| - **Go:** Required parameter additions, optional-to-required changes, and parameter deletions cannot be resolved through client customizations | ||
|
|
||
| **Mitigation:** | ||
|
|
||
| - **All languages**: Required parameter additions, optional-to-required changes, and Required parameter deletions cannot be resolved through client customizations | ||
|
|
||
| **Mitigation (parameter reorder):** | ||
|
|
||
| ```typespec | ||
| // In client.tsp — use @@override to control parameter order | ||
| op MyCustomOp( | ||
| @path provisioningServiceName: string, | ||
| ...Azure.ResourceManager.CommonTypes.ResourceGroupNameParameter, | ||
| ): ProvisioningServiceDescription; | ||
|
|
||
| @@override(ProvisioningServiceDescriptions.get, MyCustomOp, "python"); | ||
| ``` | ||
|
|
||
| **Mitigation (parameter add optional parameter):** | ||
|
|
||
| ```typespec | ||
| // In client.tsp — use @@override to control parameter order | ||
| op MyCustomOp( | ||
| @path provisioningServiceName: string, | ||
| ...Azure.ResourceManager.CommonTypes.ResourceGroupNameParameter, | ||
| @query provider?: string //put the added optional paramerter at the last | ||
|
chunyu3 marked this conversation as resolved.
|
||
| ): ProvisioningServiceDescription; | ||
|
|
||
| @@override(ProvisioningServiceDescriptions.get, MyCustomOp); | ||
| ``` | ||
|
|
||
| ### 8. Operation Removed or Hidden | ||
|
|
||
| **Detection:** TypeSpec diff shows operation deleted or `@removed` decorator added, with no replacement operation. If a replacement operation with similar parameters exists, this is a rename — see Pattern 6 (Operation Renamed) instead. | ||
|
|
||
| **Per-Language Impact:** | ||
| - **All languages:** ❌ Breaking — method no longer exists in client | ||
| - **Go:** Cannot be resolved through client customizations | ||
|
|
||
| **typespec pattern:** | ||
| ```typespec | ||
| // In main.tsp — use version gating instead of deletion | ||
| @removed(Versions.v2026_07_01) | ||
| op oldOperation(): void; | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ### 9. Combine multiple model properties into one | ||
|
|
||
| **Detection:** TypeSpec diff shows that one or more properties in a model are combined into a new model, and a new property of that model is added. | ||
|
|
||
| **Per-Language Impact:** | ||
| - **All languages:** ❌ Breaking — missing properties and new property added in a model | ||
|
|
||
| **Mitigation:** This can be resolved by applying `@flattenProperty` to the new combined property in `client.tsp`. | ||
|
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. flatten is only allowed for TypeSpec conversion
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. Addressed in 6245127 — added note that |
||
|
|
||
| **Note:** The legacy `@flattenProperty` decorator is generally only used during TypeSpec conversion from Swagger. For new TypeSpec APIs, prefer restructuring the model hierarchy instead of relying on flatten. | ||
|
|
||
| --- | ||
|
|
||
| ### 10. Interface Renamed (DataPlane only) | ||
|
|
||
| **Detection:** TypeSpec diff shows interface name changed. | ||
|
|
||
| **Per-Language Impact:** | ||
| - **All languages:** ❌ Breaking — client name changed | ||
|
|
||
| **Mitigation:** | ||
| ```typespec | ||
| // In client.tsp — restore the original interface name | ||
| @@clientName(MyService.newInterfaceName, "oldInterfaceName"); | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ## Detection Sources | ||
|
|
||
| | Source | What It Detects | When Available | | ||
| |--------|----------------|----------------| | ||
| | **TypeSpec diff** | Structural changes: renames, type changes, removals, enum→union | During authoring (inner loop) | | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,7 @@ public interface IGitHelper | |
| public Task<string> DiscoverRepoRootAsync(string pathInRepo, CancellationToken ct); | ||
| public Task<string> GetRepoNameAsync(string pathInRepo, CancellationToken ct); | ||
| public Task<List<string>> GetChangedFilesAsync(string repoRoot, string targetCommitish, string? sourceCommitish, string? diffPath, string diffFilterType, CancellationToken ct); | ||
| public Task<List<string>> GetDiffAsync(string repoRoot, string? diffPath, string? targetBranch, CancellationToken ct); | ||
| } | ||
|
|
||
| public class GitHelper(IGitHubService gitHubService, IGitCommandHelper gitCommandHelper, ILogger<GitHelper> logger) : IGitHelper | ||
|
|
@@ -307,6 +308,45 @@ public async Task<List<string>> GetChangedFilesAsync( | |
| .ToList(); | ||
| } | ||
|
|
||
| public async Task<List<string>> GetDiffAsync( | ||
|
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. Thanks @chunyu3 for adding the diff comparison. May I ask three questions?
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. Hi @haolingdong-msft, great questions! Let me address each: 1. How user's local code is passed in:
2. Where we pull code from the latest stable:
This works because our breaking change patterns are structural — if you rename a model from 3. Test cases:
The diff-based approach naturally handles this because it always compares against the baseline branch, not incremental prompts.
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. Thanks Sameeksha for the detailed reponse! Just would to understand more on the first question. e.g. user adds two promts:
If front end does not pass in prompt 1's change, will the tool detect prompt 2 as breaking change?
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. If user does not promote 'add a property a model M' to authoring plan, but he directly add a property to model M directly, then he prompt 'rename property a to b' to authoring plan, authoring plan will pass the typespec diff ( add property), then when authoring check if there is any SDK breaking change for the change 'rename property a to b', it will considering previous change, and think this change is target on a added property, so no SDK breaking change will be reported.
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. thanks @chunyu3 for the explain! Would be great if we can add a test case to verify. |
||
| string repoRoot, | ||
| string? diffPath, | ||
| string? targetBranch, | ||
| CancellationToken ct) | ||
| { | ||
| var args = new List<string> | ||
| { | ||
| "-c", "core.quotepath=off", | ||
| "-c", "i18n.logoutputencoding=utf-8", | ||
| "diff", | ||
| }; | ||
|
|
||
| if (!string.IsNullOrEmpty(targetBranch)) | ||
| { | ||
| args.Add($"{targetBranch}"); | ||
| } | ||
|
|
||
| if (!string.IsNullOrEmpty(diffPath)) | ||
| { | ||
| args.Add("--"); | ||
| args.Add(diffPath); | ||
| } | ||
|
|
||
| var options = new GitOptions([.. args], repoRoot, logOutputStream: false); | ||
| var result = await gitCommandHelper.Run(options, ct); | ||
|
|
||
| if (result.ExitCode != 0) | ||
| { | ||
| throw new InvalidOperationException($"git diff failed: {result.Output}"); | ||
| } | ||
|
|
||
| return result.Stdout | ||
| .Split(['\r', '\n'], StringSplitOptions.RemoveEmptyEntries) | ||
| .Select(line => line.Trim()) | ||
| .Where(line => !string.IsNullOrEmpty(line)) | ||
| .ToList(); | ||
|
chunyu3 marked this conversation as resolved.
|
||
| } | ||
|
|
||
| /// <summary> | ||
| /// Converts SSH GitHub URLs to HTTPS format | ||
| /// </summary> | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By reading this PR, I think the intension of this file is to let people understand the impact of typespec change to SDK without SDK generation & build. but first, I would like to understand whether all the changes listed below are already API breaking changes. If so, it is better to detect those breaking changes at typespec API level directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great question. These are NOT API breaking changes - they are SDK breaking changes. The TypeSpec change may be perfectly valid from an API/wire-protocol perspective (e.g., renaming IPRule to IpRule for consistency), but the generated SDK class name changes, breaking downstream SDK consumers. API breaking changes should be caught at the TypeSpec API level. This document focuses specifically on the SDK surface impact that TypeSpec API validation does not cover. Updated the document header in 6245127 to clarify this distinction.