From 668a28d03358c6fade2c2017570ea1eaa6272877 Mon Sep 17 00:00:00 2001 From: Joywambui-maina Date: Tue, 11 Aug 2026 10:25:40 -0700 Subject: [PATCH] fix(wrapper-generator): fail loudly on cmdlet file collisions A second operation resolving to an already-written cmdlet file now fails generation with the full collision list instead of silently overwriting it, which is the silent-drop failure mode AutoRest had. OData cast list/item pairs (owners/graph.user) now merge like plain pairs, and the sweep's collisions land as cited NamingOverrides entries: termStore and agreement-file stitches, default-singleton renames (SubSite, DefaultDrive, DefaultCalendarEvent), and nested navs the SDK never shipped. Remaining families are tracked on #3704. --- tools/Build-WrapperModule.ps1 | 14 +- .../GenerationServiceRegressionTests.cs | 77 ++++++++ tools/WrapperGenerator.Tests/NamingTests.cs | 66 ++++++- tools/WrapperGenerator/CmdletNaming.cs | 52 ++++- tools/WrapperGenerator/NamingOverrides.cs | 182 ++++++++++++++++-- .../PowerShellWrapperGenerationService.cs | 33 +++- tools/WrapperGenerator/README.md | 6 +- .../edge-cases/naming-edge-cases.md | 43 ++++- 8 files changed, 440 insertions(+), 33 deletions(-) diff --git a/tools/Build-WrapperModule.ps1 b/tools/Build-WrapperModule.ps1 index dc67dc464c..3a5a392c1d 100644 --- a/tools/Build-WrapperModule.ps1 +++ b/tools/Build-WrapperModule.ps1 @@ -143,7 +143,19 @@ function Build-OneModule { } $wrapperOut = & dotnet run --project $generatorProject -- -d $spec -o $cmdletsDir -n $clientNs 2>&1 - if ($LASTEXITCODE -ne 0) { $result.FailedAt = 'wrapper-generator'; $result.Error = ($wrapperOut | Select-Object -Last 3) -join ' | '; return $result } + if ($LASTEXITCODE -ne 0) { + # Skip warnings precede the failure; the exception message is what identifies it. + $result.FailedAt = 'wrapper-generator' + $lines = @($wrapperOut | ForEach-Object { "$_" }) + $exception = $lines | Where-Object { $_ -match 'Unhandled exception|Exception:' } | Select-Object -First 1 + $exceptionIndex = if ($exception) { $lines.IndexOf($exception) } else { -1 } + $result.Error = if ($exceptionIndex -ge 0) { + ($lines[$exceptionIndex..([Math]::Min($exceptionIndex + 5, $lines.Count - 1))] | Where-Object { $_ -notmatch '^\s+at ' }) -join ' | ' + } else { + ($lines | Where-Object { $_ -notmatch '^\s+at ' } | Select-Object -First 6) -join ' | ' + } + return $result + } # Generated artifact, machine-local by design (absolute reference into this clone). $csprojPath = Join-Path $srcDir "$moduleName.csproj" diff --git a/tools/WrapperGenerator.Tests/GenerationServiceRegressionTests.cs b/tools/WrapperGenerator.Tests/GenerationServiceRegressionTests.cs index 7275265a81..b63d68d244 100644 --- a/tools/WrapperGenerator.Tests/GenerationServiceRegressionTests.cs +++ b/tools/WrapperGenerator.Tests/GenerationServiceRegressionTests.cs @@ -181,6 +181,83 @@ public async Task GenerateAsync_SkipsUnsupportedODataPathSegments_DoesNotEmitMal Assert.DoesNotContain(files, f => f != "Shared.g.cs"); } + // Two operations resolving to the same cmdlet file must fail generation loudly, + // identifying both operations — never silently overwrite. The real shipped collision + // (/sites/{id}/sites) is renamed via NamingOverrides, so a synthetic self-referential + // path keeps the guard itself exercised. + [Fact] + public async Task GenerateAsync_FailsLoudlyWhenTwoCmdletsResolveToTheSameFile() + { + var document = new OpenApiDocument + { + Paths = new OpenApiPaths(), + Components = new OpenApiComponents + { + Schemas = new Dictionary + { + ["microsoft.graph.widget"] = new OpenApiSchema + { + Type = JsonSchemaType.Object, + Properties = new Dictionary + { + ["displayName"] = new OpenApiSchema { Type = JsonSchemaType.String }, + }, + }, + }, + }, + }; + + static OpenApiOperation ItemGet(OpenApiDocument doc) => new() + { + Responses = new OpenApiResponses + { + ["200"] = new OpenApiResponse + { + Content = new Dictionary + { + ["application/json"] = new OpenApiMediaType + { + Schema = new OpenApiSchemaReference("microsoft.graph.widget", doc), + }, + }, + }, + }, + }; + + // /widgets/{id} and /widgets/{id}/widgets/{id2} both singularize to the noun Widget; + // with two same-noun item GETs nothing merges, and both emit GetMgWidget.g.cs. + document.Paths["/widgets/{widget-id}"] = new OpenApiPathItem + { + Operations = new Dictionary { [HttpMethod.Get] = ItemGet(document) }, + }; + document.Paths["/widgets/{widget-id}/widgets/{widget-id1}"] = new OpenApiPathItem + { + Operations = new Dictionary { [HttpMethod.Get] = ItemGet(document) }, + }; + + var outputDir = Path.Combine(Path.GetTempPath(), "wrapper-generator-tests", Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(outputDir); + try + { + var config = new GeneratorConfig("Microsoft.Graph.PowerShell.Test.Client", outputDir); + var service = new PowerShellWrapperGenerationService(document, config, NullLogger.Instance); + + var ex = await Assert.ThrowsAsync(() => service.GenerateAsync(CancellationToken.None)); + + Assert.Contains("GetMgWidget.g.cs", ex.Message); + Assert.Contains("collision", ex.Message); + // Both colliding cmdlets are named Get-MgWidget, so only their builder expressions + // prove the message identifies both operations. + Assert.Contains("[Widgets[WidgetId]]", ex.Message); + Assert.Contains("Widgets[WidgetId].Widgets[WidgetId1]", ex.Message); + } + finally + { + if (Directory.Exists(outputDir)) + Directory.Delete(outputDir, recursive: true); + } + } + private static OpenApiDocument BuildDocument(HttpMethod method, string path, OpenApiOperation operation) { return new OpenApiDocument diff --git a/tools/WrapperGenerator.Tests/NamingTests.cs b/tools/WrapperGenerator.Tests/NamingTests.cs index 3f101ae752..3e8c8ebca6 100644 --- a/tools/WrapperGenerator.Tests/NamingTests.cs +++ b/tools/WrapperGenerator.Tests/NamingTests.cs @@ -110,6 +110,19 @@ private static CmdletNaming Resolve(string method, string path) => [InlineData("GET", "/solutions/bookingBusinesses/{bookingBusiness-id}", "Get", "MgBookingBusiness")] [InlineData("PATCH", "/solutions/bookingBusinesses/{bookingBusiness-id}", "Update", "MgBookingBusiness")] [InlineData("GET", "/users/{user-id}/calendar", "Get", "MgUserDefaultCalendar")] + // self-referential sites rename to SubSite instead of colliding with the parent noun + [InlineData("GET", "/sites/{site-id}/sites", "Get", "MgSubSite")] + [InlineData("GET", "/sites/{site-id}/sites/{site-id1}", "Get", "MgSubSite")] + [InlineData("GET", "/groups/{group-id}/sites/{site-id}/sites", "Get", "MgGroupSubSite")] + [InlineData("GET", "/groups/{group-id}/sites/{site-id}/sites/{site-id1}", "Get", "MgGroupSubSite")] + // default-singleton renames (issue #3704 oracle sweep) + [InlineData("GET", "/users/{user-id}/drive", "Get", "MgUserDefaultDrive")] + [InlineData("GET", "/groups/{group-id}/drive", "Get", "MgGroupDefaultDrive")] + [InlineData("GET", "/sites/{site-id}/drive", "Get", "MgSiteDefaultDrive")] + [InlineData("GET", "/groups/{group-id}/sites/{site-id}/drive", "Get", "MgGroupSiteDefaultDrive")] + [InlineData("GET", "/users/{user-id}/calendar/events", "Get", "MgUserDefaultCalendarEvent")] + // nested-collection GET renamed by the Groups.md directive (subject $1ByGroup) + [InlineData("GET", "/groups/{group-id}/groupLifecyclePolicies", "Get", "MgGroupLifecyclePolicyByGroup")] // boundary word-overlap collapse (Get-MgDomainNameReference) [InlineData("GET", "/domains/{domain-id}/domainNameReferences", "Get", "MgDomainNameReference")] // adjacent-duplicate collapse (Get-MgUserOnenoteSectionGroup... family) @@ -177,6 +190,34 @@ public void SuppressesOperationsThePublishedSdkOmits() Assert.True(NamingOverrides.IsSuppressed(HttpMethod.Get, "/solutions")); Assert.True(NamingOverrides.IsSuppressed(HttpMethod.Patch, "/solutions")); Assert.False(NamingOverrides.IsSuppressed(HttpMethod.Get, "/solutions/bookingBusinesses/{bookingBusiness-id}")); + + // The /photos collection ships no distinct cmdlet; only the /photo singleton does. + Assert.True(NamingOverrides.IsSuppressed(HttpMethod.Get, "/users/{user-id}/photos")); + Assert.True(NamingOverrides.IsSuppressed(HttpMethod.Get, "/users/{user-id}/photos/{userProfilePhoto-id}")); + Assert.False(NamingOverrides.IsSuppressed(HttpMethod.Get, "/users/{user-id}/photo")); + + // Suffix-matched suppressions apply under any root; siblings stay generated + // (issue #3704: Info-wrapper navs ship nothing, their siblings ship). + Assert.True(NamingOverrides.IsSuppressed(HttpMethod.Get, "/chats/{chat-id}/pinnedMessages/{pinnedChatMessageInfo-id}/message")); + Assert.True(NamingOverrides.IsSuppressed(HttpMethod.Get, "/teams/{team-id}/channels/{channel-id}/sharedWithTeams/{id}/team")); + Assert.False(NamingOverrides.IsSuppressed(HttpMethod.Get, "/teams/{team-id}/channels/{channel-id}/sharedWithTeams/{id}/allowedMembers")); + + // Exact-matched suppressions cover only the named node; descendants with no entry of + // their own stay generated (Security nested navs, issue #3704). + Assert.True(NamingOverrides.IsSuppressed(HttpMethod.Get, "/security/threatIntelligence/hosts/{host-id}/components")); + Assert.False(NamingOverrides.IsSuppressed(HttpMethod.Get, "/security/threatIntelligence/hosts/{host-id}/components/$count")); + Assert.False(NamingOverrides.IsSuppressed(HttpMethod.Get, "/security/threatIntelligence/hosts/{host-id}/passiveDns")); + + // termStore trees are stitched: /termStores/{id} descendants ship nothing (the 402 + // descendant command rows come from the /termStore singleton trees), and the singleton + // root GET ships no distinct cmdlet (Get-MgSiteTermStore serves both /termStore and + // /termStores; GET generates from the collection side only). + Assert.True(NamingOverrides.IsSuppressed(HttpMethod.Get, "/sites/{site-id}/termStores/{store-id}")); + Assert.True(NamingOverrides.IsSuppressed(HttpMethod.Get, "/sites/{site-id}/termStores/{store-id}/sets/{set-id}")); + Assert.True(NamingOverrides.IsSuppressed(HttpMethod.Get, "/sites/{site-id}/termStore")); + Assert.False(NamingOverrides.IsSuppressed(HttpMethod.Patch, "/sites/{site-id}/termStore")); + Assert.False(NamingOverrides.IsSuppressed(HttpMethod.Get, "/sites/{site-id}/termStores")); + Assert.False(NamingOverrides.IsSuppressed(HttpMethod.Get, "/sites/{site-id}/termStore/sets/{set-id}")); } [Theory] @@ -228,14 +269,29 @@ public void GetWithNoStructuralPartnerStaysStandalone() } [Fact] - public void AmbiguousSameNounDoesNotMerge() + public void CastListItemPairMergesLikeAPlainPair() + { + // The published SDK ships one Get-MgGroupOwnerAsUser covering both the cast on the + // collection and the cast on the item; without pairing, both emit the same file. + var list = Resolve("GET", "/groups/{group-id}/owners/graph.user"); + var item = Resolve("GET", "/groups/{group-id}/owners/{directoryObject-id}/graph.user"); + Assert.Equal(list.Noun, item.Noun); + Assert.True(Naming.IsListItemPair(list, item)); + + // Different cast types never pair. + var otherCast = Resolve("GET", "/groups/{group-id}/owners/{directoryObject-id}/graph.servicePrincipal"); + Assert.False(Naming.IsListItemPair(list, otherCast)); + } + + [Fact] + public void SelfReferentialSitesRenameInsteadOfCollidingWithParent() { - // Self-referential /sites: the collection /sites/{id}/sites and the single /sites/{id} - // both resolve to MgSite, but the "item" is the parent, not a child one id deeper, so - // the structural check rejects the merge and both stay standalone. + // Without the SubSite rename, /sites/{id}/sites singularizes to the parent's own noun + // and its cmdlet file would collide with Get-MgSite's. The renamed nouns are pinned in + // ResolvesPublishedSdkNames; this pins that the pair no longer merges or collides. var list = Resolve("GET", "/sites/{site-id}/sites"); var item = Resolve("GET", "/sites/{site-id}"); - Assert.Equal(list.Noun, item.Noun); + Assert.NotEqual(list.Noun, item.Noun); Assert.False(Naming.IsListItemPair(list, item)); } } diff --git a/tools/WrapperGenerator/CmdletNaming.cs b/tools/WrapperGenerator/CmdletNaming.cs index 6cf4339687..a9836723e2 100644 --- a/tools/WrapperGenerator/CmdletNaming.cs +++ b/tools/WrapperGenerator/CmdletNaming.cs @@ -197,16 +197,54 @@ private static string ToCastAwareBuilderMemberName(string segment) => : segment.ToFirstCharacterUpperCase(); // Whether a list GET and an item GET form a mergeable pair for the public Get-MgX - // dispatcher: the item's path must be the list's path plus exactly one trailing id - // (Users[UserId].Messages -> Users[UserId].Messages[MessageId]). Callers group by noun - // first, so this only decides the structural fit; a same-noun item that does not extend the - // list (for example the self-referential /sites/{id} vs /sites/{id}/sites) is rejected. + // dispatcher: the item's path must extend the list's path by exactly one id, either + // trailing (Users[UserId].Messages -> Users[UserId].Messages[MessageId]) or inserted + // before a shared trailing OData cast (Owners.GraphUser -> Owners[Id].GraphUser). Callers + // group by noun first, so this only decides the structural fit; a same-noun item that does + // not extend the list is rejected. public static bool IsListItemPair(CmdletNaming list, CmdletNaming item) { ArgumentNullException.ThrowIfNull(list); ArgumentNullException.ThrowIfNull(item); - return item.PathParamNames.Count == list.PathParamNames.Count + 1 - && item.PathParamNames.Take(list.PathParamNames.Count).SequenceEqual(list.PathParamNames) - && item.BuilderExpression.StartsWith(list.BuilderExpression + "[", StringComparison.Ordinal); + if (item.PathParamNames.Count != list.PathParamNames.Count + 1 + || !item.PathParamNames.Take(list.PathParamNames.Count).SequenceEqual(list.PathParamNames)) + return false; + + if (item.BuilderExpression.StartsWith(list.BuilderExpression + "[", StringComparison.Ordinal)) + return true; + + // OData cast pair: the id inserts BEFORE the trailing cast member, not at the end + // (owners/graph.user vs owners/{id}/graph.user builds Owners.GraphUser vs + // Owners[Id].GraphUser). The published SDK ships these as one cmdlet, same as a + // plain list/item pair; without this the two emit identical file names and collide. + var listCast = TrailingCastMember(list.BuilderExpression); + var itemCast = TrailingCastMember(item.BuilderExpression); + if (listCast is null || !string.Equals(listCast, itemCast, StringComparison.Ordinal)) + return false; + + var listStem = list.BuilderExpression[..^(listCast.Length + 1)]; + var itemStem = item.BuilderExpression[..^(itemCast.Length + 1)]; + if (!itemStem.StartsWith(listStem + "[", StringComparison.Ordinal) || !itemStem.EndsWith("]", StringComparison.Ordinal)) + return false; + var indexer = itemStem[(listStem.Length + 1)..^1]; + return indexer.Length > 0 && !indexer.Contains('[') && !indexer.Contains('.'); + } + + // The kiota builder member for a trailing OData cast segment (GraphUser from + // "graph.user", MicrosoftGraphUser from "microsoft.graph.user"); null when the + // expression does not end in a cast. + private static string? TrailingCastMember(string builderExpression) + { + var lastDot = builderExpression.LastIndexOf('.'); + if (lastDot < 0) + return null; + var member = builderExpression[(lastDot + 1)..]; + if (member.Contains('[')) + return null; + if (member.StartsWith("MicrosoftGraph", StringComparison.Ordinal) && member.Length > 14 && char.IsUpper(member[14])) + return member; + if (member.StartsWith("Graph", StringComparison.Ordinal) && member.Length > 5 && char.IsUpper(member[5])) + return member; + return null; } } diff --git a/tools/WrapperGenerator/NamingOverrides.cs b/tools/WrapperGenerator/NamingOverrides.cs index db36576a21..3aae42bfc8 100644 --- a/tools/WrapperGenerator/NamingOverrides.cs +++ b/tools/WrapperGenerator/NamingOverrides.cs @@ -7,12 +7,13 @@ namespace WrapperGenerator; // Hand-tuned naming exceptions, kept as data with a cited source on every entry. // -// The published Microsoft.Graph names are mostly algorithmic, but a few come from -// hand-written AutoRest directives in the msgraph-sdk-powershell module configs. Matching -// the published names 100% means mirroring those directives here. +// The published Microsoft.Graph names are mostly algorithmic. Entries here cover the rest: +// renames from hand-written AutoRest directives in the msgraph-sdk-powershell module +// configs, and suppressions for spec routes the published SDK ships nothing for. // -// Keep this list short. Add an entry only when the published name cannot come out of the -// naming rules, and cite the directive that created it. +// Add an entry only when the published surface cannot come out of the naming rules, and +// cite the evidence: the directive when one exists, otherwise the shipped-command +// inventory (the oracle, MgCommandMetadata.json). public static partial class NamingOverrides { private enum OverrideKind @@ -22,33 +23,185 @@ private enum OverrideKind StripNounPrefix, } - private sealed record Entry(OverrideKind Kind, HttpMethod? Method, string PathPrefix, bool ExactPath, string? Value, string Reason); + // How Pattern is matched against the normalized path: the full path (Exact), its start + // (Prefix), or its end (Suffix — for navs that recur under many roots, like + // .../resourceRoleScopes/{}/scope appearing under several parents). + private enum PathMatch + { + Exact, + Prefix, + Suffix, + } + + private sealed record Entry(OverrideKind Kind, HttpMethod? Method, string Pattern, PathMatch Match, string? Value, string Reason); private static readonly List Entries = [ // The SDK ships no Update cmdlet for /users/{id}/calendar. Its pipeline removes the // operation outright, in src/Calendar/Calendar.md: remove-path-by-operation // user_UpdateCalendar. The wrapper must not invent a cmdlet the SDK chose to drop. - new(OverrideKind.SuppressOperation, HttpMethod.Patch, "/users/{}/calendar", ExactPath: true, Value: null, + new(OverrideKind.SuppressOperation, HttpMethod.Patch, "/users/{}/calendar", Match: PathMatch.Exact, Value: null, Reason: "Calendar.md remove-path-by-operation user_UpdateCalendar"), // GET /users/{id}/calendar ships as Get-MgUserDefaultCalendar, renamed in // src/Calendar/Calendar.md: "^(User)(Calendar)$" -> "$1Default$2". - new(OverrideKind.ReplaceNoun, HttpMethod.Get, "/users/{}/calendar", ExactPath: true, Value: "UserDefaultCalendar", + new(OverrideKind.ReplaceNoun, HttpMethod.Get, "/users/{}/calendar", Match: PathMatch.Exact, Value: "UserDefaultCalendar", Reason: "Calendar.md directive renames UserCalendar to UserDefaultCalendar"), // The SDK ships no cmdlets for the /solutions root singleton itself (Get-MgSolution / // Update-MgSolution do not exist): src/Bookings/Bookings.md removes every solutionsRoot // operation with remove-path-by-operation ^solution\.solutionsRoot.*$. Exact-path, all // methods, so operations on children like /solutions/bookingBusinesses are unaffected. - new(OverrideKind.SuppressOperation, Method: null, "/solutions", ExactPath: true, Value: null, + new(OverrideKind.SuppressOperation, Method: null, "/solutions", Match: PathMatch.Exact, Value: null, Reason: "Bookings.md remove-path-by-operation ^solution\\.solutionsRoot.*$"), // Most nouns under /solutions/ drop the "Solution" prefix (for example // Get-MgBookingBusiness, Get-MgVirtualEventWebinar). BackupRestore is a known // exception where published cmdlets keep the Solution prefix. - new(OverrideKind.StripNounPrefix, Method: null, "/solutions/", ExactPath: false, Value: "Solution", + new(OverrideKind.StripNounPrefix, Method: null, "/solutions/", Match: PathMatch.Prefix, Value: "Solution", Reason: "Bookings/VirtualEvents naming pattern under /solutions/*; BackupRestore is explicitly excluded in ApplyNounOverrides"), + + // The spec carries two parallel termStore trees; the shipped surface stitches them: + // GET/POST come from the /termStores collection (Get-MgSiteTermStore, New-...), while + // PATCH/DELETE and all 402 descendant command rows come from the /termStore singleton. + // Nothing ships under /termStores/{id}, and the singleton root GET has no distinct + // cmdlet — generating either would collide with its shipped twin. + new(OverrideKind.SuppressOperation, Method: null, "/sites/{}/termstores/{}", Match: PathMatch.Prefix, Value: null, + Reason: "oracle: zero commands under /termStores/{id}; descendants ship from the /termStore singleton tree"), + new(OverrideKind.SuppressOperation, HttpMethod.Get, "/sites/{}/termstore", Match: PathMatch.Exact, Value: null, + Reason: "oracle: Get-MgSiteTermStore serves GET /termStore and /termStores; GET generates from the collection side only"), + new(OverrideKind.SuppressOperation, Method: null, "/groups/{}/sites/{}/termstores/{}", Match: PathMatch.Prefix, Value: null, + Reason: "oracle: zero commands under /termStores/{id}; descendants ship from the /termStore singleton tree"), + new(OverrideKind.SuppressOperation, HttpMethod.Get, "/groups/{}/sites/{}/termstore", Match: PathMatch.Exact, Value: null, + Reason: "oracle: Get-MgGroupSiteTermStore serves GET /termStore and /termStores; GET generates from the collection side only"), + + // Get-MgUserPhoto serves both /photo and /photos; the /photos routes ship no distinct + // cmdlet, and generating them would collide with the singleton's noun. + new(OverrideKind.SuppressOperation, Method: null, "/users/{}/photos", Match: PathMatch.Exact, Value: null, + Reason: "oracle: Get-MgUserPhoto serves /photo and /photos; the collection ships no distinct cmdlet"), + new(OverrideKind.SuppressOperation, Method: null, "/users/{}/photos/{}", Match: PathMatch.Exact, Value: null, + Reason: "oracle: /photos/{} ships nothing; the photo surface is the /photo singleton"), + + // Self-referential /sites: singularizing sites/{id}/sites collapses to the parent's + // noun, so the sub-sites cmdlets would overwrite Get-MgSite. The SDK ships them + // renamed: Get-MgSubSite and Get-MgGroupSubSite (v1.0 and beta, incl. $count). + new(OverrideKind.ReplaceNoun, Method: null, "/sites/{}/sites", Match: PathMatch.Exact, Value: "SubSite", + Reason: "Sites.md directive; oracle ships Get-MgSubSite for /sites/{site-id}/sites"), + new(OverrideKind.ReplaceNoun, Method: null, "/sites/{}/sites/{}", Match: PathMatch.Exact, Value: "SubSite", + Reason: "Sites.md directive; oracle ships Get-MgSubSite for /sites/{site-id}/sites/{site-id1}"), + new(OverrideKind.ReplaceNoun, Method: null, "/groups/{}/sites/{}/sites", Match: PathMatch.Exact, Value: "GroupSubSite", + Reason: "Sites.md directive; oracle ships Get-MgGroupSubSite"), + new(OverrideKind.ReplaceNoun, Method: null, "/groups/{}/sites/{}/sites/{}", Match: PathMatch.Exact, Value: "GroupSubSite", + Reason: "Sites.md directive; oracle ships Get-MgGroupSubSite"), + + // ---- Collision resolutions from the full-inventory oracle sweep (issue #3704). ---- + + // Identity.Governance: agreement file item ops ship only from the /file singleton + // (Update/Remove-MgAgreementFile); /files/{} items ship nothing, /files/{}/versions does. + new(OverrideKind.SuppressOperation, Method: null, "/agreements/{}/files/{}", Match: PathMatch.Exact, Value: null, + Reason: "oracle: /files/{} item ops ship nothing; file surface is the /file singleton (Update/Remove-MgAgreementFile)"), + new(OverrideKind.SuppressOperation, Method: null, "/identitygovernance/termsofuse/agreements/{}/files/{}", Match: PathMatch.Exact, Value: null, + Reason: "oracle: ships nothing; mirrors /agreements/{}/files/{} suppression"), + // GET of the file/files pair ships from the collection (same command on both URIs), + // like the termStore root stitch; Update/Remove stay on the singleton. + new(OverrideKind.SuppressOperation, HttpMethod.Get, "/agreements/{}/file", Match: PathMatch.Exact, Value: null, + Reason: "oracle: Get-MgAgreementFile serves /file and /files; GET generated from the collection side only"), + new(OverrideKind.SuppressOperation, HttpMethod.Get, "/identitygovernance/termsofuse/agreements/{}/file", Match: PathMatch.Exact, Value: null, + Reason: "oracle: Get-MgIdentityGovernanceTermsOfUseAgreementFile serves /file and /files; GET from the collection side only"), + // The /scope node duplicates its parent's noun and ships nothing anywhere; its + // children ship with the Scope segment elided (…ResourceRoleScopeResource). + new(OverrideKind.SuppressOperation, Method: null, "/resourcerolescopes/{}/scope", Match: PathMatch.Suffix, Value: null, + Reason: "oracle: the /scope node ships nothing under any parent; children ship with Scope elided"), + new(OverrideKind.SuppressOperation, Method: null, "/identitygovernance/entitlementmanagement/assignments/{}/assignmentpolicy", Match: PathMatch.Exact, Value: null, + Reason: "nav duplicate of /assignmentPolicies (Get-MgEntitlementManagementAssignmentPolicy); ships nothing"), + new(OverrideKind.SuppressOperation, Method: null, "/identitygovernance/entitlementmanagement/resources/{}/environment", Match: PathMatch.Exact, Value: null, + Reason: "nav duplicate of /resourceEnvironments (Get-MgEntitlementManagementResourceEnvironment); ships nothing"), + new(OverrideKind.SuppressOperation, Method: null, "/identitygovernance/lifecycleworkflows", Match: PathMatch.Exact, Value: null, + Reason: "the container node's own operations ship nothing; its children ship"), + new(OverrideKind.SuppressOperation, Method: null, "/identitygovernance/termsofuse/agreements/{}/acceptances", Match: PathMatch.Prefix, Value: null, + Reason: "ships nothing; acceptances ship from /termsOfUse/agreementAcceptances (Get-MgIdentityGovernanceTermsOfUseAgreementAcceptance)"), + + // Security threatIntelligence: nested navs under articles/{} and hosts/{} duplicate + // the shipped top-level sets (articleIndicators, hostComponents, hostCookies, + // hostPairs, hostPorts, hostSslCertificates, hostTrackers) and ship nothing + // themselves. Exact-only: two of these navs have shipped $count children + // (Get-MgSecurityThreatIntelligenceHost{SslCertificate,Tracker}Count); the other five + // ship no children at all. + new(OverrideKind.SuppressOperation, Method: null, "/security/threatintelligence/articles/{}/indicators", Match: PathMatch.Exact, Value: null, Reason: "oracle: ships nothing; top-level articleIndicators ships"), + new(OverrideKind.SuppressOperation, Method: null, "/security/threatintelligence/articles/{}/indicators/{}", Match: PathMatch.Exact, Value: null, Reason: "oracle: ships nothing; top-level articleIndicators ships"), + new(OverrideKind.SuppressOperation, Method: null, "/security/threatintelligence/hosts/{}/components", Match: PathMatch.Exact, Value: null, Reason: "oracle: ships nothing; top-level hostComponents ships"), + new(OverrideKind.SuppressOperation, Method: null, "/security/threatintelligence/hosts/{}/components/{}", Match: PathMatch.Exact, Value: null, Reason: "oracle: ships nothing; top-level hostComponents ships"), + new(OverrideKind.SuppressOperation, Method: null, "/security/threatintelligence/hosts/{}/cookies", Match: PathMatch.Exact, Value: null, Reason: "oracle: ships nothing; top-level hostCookies ships"), + new(OverrideKind.SuppressOperation, Method: null, "/security/threatintelligence/hosts/{}/cookies/{}", Match: PathMatch.Exact, Value: null, Reason: "oracle: ships nothing; top-level hostCookies ships"), + new(OverrideKind.SuppressOperation, Method: null, "/security/threatintelligence/hosts/{}/hostpairs", Match: PathMatch.Exact, Value: null, Reason: "oracle: ships nothing; top-level hostPairs ships"), + new(OverrideKind.SuppressOperation, Method: null, "/security/threatintelligence/hosts/{}/hostpairs/{}", Match: PathMatch.Exact, Value: null, Reason: "oracle: ships nothing; top-level hostPairs ships"), + new(OverrideKind.SuppressOperation, Method: null, "/security/threatintelligence/hosts/{}/ports", Match: PathMatch.Exact, Value: null, Reason: "oracle: ships nothing; top-level hostPorts ships"), + new(OverrideKind.SuppressOperation, Method: null, "/security/threatintelligence/hosts/{}/ports/{}", Match: PathMatch.Exact, Value: null, Reason: "oracle: ships nothing; top-level hostPorts ships"), + new(OverrideKind.SuppressOperation, Method: null, "/security/threatintelligence/hosts/{}/sslcertificates", Match: PathMatch.Exact, Value: null, Reason: "oracle: ships nothing; top-level hostSslCertificates ships"), + new(OverrideKind.SuppressOperation, Method: null, "/security/threatintelligence/hosts/{}/sslcertificates/{}", Match: PathMatch.Exact, Value: null, Reason: "oracle: ships nothing; top-level hostSslCertificates ships"), + new(OverrideKind.SuppressOperation, Method: null, "/security/threatintelligence/hosts/{}/trackers", Match: PathMatch.Exact, Value: null, Reason: "oracle: ships nothing; top-level hostTrackers ships"), + new(OverrideKind.SuppressOperation, Method: null, "/security/threatintelligence/hosts/{}/trackers/{}", Match: PathMatch.Exact, Value: null, Reason: "oracle: ships nothing; top-level hostTrackers ships"), + // The attackSimulation container node itself ships nothing; children under a + // simulation item ship nothing either (the list/item pair then merges normally). + new(OverrideKind.SuppressOperation, Method: null, "/security/attacksimulation", Match: PathMatch.Exact, Value: null, + Reason: "oracle: the container node ships nothing; its child collections ship"), + new(OverrideKind.SuppressOperation, Method: null, "/security/attacksimulation/simulations/{}/", Match: PathMatch.Prefix, Value: null, + Reason: "oracle: nothing under a simulation item ships in v1.0"), + + // Calendar: the shipped default-calendar surface. Events under a NAMED calendar and + // the default-calendar event item tree ship nothing; event items ship from + // /users/{}/events (Get-MgUserEvent family). + new(OverrideKind.ReplaceNoun, Method: null, "/users/{}/calendar/events", Match: PathMatch.Exact, Value: "UserDefaultCalendarEvent", + Reason: "oracle: list/create ship as Get/New-MgUserDefaultCalendarEvent"), + new(OverrideKind.SuppressOperation, Method: null, "/users/{}/calendar/events/{}", Match: PathMatch.Prefix, Value: null, + Reason: "oracle: default-calendar event items ship nothing; items ship from /users/{}/events/{}"), + new(OverrideKind.SuppressOperation, Method: null, "/users/{}/calendars/{}/events/{}", Match: PathMatch.Prefix, Value: null, + Reason: "oracle: named-calendar event items ship nothing; items ship from /users/{}/events/{}"), + new(OverrideKind.SuppressOperation, Method: null, "/users/{}/calendars/{}/calendarpermissions", Match: PathMatch.Prefix, Value: null, + Reason: "oracle: permissions ship only from the default calendar (Get-MgUserCalendarPermission on /users/{}/calendar/calendarPermissions)"), + + // Teams Info-wrapper navs: the wrapped single-entity navigation ships nothing under + // any root. The suffix matches just the nav node, so shipped siblings + // (…SharedWithTeamAllowedMember) are unaffected. + new(OverrideKind.SuppressOperation, Method: null, "/pinnedmessages/{}/message", Match: PathMatch.Suffix, Value: null, + Reason: "oracle: ships nothing under any root; list side ships (Get-MgChatPinnedMessage)"), + new(OverrideKind.SuppressOperation, Method: null, "/sharedwithteams/{}/team", Match: PathMatch.Suffix, Value: null, + Reason: "oracle: ships nothing; sibling /allowedMembers ships, so node-only"), + new(OverrideKind.SuppressOperation, Method: null, "/associatedteams/{}/team", Match: PathMatch.Suffix, Value: null, + Reason: "oracle: ships nothing; list side ships (Get-MgUserTeamworkAssociatedTeam)"), + + // Groups: the nested lifecycle-policies GET ships renamed; everything else on that + // route ships from the top-level set. Photos items ship nothing (singleton /photo). + new(OverrideKind.ReplaceNoun, HttpMethod.Get, "/groups/{}/grouplifecyclepolicies", Match: PathMatch.Exact, Value: "GroupLifecyclePolicyByGroup", + Reason: "Groups.md directive (subject $1ByGroup); oracle ships Get-MgGroupLifecyclePolicyByGroup"), + new(OverrideKind.SuppressOperation, Method: null, "/groups/{}/grouplifecyclepolicies/", Match: PathMatch.Prefix, Value: null, + Reason: "oracle: item/children under the nested route ship nothing; the set ships top-level"), + new(OverrideKind.SuppressOperation, Method: null, "/groups/{}/photos/{}", Match: PathMatch.Exact, Value: null, + Reason: "oracle: photos items ship nothing; shipped surface is the /photo singleton (Get-MgGroupPhoto)"), + + // Small-module resolutions. + new(OverrideKind.SuppressOperation, Method: null, "/solutions/virtualevents", Match: PathMatch.Exact, Value: null, + Reason: "oracle: the virtualEvents root node ships nothing; children ship (Get-MgVirtualEventWebinar)"), + new(OverrideKind.SuppressOperation, Method: null, "/replies/{}/replyto", Match: PathMatch.Suffix, Value: null, + Reason: "oracle: the replyTo nav ships nothing under any root"), + new(OverrideKind.SuppressOperation, Method: null, "/deviceappmanagement/mobileapps/{}/categories", Match: PathMatch.Prefix, Value: null, + Reason: "oracle: nested app categories ship nothing; the set ships top-level (mobileAppCategories)"), + new(OverrideKind.SuppressOperation, Method: null, "/education/classes/{}/assignments/{}/categories", Match: PathMatch.Exact, Value: null, + Reason: "oracle: the plain path ships nothing; the shipped surface is the $ref route"), + new(OverrideKind.SuppressOperation, Method: null, "/education/users/{}/user", Match: PathMatch.Exact, Value: null, + Reason: "oracle: the user self-nav node ships nothing; its children (mailboxSettings) ship"), + new(OverrideKind.ReplaceNoun, HttpMethod.Get, "/groups/{}/drive", Match: PathMatch.Exact, Value: "GroupDefaultDrive", + Reason: "Files.md directive (subject $1Default$2); oracle ships Get-MgGroupDefaultDrive"), + new(OverrideKind.ReplaceNoun, HttpMethod.Get, "/users/{}/drive", Match: PathMatch.Exact, Value: "UserDefaultDrive", + Reason: "Files.md directive (subject $1Default$2); oracle ships Get-MgUserDefaultDrive"), + new(OverrideKind.ReplaceNoun, HttpMethod.Get, "/sites/{}/drive", Match: PathMatch.Exact, Value: "SiteDefaultDrive", + Reason: "oracle ships Get-MgSiteDefaultDrive for the site default-drive singleton"), + new(OverrideKind.ReplaceNoun, HttpMethod.Get, "/groups/{}/sites/{}/drive", Match: PathMatch.Exact, Value: "GroupSiteDefaultDrive", + Reason: "oracle ships Get-MgGroupSiteDefaultDrive"), + new(OverrideKind.SuppressOperation, HttpMethod.Get, "/shares/{}/list/items/{}", Match: PathMatch.Exact, Value: null, + Reason: "oracle: the bare shared-list item GET ships nothing; its descendants ship"), + new(OverrideKind.SuppressOperation, Method: null, "/identityproviders", Match: PathMatch.Prefix, Value: null, + Reason: "oracle: the deprecated top-level /identityProviders set ships nothing in v1.0; shipped surface is /identity/identityProviders (Get-MgIdentityProvider)"), ]; [GeneratedRegex(@"\{[^}]*\}")] @@ -107,8 +260,11 @@ private static bool Matches(Entry entry, HttpMethod httpMethod, string normalize // HttpMethod's own equality is case-insensitive, so no string comparison is needed. if (entry.Method is not null && entry.Method != httpMethod) return false; - return entry.ExactPath - ? string.Equals(normalizedPath, entry.PathPrefix, StringComparison.Ordinal) - : normalizedPath.StartsWith(entry.PathPrefix, StringComparison.Ordinal); + return entry.Match switch + { + PathMatch.Exact => string.Equals(normalizedPath, entry.Pattern, StringComparison.Ordinal), + PathMatch.Prefix => normalizedPath.StartsWith(entry.Pattern, StringComparison.Ordinal), + _ => normalizedPath.EndsWith(entry.Pattern, StringComparison.Ordinal), + }; } } diff --git a/tools/WrapperGenerator/PowerShellWrapperGenerationService.cs b/tools/WrapperGenerator/PowerShellWrapperGenerationService.cs index f339190253..b0b1edd923 100644 --- a/tools/WrapperGenerator/PowerShellWrapperGenerationService.cs +++ b/tools/WrapperGenerator/PowerShellWrapperGenerationService.cs @@ -20,6 +20,12 @@ public sealed partial class PowerShellWrapperGenerationService private readonly GeneratorConfig config; private readonly ILogger logger; private readonly HashSet modelSubNamespaces; + + // Every file written this run, keyed case-insensitively (Windows file systems are), so a + // second cmdlet resolving to an existing file is a detected collision instead of a silent + // overwrite. + private readonly Dictionary writtenCmdletFiles = new(StringComparer.OrdinalIgnoreCase); + private readonly List fileCollisions = []; private readonly Dictionary kiotaReservedRenames; public PowerShellWrapperGenerationService(OpenApiDocument document, GeneratorConfig configuration, ILogger logger) @@ -96,6 +102,9 @@ public async Task GenerateAsync(CancellationToken cancellationToken) var ctx = new EmitContext(ClientNamespace: config.ClientNamespaceName); + writtenCmdletFiles.Clear(); + fileCollisions.Clear(); + Directory.CreateDirectory(config.OutputPath); foreach (var stale in Directory.GetFiles(config.OutputPath, "*.g.cs")) File.Delete(stale); @@ -198,6 +207,15 @@ public async Task GenerateAsync(CancellationToken cancellationToken) written += await EmitGetOperationsAsync(getOperations, ctx, cancellationToken).ConfigureAwait(false); + // All collisions for the run are reported together so one generation surfaces the + // complete list; see edge-cases/naming-edge-cases.md for how each kind is resolved. + if (fileCollisions.Count > 0) + { + throw new InvalidOperationException( + $"{fileCollisions.Count} cmdlet name collision(s): a later operation would overwrite an already-written cmdlet file. " + + $"Resolve each with a NamingOverrides rename or suppression.\n " + string.Join("\n ", fileCollisions)); + } + LogWroteFiles(written + 1, config.OutputPath); } @@ -208,9 +226,9 @@ public async Task GenerateAsync(CancellationToken cancellationToken) // which one to invoke. // // A pairing is only trusted when it is structurally unambiguous: exactly one collection GET - // and one single-entity GET share the noun, and the item's path is the list's path plus one - // trailing id (Users[UserId].Messages -> Users[UserId].Messages[MessageId]). Everything - // else keeps the standalone shape: singleton navs with no list (GET /users/{id}/calendar), + // and one single-entity GET share the noun, and the item's path extends the list's path by + // exactly one id, in either of the shapes Naming.IsListItemPair accepts. Everything else + // keeps the standalone shape: singleton navs with no list (GET /users/{id}/calendar), // list-only endpoints such as delta queries, or an unexpected same-noun collision. private async Task EmitGetOperationsAsync(List getOperations, EmitContext ctx, CancellationToken cancellationToken) { @@ -298,6 +316,15 @@ private async Task EmitGetOperationsAsync(List getOpera private async Task WriteCmdletFileAsync(CmdletNaming naming, string source, CancellationToken cancellationToken) { var fileName = naming.ClassName.Replace("Command", "", StringComparison.Ordinal) + ".g.cs"; + // Both colliding cmdlets usually share the same name, so the builder expression (the + // request path) is what actually identifies which two operations collided. + var cmdletName = $"{naming.VerbName}-{naming.Noun} [{naming.BuilderExpression}]"; + if (writtenCmdletFiles.TryGetValue(fileName, out var existing)) + { + fileCollisions.Add($"{fileName}: '{cmdletName}' collides with already-written '{existing}'"); + return 0; + } + writtenCmdletFiles[fileName] = cmdletName; await File.WriteAllTextAsync(Path.Combine(config.OutputPath, fileName), source, cancellationToken).ConfigureAwait(false); LogWroteCmdletFile(fileName, naming.VerbName, naming.Noun); return 1; diff --git a/tools/WrapperGenerator/README.md b/tools/WrapperGenerator/README.md index caab1d5e93..eaf37248be 100644 --- a/tools/WrapperGenerator/README.md +++ b/tools/WrapperGenerator/README.md @@ -52,7 +52,7 @@ Singularization runs per camel-case word (so `termsAndConditions` → `TermAndCo | ends in `ss`/`us`/`is` stays | `Access`, `Status`, `Analysis` | | trailing `s` drops | `Messages` → `Message` | -A few published names aren't algorithmic — they come from hand-written directives in the SDK's module configs. Those live as data in `NamingOverrides.cs`, each with a cited source, rather than as special cases in the naming code. There are three today: suppress `PATCH /users/{id}/calendar` (the SDK ships no such cmdlet), rename `GET /users/{id}/calendar` to `…UserDefaultCalendar`, and strip the `Solution` prefix for most `/solutions/*` nouns (for example, `Get-MgBookingBusiness`, not `Get-MgSolutionBookingBusiness`) while preserving it for known exceptions such as BackupRestore (`Get-MgSolutionBackupRestore`). +A few published names aren't algorithmic, and the spec publishes some routes the SDK never shipped. Both live as data in `NamingOverrides.cs` — renames mirroring the SDK's hand-written AutoRest directives, and suppressions for routes that ship nothing — each entry citing its evidence: the directive when one exists, otherwise the shipped-command inventory. Examples: the `GET /users/{id}/calendar` rename to `…UserDefaultCalendar` (Calendar.md), the `Solution` prefix strip under `/solutions/*` with the BackupRestore exception (Bookings.md), and the self-referential `sites/{id}/sites` rename to `SubSite`/`GroupSubSite` (Sites.md) — without which the sub-sites cmdlets would collide with `Get-MgSite` itself. The generator fails loudly on any such file collision rather than silently overwriting. ## The one subtle part: list + item GET become one cmdlet @@ -126,7 +126,7 @@ The wrappers compile and run only alongside step 1's output. Wiring the two into | `PowerShellWrapperGenerationService.cs` | The orchestrator: walks the paths, pairs list/item GETs, writes the files | | `CmdletNaming.cs` | Verb + noun + the `client.X[Y].Z` request chain | | `Singularizer.cs` | The per-word singularization rules | -| `NamingOverrides.cs` | The three hand-cited name exceptions | +| `NamingOverrides.cs` | Cited rename/suppression data mirroring the shipped SDK surface | | `CmdletEmitter.cs` | The C# templates for each cmdlet shape (the actual code text) | | `SchemaProperties.cs` | Which body properties become `New`/`Update` parameters | | `OperationInfo.cs`, `EmitContext.cs`, `GeneratorConfig.cs` | Small data/config carriers | @@ -157,7 +157,7 @@ dotnet run --project tools/WrapperGenerator -- ` ```powershell # 1. Naming rules pinned to published Microsoft.Graph names dotnet test tools/WrapperGenerator.Tests -# => Passed! - Failed: 0, Passed: 103, Total: 103 +# => Passed! - Failed: 0, Passed: 115, Total: 115 # 2. Parity gate: generate, then check every cmdlet name against Graph's own command inventory .\tools\Compare-WrapperCmdletNames.ps1 -GeneratedPath diff --git a/tools/WrapperGenerator/edge-cases/naming-edge-cases.md b/tools/WrapperGenerator/edge-cases/naming-edge-cases.md index e5d541e562..309c64f5e8 100644 --- a/tools/WrapperGenerator/edge-cases/naming-edge-cases.md +++ b/tools/WrapperGenerator/edge-cases/naming-edge-cases.md @@ -34,7 +34,7 @@ Entry template (keep the field names exact so the file converts cleanly): ``` ## - **Class:** -- **Status:** +- **Status:** — optionally followed by a short parenthetical qualifier - **Evidence:** - **Decision:** - **Migration impact:** @@ -50,6 +50,8 @@ Entry template (keep the field names exact so the file converts cleanly): | operationId preposition truncation | operationid-truncation | structurally-avoided | | `SkypeForBusiness` subject truncation | operationid-truncation | not-yet-reachable | | `Cookies`/`Skus`/`Dns`/`Ios`/`Statistics` quirks | inflection-defect | reproduced-for-parity | +| Self-referential `sites/{id}/sites` → `SubSite` | adjacent-duplicate-segments | handled | +| Route duplicates (spec paths the SDK never shipped) | duplicate-routes | partially-handled | ## Whois truncated to Whoi on the host navigation @@ -143,6 +145,45 @@ Entry template (keep the field names exact so the file converts cleanly): `data`, `delta`, `quota` (Humanizer-specific mistakes this rule engine never makes) and `statistics` (the one that applied here). +## Self-referential paths collide with their parent's cmdlet + +- **Class:** adjacent-duplicate-segments +- **Status:** handled (loud failure + directive-cited renames) +- **Evidence:** singularizing a self-referencing path collapses it onto its parent's noun: + `/sites/{id}/sites` produced `GetMgSite.g.cs`, silently overwriting the get-site-by-id + cmdlet — the same silent-drop failure AutoRest had. Nothing could detect it: writes are + not logged at console level, the summary counts surviving files, and the parity gate only + inspects files that exist. +- **Decision:** the generator now fails generation loudly on any cmdlet file collision, + listing every colliding pair. Shipped cases are renamed via NamingOverrides with their + directive cited (`sites/{id}/sites` → `SubSite`/`GroupSubSite`, per Sites.md + `subject: SubSite` directives); paths the SDK ships nothing for are suppressed as they + surface. +- **Migration impact:** none — renames match the published names exactly. +- **References:** issue #3704; `NamingOverrides.cs` SubSite entries; Sites.md lines 32–61. + +## Route duplicates: the spec publishes paths the SDK never shipped + +- **Class:** duplicate-routes +- **Status:** partially-handled (oracle-cited suppressions/renames) +- **Evidence:** the collision guard's first full-inventory sweep found 966 silent collisions + (per-module counts on issue #3704). Beyond self-references, the dominant cause is the spec + publishing the same data + under two routes while the SDK ships exactly one: nested navs duplicating top-level sets + (`hosts/{id}/components` vs `hostComponents` — 14 Security paths, ships nothing nested), + default-singleton vs collection (`/users/{id}/drive` ships renamed `UserDefaultDrive`; + `/users/{id}/calendar/events` ships `UserDefaultCalendarEvent`), Info-wrapper navs that + never shipped (`pinnedMessages/{id}/message`), and stitched pairs where GET ships from one + route and PATCH/DELETE from the other (termStore, agreement file/files). +- **Decision:** each resolved family is a `NamingOverrides` entry citing the shipped + command or the oracle's absence. Two families remain open on #3704 with full evidence: + the Identity.Governance mirrored navigations (the shipped survivor alternates by nesting + level, needing a dedupe design decision) and the Sites termStore `children` recursion + (resolver and direct oracle probes disagree; needs reconciliation before encoding). +- **Migration impact:** none — suppressed routes never shipped; renames match shipped names. +- **References:** issue #3704 (remainder inventory + resolver evidence); `NamingOverrides.cs` + "Collision resolutions" section. + ## Watch list Cases spotted but deliberately not acted on yet, so they aren't lost: