GetCertificateVersionAsync throws an Argument Exceptions for the Version parameter

[MohamedSherif]

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]

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

[pallavit]

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

[heaths]

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]

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.

azure-sdk-for-net/sdk/keyvault/Azure.Security.KeyVault.Certificates/src/CertificateClient.cs

Line 441 in 2ba5db3

Argument.AssertNotNullOrEmpty(version, nameof(version));

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.

azure-sdk-for-net/sdk/keyvault/Azure.Security.KeyVault.Certificates/src/CertificateClient.cs

Lines 409 to 427 in 2ba5db3

	public virtual Response<KeyVaultCertificate> GetCertificateVersion(string certificateName, string version, CancellationToken cancellationToken = default)
	{
	Argument.AssertNotNullOrEmpty(certificateName, nameof(certificateName));
	using DiagnosticScope scope = _pipeline.CreateScope($"{nameof(CertificateClient)}.{nameof(GetCertificateVersion)}");
	scope.AddAttribute(OTelCertificateNameKey, certificateName);
	scope.AddAttribute(OTelCertificateVersionKey, version);
	scope.Start();
	try
	{
	return _pipeline.SendRequest(RequestMethod.Get, () => new KeyVaultCertificate(), cancellationToken, CertificatesPath, certificateName, "/", version);
	}
	catch (Exception e)
	{
	scope.Failed(e);
	throw;
	}
	}