Migrate SecurityInsights to TypeSpec#60255
Conversation
github-actions
Bot
added
the
Mgmt
…o securityinsights-mpg-migration
| { | ||
| /// <summary> | ||
| /// Obsolete legacy wrapper around the Microsoft.OperationalInsights/workspaces resource that previously exposed Security Insights operations as instance methods. | ||
| /// Every public member now throws <see cref="NotSupportedException"/>; the exception message points to the equivalent extension method on <see cref="ArmClient"/> in <see cref="SecurityInsightsExtensions"/>. |
There was a problem hiding this comment.
[4.9] This compatibility wrapper still exists on the public surface and GetOperationalInsightsWorkspaceSecurityInsightsResource(...) still returns it, but every public member now throws NotSupportedException. That preserves the API shape while breaking existing GA callers at runtime. Please keep the obsolete shim functional by delegating to the new ArmClient extension methods instead of throwing.
| @@ -17,4 +13,4 @@ public partial class SecurityInsightsWatchlistData | |||
| /// <summary> The source of the watchlist. </summary> | |||
| public Source? Source { get; set; } | |||
There was a problem hiding this comment.
[4.9] Source is being restored only as a disconnected auto-property. The generated model now reads/writes SourceString through Properties, so callers using the old GA member will silently read defaults and writes will not flow into the payload. Please make this compatibility member translate to the wire-backed member (or restore the shipped shape through spec/customization) rather than leaving it detached.
| namespace Azure.ResourceManager.SecurityInsights.Models | ||
| { | ||
| /// <summary> Represents a query to run on the TI objects in the workspace. </summary> | ||
| public partial class CountQuery |
There was a problem hiding this comment.
[5.1] CountQuery is too generic for the new public surface: in IntelliSense it does not convey what is being counted without reading the namespace/docs. Since this type is generator output, please rename it in TypeSpec with a scoped decorator such as @@clientName(..., "csharp") so the emitted C# type carries Security Insights / threat-intelligence context.
| @@ -7,44 +7,15 @@ | |||
|
|
|||
| using System; | |||
| using System.Collections.Generic; | |||
| using Azure.Core; | |||
| using Azure.ResourceManager.SecurityInsights; | |||
|
|
|||
| namespace Azure.ResourceManager.SecurityInsights.Models | |||
| { | |||
| /// <summary> An entity describing the publish status of a content item. </summary> | |||
| public partial class JobItem | |||
There was a problem hiding this comment.
[5.1] JobItem is also too generic as a new public model name. This type represents publication status for Security Insights content, so the emitted C# name should carry that domain context instead of relying on namespace/docs. Please fix this at the TypeSpec layer with a scoped @@clientName(..., "csharp").
| namespace Azure.ResourceManager.SecurityInsights.Models | ||
| { | ||
| /// <summary> The status of the hunt. </summary> | ||
| public readonly partial struct Status : IEquatable<Status> |
There was a problem hiding this comment.
[5.1] Status is too generic as a new public enum-like type name. In IntelliSense it is ambiguous with many unrelated statuses and does not communicate the Security Insights concept it models. Please rename it in TypeSpec with a scoped @@clientName(..., "csharp") so the generated C# type carries domain context.
|
|
||
| namespace Azure.ResourceManager.SecurityInsights.Models | ||
| { | ||
| /// <summary> Template property bag. </summary> | ||
| public partial class TemplateProperties : TemplateBaseProperties | ||
| public partial class TemplateProperties |
There was a problem hiding this comment.
[5.1] TemplateProperties is a placeholder-style public type name and is too generic on its own. Because this is generated surface, please prefer a scoped TypeSpec rename (@@clientName(..., "csharp")) so the C# API uses a Security Insights-specific name rather than exposing this generic one.
pshao25
left a comment
pshao25
left a comment
There was a problem hiding this comment.
Phase 1: no versioning blocker; ApiCompatVersion remains 1.1.0 and there are no package-local ApiCompat baseline additions.
Phase 2: the naming scanner against the 1.1.0 baseline found 26 new rule hits on the added surface. Contextual naming coverage: evaluated 372 new public types and flagged 4 high-confidence cases.
Phase 3: no baseline-suppression issue, but the migration still has runtime compatibility regressions in the legacy shims.
Phase 4 (customization review): the legacy workspace wrapper now preserves shape but breaks behavior by throwing for every public operation, and the watchlist compatibility property shown in the diff is disconnected from the generated wire-backed member.
Phase 5 (decorator preference): the new generic generated type names should be fixed in TypeSpec with scoped @@clientName(..., "csharp") rather than SDK-side/generated-file edits where possible.
Phase 6 (new API triage): the broad 2024-01-preview expansion looks like real additive surface overall; I did not find a high-confidence duplicate API case, but I did find 4 rename/drift naming issues worth fixing.
Final counts: 0 critical, 6 should-fix, 0 suggestions. Total inline comments: 6.
1 participant
Spec PR: Azure/azure-rest-api-specs#44218
Fix #56944