search

Azure / azure-sdk-for-net

This repository is for active development of the Azure SDK for .NET. For consumers of the SDK we recommend visiting our public developer docs at https://learn.microsoft.com/dotnet/azure/ or our versioned developer docs at https://azure.github.io/azure-sdk-for-net.
MIT License
5.47k stars 4.8k forks source link

GetCertificateVersionAsync throws an Argument Exceptions for the Version parameter #43441

Open MohamedSherif opened 6 months ago

MohamedSherif commented 6 months ago

Library name and version

Azure.Security.KeyVault.Certificates 4.6.0

Describe the bug

The async method GetCertificateVersionAsync throws Argument Exception if the version parameter is empty and throws ArgumentNullException if the version parameter is null. This is not the expected behavior according to the documentation given to this method. This caused service failure on our side. We used to call the method in the old deprecated library with string.Empty value for the version parameter to get The root certificate.

Expected behavior

It is expected that the method accepts null or empty value for the version parameter. It should throw the exceptions only in the case of the certificateName is null or empty.

Actual behavior

The method throws exception when we pass string.Empty as the value of version parameter.

Reproduction Steps

certificateClient.GetCertificateVersionAsync(certificateName, string.Empty, cancellationToken);

Environment

No response

github-actions[bot] commented 6 months ago

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

pallavit commented 6 months ago

@nisha-bhatia please take a look when you get a chance.

heaths commented 6 months ago

The exceptions are consistent with .NET itself. string.Empty != null. All .NET methods taking strings are the same, as are all our other current SDKs. If the method documentation is wrong, that should be updated, but the behavior is correct.

Note that the version parameter is optional in the modern SDKs and doesn't even need to be specified. null is already the default.

MohamedSherif commented 6 months ago

Thanks @heaths for your reply.

I think this line is the line that causes this exception to be thrown in the async version of this method. If the version parameter is optional, it should NOT be asserted for Not null or empty. https://github.com/Azure/azure-sdk-for-net/blob/2ba5db394f2d52a490f3e8fb58991dd27f94f3c8/sdk/keyvault/Azure.Security.KeyVault.Certificates/src/CertificateClient.cs#L441

I am comparing with the non async version of this method and it doesn't have this line. that's why I think the documentation is correct and the behavior need to be changed. https://github.com/Azure/azure-sdk-for-net/blob/2ba5db394f2d52a490f3e8fb58991dd27f94f3c8/sdk/keyvault/Azure.Security.KeyVault.Certificates/src/CertificateClient.cs#L409-L427