Skip to content

refactor: extract pure resolver methods for subscription resolution #2395

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.

Sign up for GitHub

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

Closed
jongio wants to merge 2 commits into from
Closed

refactor: extract pure resolver methods for subscription resolution #2395

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1b0fc5f
Fix subscription ID tests when logged in with Azure CLI
alzimmermsft Apr 13, 2026
e726e7b
refactor: extract pure resolver methods for subscription resolution
jongio Apr 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 17 additions & 9 deletions ...re.Mcp.Core/tests/Azure.Mcp.Core.UnitTests/Areas/Subscription/SubscriptionCommandTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

namespace Azure.Mcp.Core.UnitTests.Areas.Subscription;

public class SubscriptionCommandTests
public class SubscriptionCommandTests : IDisposable
{
private readonly IServiceProvider _serviceProvider;
private readonly IStorageService _storageService;
Expand All @@ -38,6 +38,11 @@ public SubscriptionCommandTests()
_commandDefinition = _command.GetCommand();
}

public void Dispose()
{
EnvironmentHelpers.ClearAzureSubscriptionId();
}

[Fact]
public void Validate_WithEnvironmentVariableOnly_PassesValidation()
{
Expand All @@ -56,6 +61,7 @@ public async Task ExecuteAsync_WithEnvironmentVariableOnly_CallsServiceWithCorre
{
// Arrange
EnvironmentHelpers.SetAzureSubscriptionId("env-subs");
var expectedSubscription = CommandHelper.GetDefaultSubscription();

Comment on lines 46 to 65
Copy link

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These test names still refer to “environment variable only”, but the expected/verified subscription is now the resolved default (Azure CLI profile can override the env var). Rename the tests to reflect that they validate default-subscription resolution rather than specifically env-var behavior.

Copilot uses AI. Check for mistakes.
var expectedAccounts = new ResourceQueryResults<StorageAccountInfo>(
[
Expand All @@ -65,7 +71,7 @@ public async Task ExecuteAsync_WithEnvironmentVariableOnly_CallsServiceWithCorre

_storageService.GetAccountDetails(
Arg.Is<string?>(s => string.IsNullOrEmpty(s)),
Arg.Is("env-subs"),
Arg.Is(expectedSubscription!),
Arg.Any<string>(),
Arg.Any<RetryPolicyOptions>(),
Arg.Any<CancellationToken>())
Expand All @@ -79,10 +85,10 @@ public async Task ExecuteAsync_WithEnvironmentVariableOnly_CallsServiceWithCorre
// Assert
Assert.NotNull(response);

// Verify the service was called with the environment variable subscription
// Verify the service was called with the resolved default subscription
_ = _storageService.Received(1).GetAccountDetails(
Arg.Is<string?>(s => string.IsNullOrEmpty(s)),
"env-subs",
expectedSubscription!,
Arg.Any<string>(),
Arg.Any<RetryPolicyOptions>(),
Arg.Any<CancellationToken>());
Expand All @@ -93,6 +99,8 @@ public async Task ExecuteAsync_WithBothOptionAndEnvironmentVariable_PrefersOptio
{
// Arrange
EnvironmentHelpers.SetAzureSubscriptionId("env-subs");
var defaultSubscription = CommandHelper.GetDefaultSubscription();
var expectedSubscription = "option-subs";

var expectedAccounts = new ResourceQueryResults<StorageAccountInfo>(
[
Expand All @@ -102,30 +110,30 @@ public async Task ExecuteAsync_WithBothOptionAndEnvironmentVariable_PrefersOptio

_storageService.GetAccountDetails(
Arg.Is<string?>(s => string.IsNullOrEmpty(s)),
Arg.Is("option-subs"),
Arg.Is(expectedSubscription),
Arg.Any<string>(),
Arg.Any<RetryPolicyOptions>(),
Arg.Any<CancellationToken>())
.Returns(Task.FromResult(expectedAccounts));

var parseResult = _commandDefinition.Parse(["--subscription", "option-subs"]);
var parseResult = _commandDefinition.Parse(["--subscription", expectedSubscription]);

// Act
var response = await _command.ExecuteAsync(_context, parseResult, TestContext.Current.CancellationToken);

// Assert
Assert.NotNull(response);

// Verify the service was called with the option subscription, not the environment variable
// Verify the service was called with the option subscription, not the default
_ = _storageService.Received(1).GetAccountDetails(
Arg.Is<string?>(s => string.IsNullOrEmpty(s)),
"option-subs",
expectedSubscription,
Arg.Any<string>(),
Arg.Any<RetryPolicyOptions>(),
Arg.Any<CancellationToken>());
_ = _storageService.DidNotReceive().GetAccountDetails(
Arg.Is<string?>(s => string.IsNullOrEmpty(s)),
"env-subs",
defaultSubscription!,
Arg.Any<string>(),
Arg.Any<RetryPolicyOptions>(),
Arg.Any<CancellationToken>());
Expand Down
105 changes: 105 additions & 0 deletions core/Azure.Mcp.Core/tests/Azure.Mcp.Core.UnitTests/Helpers/CommandHelperResolverTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

using Microsoft.Mcp.Core.Helpers;
using Xunit;

namespace Azure.Mcp.Core.UnitTests.Helpers;

public class CommandHelperResolverTests
{
// --- ResolveDefaultSubscription tests ---

[Fact]
public void ResolveDefaultSubscription_ProfileTakesPriority()
{
var result = CommandHelper.ResolveDefaultSubscription("cli-sub", "env-sub");
Assert.Equal("cli-sub", result);
}

[Fact]
public void ResolveDefaultSubscription_FallsBackToEnv_WhenProfileNull()
{
var result = CommandHelper.ResolveDefaultSubscription(null, "env-sub");
Assert.Equal("env-sub", result);
}

[Fact]
public void ResolveDefaultSubscription_FallsBackToEnv_WhenProfileEmpty()
{
var result = CommandHelper.ResolveDefaultSubscription("", "env-sub");
Assert.Equal("env-sub", result);
}

[Fact]
public void ResolveDefaultSubscription_ReturnsNull_WhenBothNull()
{
var result = CommandHelper.ResolveDefaultSubscription(null, null);
Assert.Null(result);
}

[Fact]
public void ResolveDefaultSubscription_ReturnsEnv_WhenBothEmpty()
{
var result = CommandHelper.ResolveDefaultSubscription("", "");
Assert.Equal("", result);
Comment on lines +42 to +45
Copy link

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test ResolveDefaultSubscription_ReturnsEnv_WhenBothEmpty asserts an empty string result when both inputs are empty. That contradicts the production method’s doc (“first non-empty subscription source”) and makes it easier for empty env vars to be treated as a “resolved” value. Consider updating the resolver to treat empty as absent and changing this test to expect null.

Suggested change
public void ResolveDefaultSubscription_ReturnsEnv_WhenBothEmpty()
{
var result = CommandHelper.ResolveDefaultSubscription("", "");
Assert.Equal("", result);
public void ResolveDefaultSubscription_ReturnsNull_WhenBothEmpty()
{
var result = CommandHelper.ResolveDefaultSubscription("", "");
Assert.Null(result);

Copilot uses AI. Check for mistakes.
}

// --- ResolveSubscription tests ---

[Fact]
public void ResolveSubscription_ExplicitOptionWins()
{
var result = CommandHelper.ResolveSubscription("explicit-sub", "default-sub");
Assert.Equal("explicit-sub", result);
}

[Fact]
public void ResolveSubscription_FallsBackToDefault_WhenOptionNull()
{
var result = CommandHelper.ResolveSubscription(null, "default-sub");
Assert.Equal("default-sub", result);
}

[Fact]
public void ResolveSubscription_FallsBackToDefault_WhenOptionEmpty()
{
var result = CommandHelper.ResolveSubscription("", "default-sub");
Assert.Equal("default-sub", result);
}

[Fact]
public void ResolveSubscription_PlaceholderSubscription_FallsBackToDefault()
{
var result = CommandHelper.ResolveSubscription("Azure subscription 1", "default-sub");
Assert.Equal("default-sub", result);
}

[Fact]
public void ResolveSubscription_PlaceholderDefault_FallsBackToDefault()
{
var result = CommandHelper.ResolveSubscription("Some default name", "default-sub");
Assert.Equal("default-sub", result);
}

[Fact]
public void ResolveSubscription_PlaceholderWithNoDefault_ReturnsPlaceholder()
{
var result = CommandHelper.ResolveSubscription("Azure subscription 1", null);
Assert.Equal("Azure subscription 1", result);
}

[Fact]
public void ResolveSubscription_BothNull_ReturnsNull()
{
var result = CommandHelper.ResolveSubscription(null, null);
Assert.Null(result);
}

[Fact]
public void ResolveSubscription_EmptyOptionAndEmptyDefault_ReturnsEmpty()
{
var result = CommandHelper.ResolveSubscription("", "");
Assert.Equal("", result);
}
}
25 changes: 18 additions & 7 deletions core/Azure.Mcp.Core/tests/Azure.Mcp.Core.UnitTests/Helpers/CommandHelperTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,34 +7,41 @@

namespace Azure.Mcp.Core.UnitTests.Helpers;

public class CommandHelperTests
public class CommandHelperTests : IDisposable
{
public void Dispose()
{
EnvironmentHelpers.ClearAzureSubscriptionId();
}

[Fact]
public void GetSubscription_EmptySubscriptionParameter_ReturnsEnvironmentValue()
{
// Arrange
EnvironmentHelpers.SetAzureSubscriptionId("env-subs");
var expected = CommandHelper.GetDefaultSubscription();
var parseResult = GetParseResult(["--subscription", ""]);

// Act
var actual = CommandHelper.GetSubscription(parseResult);

// Assert
Assert.Equal("env-subs", actual);
Assert.Equal(expected, actual);
}

[Fact]
public void GetSubscription_MissingSubscriptionParameter_ReturnsEnvironmentValue()
{
// Arrange
EnvironmentHelpers.SetAzureSubscriptionId("env-subs");
var expected = CommandHelper.GetDefaultSubscription();
var parseResult = GetParseResult([]);

// Act
var actual = CommandHelper.GetSubscription(parseResult);

// Assert
Assert.Equal("env-subs", actual);
Assert.Equal(expected, actual);
Comment on lines 17 to +44
Copy link

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Several test names still claim they validate “environment value” behavior, but the assertions now intentionally accept Azure CLI profile precedence by comparing to GetDefaultSubscription(). Consider renaming these tests (or adding clarifying comments) so the name matches what’s actually being asserted.

Copilot uses AI. Check for mistakes.
}

[Fact]
Expand All @@ -56,53 +63,57 @@ public void GetSubscription_ParameterValueContainingSubscription_ReturnsEnvironm
{
// Arrange
EnvironmentHelpers.SetAzureSubscriptionId("env-subs");
var expected = CommandHelper.GetDefaultSubscription();
var parseResult = GetParseResult(["--subscription", "Azure subscription 1"]);

// Act
var actual = CommandHelper.GetSubscription(parseResult);

// Assert
Assert.Equal("env-subs", actual);
Assert.Equal(expected, actual);
}

[Fact]
public void GetSubscription_ParameterValueContainingDefault_ReturnsEnvironmentValue()
{
// Arrange
EnvironmentHelpers.SetAzureSubscriptionId("env-subs");
var expected = CommandHelper.GetDefaultSubscription();
var parseResult = GetParseResult(["--subscription", "Some default name"]);

// Act
var actual = CommandHelper.GetSubscription(parseResult);

// Assert
Assert.Equal("env-subs", actual);
Assert.Equal(expected, actual);
}

[Fact]
public void GetSubscription_NoEnvironmentVariableParameterValueContainingDefault_ReturnsParameterValue()
{
// Arrange
var subscription = CommandHelper.GetProfileSubscription();
var parseResult = GetParseResult(["--subscription", "Some default name"]);

// Act
var actual = CommandHelper.GetSubscription(parseResult);

// Assert
Assert.Equal("Some default name", actual);
Assert.Equal(subscription ?? "Some default name", actual);
}

[Fact]
public void GetSubscription_NoEnvironmentVariableParameterValueContainingSubscription_ReturnsParameterValue()
{
// Arrange
var subscription = CommandHelper.GetProfileSubscription();
var parseResult = GetParseResult(["--subscription", "Azure subscription 1"]);

// Act
var actual = CommandHelper.GetSubscription(parseResult);

// Assert
Assert.Equal("Azure subscription 1", actual);
Assert.Equal(subscription ?? "Azure subscription 1", actual);
}

private static ParseResult GetParseResult(params string[] args)
Expand Down
39 changes: 22 additions & 17 deletions core/Microsoft.Mcp.Core/src/Helpers/CommandHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,18 +30,8 @@ public static bool HasSubscriptionAvailable(CommandResult commandResult)

public static string? GetSubscription(ParseResult parseResult)
{
// Get subscription from command line option or fallback to default subscription
var subscriptionValue = parseResult.GetValueOrDefault<string>(OptionDefinitions.Common.Subscription.Name);

if (!string.IsNullOrEmpty(subscriptionValue) && !IsPlaceholder(subscriptionValue))
{
return subscriptionValue;
}

var defaultSubscription = GetDefaultSubscription();
return !string.IsNullOrEmpty(defaultSubscription)
? defaultSubscription
: subscriptionValue;
return ResolveSubscription(subscriptionValue, GetDefaultSubscription());
}

/// <summary>
Expand All @@ -50,16 +40,31 @@ public static bool HasSubscriptionAvailable(CommandResult commandResult)
/// The CLI profile read is cached for the lifetime of the process to avoid redundant file I/O.
/// </summary>
public static string? GetDefaultSubscription()
=> ResolveDefaultSubscription(GetProfileSubscription(), EnvironmentHelpers.GetAzureSubscriptionId());

internal static string? GetProfileSubscription() => s_profileDefault.Value;

/// <summary>
/// Pure resolution logic: returns the first non-empty subscription source.
/// Priority: Azure CLI profile > AZURE_SUBSCRIPTION_ID environment variable.
/// </summary>
internal static string? ResolveDefaultSubscription(string? profileSubscription, string? envSubscription)
=> !string.IsNullOrEmpty(profileSubscription) ? profileSubscription : envSubscription;
Copy link

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ResolveDefaultSubscription’s XML doc says it returns the first non-empty source, but the implementation returns envSubscription even when it’s an empty string (e.g., env var set to ""). Consider treating envSubscription as absent when null/empty (and returning null when both sources are null/empty), or adjust the doc/tests to match the intended behavior.

Suggested change
=> !string.IsNullOrEmpty(profileSubscription) ? profileSubscription : envSubscription;
=> !string.IsNullOrEmpty(profileSubscription)
? profileSubscription
: !string.IsNullOrEmpty(envSubscription)
? envSubscription
: null;

Copilot uses AI. Check for mistakes.

/// <summary>
/// Pure resolution logic: returns the explicit option value if valid, otherwise the default.
/// Placeholder values (containing "subscription" or "default") are treated as absent.
/// </summary>
internal static string? ResolveSubscription(string? optionValue, string? defaultSubscription)
{
// Primary: Azure CLI profile (set via 'az account set') - cached to avoid repeated file I/O
var profileDefault = s_profileDefault.Value;
if (!string.IsNullOrEmpty(profileDefault))
if (!string.IsNullOrEmpty(optionValue) && !IsPlaceholder(optionValue))
{
return profileDefault;
return optionValue;
}

// Fallback: AZURE_SUBSCRIPTION_ID environment variable (cheap, not cached)
return EnvironmentHelpers.GetAzureSubscriptionId();
return !string.IsNullOrEmpty(defaultSubscription)
? defaultSubscription
: optionValue;
Comment on lines +54 to +67
Copy link

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ResolveSubscription’s XML doc says placeholder values are treated as absent, but the method currently returns the placeholder when defaultSubscription is null/empty (to preserve the old behavior). Either update the documentation to reflect this fallback, or change the behavior so placeholders are never returned.

Copilot uses AI. Check for mistakes.
}

private static bool IsPlaceholder(string value) => value.Contains("subscription") || value.Contains("default");
Copy link

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IsPlaceholder uses case-sensitive substring matching. This will miss placeholder text like "Azure Subscription 1" or "Default" depending on casing, which can cause unintended validation behavior. Use a StringComparison (e.g., OrdinalIgnoreCase) when checking for placeholder tokens.

Suggested change
private static bool IsPlaceholder(string value) => value.Contains("subscription") || value.Contains("default");
private static bool IsPlaceholder(string value) =>
value.Contains("subscription", StringComparison.OrdinalIgnoreCase) ||
value.Contains("default", StringComparison.OrdinalIgnoreCase);

Copilot uses AI. Check for mistakes.
Expand Down
Loading
Loading