-
Notifications
You must be signed in to change notification settings - Fork 537
Fix case-sensitive tenant ID comparison and introduce StringComparisons helper #2907
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
base: main
Are you sure you want to change the base?
Changes from 6 commits
8408838
fad1a06
45befbf
ff0f00b
9df5c54
738e697
073ff50
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,44 @@ | ||
| // Copyright (c) Microsoft Corporation. | ||
| // Licensed under the MIT License. | ||
|
|
||
| namespace Microsoft.Mcp.Core.Helpers; | ||
|
|
||
| /// <summary> | ||
| /// Centralizes the <see cref="StringComparison"/> values to use when comparing | ||
| /// well-known string values, making the intended comparison semantics explicit | ||
| /// at each call site. | ||
| /// </summary> | ||
| public static class StringComparisons | ||
| { | ||
| /// <summary> | ||
| /// The comparison to use when comparing Azure tenant IDs. Tenant IDs are | ||
| /// GUIDs whose casing is not significant, so they are compared | ||
| /// case-insensitively. | ||
| /// </summary> | ||
| public static StringComparison TenantId => StringComparison.OrdinalIgnoreCase; | ||
|
|
||
| /// <summary> | ||
| /// The comparison to use when comparing Azure subscription IDs. Subscription | ||
| /// IDs are GUIDs whose casing is not significant, so they are compared | ||
| /// case-insensitively. | ||
| /// </summary> | ||
| public static StringComparison SubscriptionId => StringComparison.OrdinalIgnoreCase; | ||
|
|
||
| /// <summary> | ||
| /// The comparison to use when comparing Azure subscription display names, | ||
| /// which are matched case-insensitively. | ||
| /// </summary> | ||
| public static StringComparison SubscriptionDisplayName => StringComparison.OrdinalIgnoreCase; | ||
|
|
||
| /// <summary> | ||
| /// The comparison to use when comparing Azure resource group names, which | ||
| /// Azure Resource Manager treats case-insensitively. | ||
| /// </summary> | ||
| public static StringComparison ResourceGroup => StringComparison.OrdinalIgnoreCase; | ||
|
|
||
| /// <summary> | ||
| /// The comparison to use when matching an Azure resource by its name. Azure | ||
| /// resource names are matched case-insensitively for lookup purposes. | ||
| /// </summary> | ||
| public static StringComparison ResourceName => StringComparison.OrdinalIgnoreCase; | ||
|
Contributor
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. Are Azure resources always a case-insensitive comparison? This is the only one that gives me concern as it's a broad category.
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 may be doing case insensitive lookup/queries but I don't think we can assume two names differ only in case refer to the same resource. https://learn.microsoft.com/en-us/azure/azure-resource-manager/management/resource-name-rules |
||
| } | ||
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.
The point of this type is to make it clear at the call site (
string.Equals(...), etc.) that the expected comparison is being used. Also, if we're using the wrong comparison we can change it here to update it for all consumers.I didn't move all usages of
StringComparison.OrdinalorStringComparison.OrdinalIgnoreCaseto this file in order to keep the size of the change manageable. More can be added in a future update.