Skip to content
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

[FEATURE REQ] KeyVaultKeyIdentifier.TryParse #23146

Closed
jimmyca15 opened this issue Aug 6, 2021 · 6 comments · Fixed by #23419
Closed

[FEATURE REQ] KeyVaultKeyIdentifier.TryParse #23146

jimmyca15 opened this issue Aug 6, 2021 · 6 comments · Fixed by #23419
Assignees
Labels
Client This issue points to a problem in the data-plane of the library. KeyVault needs-team-attention This issue needs attention from Azure service team or SDK team

Comments

@jimmyca15
Copy link
Member

Library or service name.
Azure.Security.KeyVault.Keys

Is your feature request related to a problem? Please describe.
If I develop an application that takes user input that should be a key vault key URI I have to write the following code to validate it is a Key Vault URI.

try
{
    identifier = new KeyVaultKeyIdentifier(id);
}
catch (ArgumentException)
{
    // Invalid id
}

Ideally I would like to write

if (KeyVaultKeyIdentifier.TryParse(id, out KeyVaultKeyIdentifier identifier))
{
    // use it
}
else
{
    // Return error
}
@ghost ghost added the needs-triage This is a new issue that needs to be triaged to the appropriate team. label Aug 6, 2021
@jsquire jsquire added Client This issue points to a problem in the data-plane of the library. KeyVault needs-team-attention This issue needs attention from Azure service team or SDK team labels Aug 6, 2021
@ghost ghost removed the needs-triage This is a new issue that needs to be triaged to the appropriate team. label Aug 6, 2021
@jsquire
Copy link
Member

jsquire commented Aug 6, 2021

Thank you for your feedback. Tagging and routing to the team member best able to assist.

@heaths
Copy link
Member

heaths commented Aug 6, 2021

@KrzysztofCwalina customers here are asking for my original design. Do you see any problem adding this? The KeyVault*Identifier structs across languages were only ever meant for parsing anyway - merely exposing what we were all already doing internally anyway.

@avanigupta
Copy link
Member

Hi @heaths, any update on whether this API will be added to the next stable release?

@KrzysztofCwalina
Copy link
Member

KrzysztofCwalina commented Aug 19, 2021

I am not totally opposed to digging deeper here and possibly solving a problem, but:

It's not really parsing. The URI is already parsed when it's passed to the ctor. The issue seems to be that some users (e.g. @jimmyca15) do not like exceptions for invalid arguments. I totally understand issues with C# exception handling (I am myself not a big fan), and I talked with the C# team many times about it, but don't think TryParse was intended for that. In .NET, we throw exceptions for invalid arguments.

.NET guidelines specifically state that Try methods should not be added because we don't like exceptions. Try methods are performance optimizations, and so if you add them you need to show that performance is a problem (not in microbenchmarks).

If we started to add Try methods to all places where we don't like exceptions, we would end up with every BCL API duplicated (try and non-try variants).

@MadsTorgersen, @jaredpar, if would be nice to have try expressions in the language, just like we planned in M# :-)

see "int value1 = try Foo() else 42;" @ http://joeduffyblog.com/2016/02/07/the-error-model/

@heaths
Copy link
Member

heaths commented Aug 19, 2021

The URI is not parsed. It is a URI, yes, but not necessarily a valid Key Vault URI. That's why I had KeyVaultSecretIdentifier.TryParse originally. As @tg-msft pointed out from the .NET Framework Guidelines,

DO NOT use exceptions for the normal flow of control, if possible. Prefer Tester–Doer or the Try pattern to throwing exceptions in common cases.

The point of this struct was to parse a URI as a valid Key Vault (or Managed HSM) URI. I agree that we need a TryParse.

@heaths
Copy link
Member

heaths commented Aug 19, 2021

We will add a TryCreate in the next preview (and stable) release. We feel this will better coincide with the existing ability to create (via the constructor) a KeyVaultKeyIdentifier and be less confusing since the URI is already parsed into a System.Uri.

heaths added a commit to heaths/azure-sdk-for-net that referenced this issue Aug 19, 2021
heaths added a commit that referenced this issue Aug 20, 2021
* Add TryCreate to Identifier structures

Fixes #23146

* Resolve feedback
azure-sdk pushed a commit to azure-sdk/azure-sdk-for-net that referenced this issue Mar 21, 2023
Edit description of properties.provisioningState (Azure#23146)
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Client This issue points to a problem in the data-plane of the library. KeyVault needs-team-attention This issue needs attention from Azure service team or SDK team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants
@jsquire @heaths @avanigupta @KrzysztofCwalina @jimmyca15 and others