Small PoC on simplifying command and parse result handling#2261
Small PoC on simplifying command and parse result handling#2261alzimmermsft wants to merge 1 commit intomicrosoft:mainfrom
Conversation
github-actions
bot
added
tools-Compute
tools-Functions
tools-LoadTesting
tools-Monitor
labels
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
Design direction is right - routing through option.Name to find the registered instance cleanly fixes the AsRequired/AsOptional identity mismatch. A few things to address across the surface:
TryGetValue<T>(CommandResult, Option<T>, out T?) (CommandResultExtensions.cs line 58) still calls commandResult.GetResult(option) directly. It hasn't been migrated to name-based routing and has the same instance-mismatch problem as the methods you've fixed.
This also needs at least one test demonstrating the fix works: register an option via AsRequired() on the command, then call GetValueOrDefault() with the original static definition. That's the scenario this PR exists for - show it works.
| // Custom validation: At least one update property must be specified | ||
| if (string.IsNullOrEmpty(commandResult.GetValueOrDefault<string>(ComputeOptionDefinitions.UpgradePolicy.Name)) && | ||
| !commandResult.GetValueOrDefault<int?>(ComputeOptionDefinitions.Capacity.Name).HasValue && | ||
| if (string.IsNullOrEmpty(commandResult.GetValueOrDefault(ComputeOptionDefinitions.UpgradePolicy)) && |
There was a problem hiding this comment.
Only the first 2 conditions were migrated to the new pattern. Lines 69-73 (VmSize, Overprovision, EnableAutoOsUpgrade, ScaleInPolicy, Tags) still use .Name. Same gap in BindOptions (lines 84-91). Since this PoC demonstrates the new API, migrating the whole method makes the before/after comparison cleaner.
| if (string.IsNullOrEmpty(commandResult.GetValueOrDefault<string>(ComputeOptionDefinitions.UpgradePolicy.Name)) && | ||
| !commandResult.GetValueOrDefault<int?>(ComputeOptionDefinitions.Capacity.Name).HasValue && | ||
| if (string.IsNullOrEmpty(commandResult.GetValueOrDefault(ComputeOptionDefinitions.UpgradePolicy)) && | ||
| !commandResult.HasOptionResult(ComputeOptionDefinitions.Capacity) && |
There was a problem hiding this comment.
Capacity is Option<int> but the old code queried <int?> - FindOptionTByName<int?> can't match Option<int>, so this check always evaluated as "capacity not provided" even when it was. Switching to HasOptionResult fixes this pre-existing bug. Worth calling out in the PR description.
Also note the semantic shift: HasOptionResult checks token presence while the old code checked .HasValue on the returned nullable. For Option<int> without a default these are equivalent, but be intentional about which semantic you want.
| options.SslCheck = parseResult.CommandResult.GetValueWithoutDefault<bool?>(MonitorOptionDefinitions.WebTest.SslCheck.Name); | ||
| options.SslLifetimeCheckInDays = parseResult.CommandResult.GetValueWithoutDefault<int?>(MonitorOptionDefinitions.WebTest.SslLifetimeCheckInDays.Name); | ||
| options.TimeoutInSeconds = parseResult.CommandResult.GetValueWithoutDefault<int?>(MonitorOptionDefinitions.WebTest.TimeoutInSeconds.Name); | ||
| options.AppInsightsComponentId = parseResult.CommandResult.GetValueWithoutDefault(MonitorOptionDefinitions.WebTest.AppInsightsComponentId); |
There was a problem hiding this comment.
ResourceName (line 102) and ResourceGroup (line 103) still use the old .Name pattern while everything below was migrated.
| return GetValueOrDefaultImpl(commandResult, option); | ||
| } | ||
|
|
||
| private static T? GetValueOrDefaultImpl<T>(this CommandResult commandResult, Option<T> option) |
There was a problem hiding this comment.
this on a private method is a no-op since it can't be discovered as an extension externally. Same applies to GetValueWithoutDefaultImpl. Drop the this modifier to make the intent clear.
2 participants
What does this PR do?
Proof of concept for simplifying how
CommandResultandParseResultis handled withOptionandOption<T>.This changes the need to use
Option.Nameto retrieve the value when using the extension methods, instead the extension method handles using the name of theOption.Nameto handle everything properly.Option.Nameis used for lookup as we have extension methods forOptionswhich changes their required or optional behavior, but this creates a new instance which changes how lookup is handled byCommandResultandParseResultas it's just looking up against a dictionary, and since the instance is different this could fail.GitHub issue number?
[Link to the GitHub issue this PR addresses]Pre-merge Checklist
servers/Azure.Mcp.Server/README.mdand/orservers/Fabric.Mcp.Server/README.mddocumentationREADME.mdchanges running the script./eng/scripts/Process-PackageReadMe.ps1. See Package READMEToolDescriptionEvaluatorand obtained a score of0.4or more and a top 3 ranking for all related test promptsconsolidated-tools.jsonbreaking-changelabelservers/Azure.Mcp.Server/docs/azmcp-commands.md./eng/scripts/Update-AzCommandsMetadata.ps1to update tool metadata inazmcp-commands.md(required for CI)servers/Azure.Mcp.Server/docs/e2eTestPrompts.mdcrypto mining, spam, data exfiltration, etc.)/azp run mcp - pullrequest - liveto run Live Test Pipeline