Skip to content

Commit

Permalink
Resolve several Key Vault backlog issues (#25593)
Browse files Browse the repository at this point in the history 
* Use operation helpers

Partially fixes #22912

* Resolve PR feedback

* Add OperationInternal tests

* Use OperationInternal for all pseudo-LROs

Resolves #22912

* Use KeyVaultPipeline for SecretClient

Resolves #8115

* Deduplicate identifier parsing

Resolves #24262

* Resolve PR feedback
  • Loading branch information
@heaths
heaths authored Dec 2, 2021
1 parent 8ba684a commit 71054d5
Show file tree
Hide file tree
Showing 19 changed files with 303 additions and 412 deletions.
9 changes: 9 additions & 0 deletions sdk/core/Azure.Core/src/Shared/OperationInternal.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,15 @@ public OperationInternal(
_operation = operation;
}

/// <summary>
/// Sets the <see cref="OperationInternal"/> state immediately.
/// </summary>
/// <param name="state">The <see cref="OperationState"/> used to set <see cref="OperationInternalBase.HasCompleted"/> and other members.</param>
public void SetState(OperationState state)
{
ApplyStateAsync(false, state.RawResponse, state.HasCompleted, state.HasSucceeded, state.OperationFailedException, throwIfFailed: false).EnsureCompleted();
}

protected override async ValueTask<Response> UpdateStateAsync(bool async, CancellationToken cancellationToken)
{
OperationState state = await _operation.UpdateStateAsync(async, cancellationToken).ConfigureAwait(false);
Expand Down
10 changes: 8 additions & 2 deletions sdk/core/Azure.Core/src/Shared/OperationInternalBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ private async ValueTask<Response> UpdateStatusAsync(bool async, CancellationToke
}
}

protected async ValueTask<Response> ApplyStateAsync(bool async, Response response, bool hasCompleted, bool hasSucceeded, RequestFailedException? requestFailedException)
protected async ValueTask<Response> ApplyStateAsync(bool async, Response response, bool hasCompleted, bool hasSucceeded, RequestFailedException? requestFailedException, bool throwIfFailed = true)
{
RawResponse = response;

Expand All @@ -198,7 +198,13 @@ protected async ValueTask<Response> ApplyStateAsync(bool async, Response respons
(async
? await _diagnostics.CreateRequestFailedExceptionAsync(response).ConfigureAwait(false)
: _diagnostics.CreateRequestFailedException(response));
throw OperationFailedException;

if (throwIfFailed)
{
throw OperationFailedException;
}

return response;
}

protected static TimeSpan GetServerDelay(Response response, TimeSpan pollingInterval)
Expand Down
13 changes: 13 additions & 0 deletions sdk/core/Azure.Core/src/Shared/OperationInternalOfT.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,19 @@ public async ValueTask<Response<T>> WaitForCompletionAsync(TimeSpan pollingInter
return Response.FromValue(Value, rawResponse);
}

/// <summary>
/// Sets the <see cref="OperationInternal{T}"/> state immediately.
/// </summary>
/// <param name="state">The <see cref="OperationState{T}"/> used to set <see cref="OperationInternalBase.HasCompleted"/> and other members.</param>
public void SetState(OperationState<T> state)
{
if (state.HasCompleted && state.HasSucceeded)
{
Value = state.Value!;
}
ApplyStateAsync(false, state.RawResponse, state.HasCompleted, state.HasSucceeded, state.OperationFailedException, throwIfFailed: false).EnsureCompleted();
}

protected override async ValueTask<Response> UpdateStateAsync(bool async, CancellationToken cancellationToken)
{
OperationState<T> state = await _operation.UpdateStateAsync(async, cancellationToken).ConfigureAwait(false);
Expand Down
62 changes: 62 additions & 0 deletions sdk/core/Azure.Core/tests/OperationInternalTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,68 @@ public void RawResponseInitialization()
}
}

[Test]
public void SetStateSucceeds()
{
var operationInternal = CreateOperation(isOfT, UpdateResult.Pending);
if (operationInternal is OperationInternal oi)
{
oi.SetState(OperationState.Success(mockResponse));
}
else if (operationInternal is OperationInternal<int> oit)
{
oit.SetState(OperationState<int>.Success(mockResponse, 1));
}

Assert.IsTrue(operationInternal.HasCompleted);
if (operationInternal is OperationInternal<int> oit2)
{
Assert.IsTrue(oit2.HasValue);
Assert.AreEqual(1, oit2.Value);
}
}

[Test]
public void SetStateIsPending()
{
var operationInternal = CreateOperation(isOfT, UpdateResult.Pending);
if (operationInternal is OperationInternal oi)
{
oi.SetState(OperationState.Pending(mockResponse));
}
else if (operationInternal is OperationInternal<int> oit)
{
oit.SetState(OperationState<int>.Pending(mockResponse));
}

Assert.IsFalse(operationInternal.HasCompleted);
if (operationInternal is OperationInternal<int> oit2)
{
Assert.IsFalse(oit2.HasValue);
}
}

[Test]
public void SetStateFails()
{
var operationInternal = CreateOperation(isOfT, UpdateResult.Pending);
if (operationInternal is OperationInternal oi)
{
oi.SetState(OperationState.Failure(mockResponse));
}
else if (operationInternal is OperationInternal<int> oit)
{
oit.SetState(OperationState<int>.Failure(mockResponse));
}

Assert.IsTrue(operationInternal.HasCompleted);
if (operationInternal is OperationInternal<int> oit2)
{
Assert.IsFalse(oit2.HasValue);
Assert.Throws<RequestFailedException>(() => _ = oit2.Value);
}
}

[Test]
public async Task UpdateStatusWhenOperationIsPending([Values(true, false)] bool async)
{
Expand Down
2 changes: 2 additions & 0 deletions ...ault/Azure.Security.KeyVault.Certificates/src/Azure.Security.KeyVault.Certificates.csproj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@
<Compile Include="$(AzureCoreSharedSources)ContentTypeUtilities.cs" LinkBase="Shared" />
<Compile Include="$(AzureCoreSharedSources)DiagnosticScope.cs" LinkBase="Shared" />
<Compile Include="$(AzureCoreSharedSources)OperationHelpers.cs" LinkBase="Shared" />
<Compile Include="$(AzureCoreSharedSources)OperationInternal.cs" LinkBase="Shared" />
<Compile Include="$(AzureCoreSharedSources)OperationInternalBase.cs" LinkBase="Shared" />
<Compile Include="$(AzureCoreSharedSources)PageResponseEnumerator.cs" LinkBase="Shared" />
<Compile Include="$(AzureCoreSharedSources)DiagnosticScopeFactory.cs" LinkBase="Shared" />
<Compile Include="$(AzureCoreSharedSources)TaskExtensions.cs" LinkBase="Shared" />
Expand Down
16 changes: 6 additions & 10 deletions sdk/keyvault/Azure.Security.KeyVault.Certificates/src/CertificateProperties.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -158,18 +158,14 @@ internal void ReadProperty(JsonProperty prop)
}
}

private void ParseId(Uri idToParse)
private void ParseId(Uri id)
{
// We expect an identifier with either 3 or 4 segments: host + collection + name [+ version]
if (idToParse.Segments.Length != 3 && idToParse.Segments.Length != 4)
throw new ArgumentException(string.Format(CultureInfo.InvariantCulture, "Invalid ObjectIdentifier: {0}. Bad number of segments: {1}", idToParse, idToParse.Segments.Length));
KeyVaultIdentifier identifier = KeyVaultIdentifier.ParseWithCollection(id, "certificates");

if (!string.Equals(idToParse.Segments[1], "certificates" + "/", StringComparison.OrdinalIgnoreCase))
throw new ArgumentException(string.Format(CultureInfo.InvariantCulture, "Invalid ObjectIdentifier: {0}. segment [1] should be 'certificates/', found '{1}'", idToParse, idToParse.Segments[1]));

VaultUri = new Uri($"{idToParse.Scheme}://{idToParse.Authority}");
Name = idToParse.Segments[2].Trim('/');
Version = (idToParse.Segments.Length == 4) ? idToParse.Segments[3].TrimEnd('/') : null;
Id = id;
VaultUri = identifier.VaultUri;
Name = identifier.Name;
Version = identifier.Version;
}
}
}
85 changes: 28 additions & 57 deletions sdk/keyvault/Azure.Security.KeyVault.Certificates/src/DeleteCertificateOperation.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,35 +2,39 @@
// Licensed under the MIT License.

using System;
using System.Collections.Generic;
using System.Threading;
using System.Threading.Tasks;
using Azure.Core;
using Azure.Core.Pipeline;

namespace Azure.Security.KeyVault.Certificates
{
/// <summary>
/// A long-running operation for <see cref="CertificateClient.StartDeleteCertificate(string, CancellationToken)"/> or <see cref="CertificateClient.StartDeleteCertificateAsync(string, CancellationToken)"/>.
/// </summary>
public class DeleteCertificateOperation : Operation<DeletedCertificate>
public class DeleteCertificateOperation : Operation<DeletedCertificate>, IOperation
{
private static readonly TimeSpan s_defaultPollingInterval = TimeSpan.FromSeconds(2);

private readonly KeyVaultPipeline _pipeline;
private readonly OperationInternal _operationInternal;
private readonly DeletedCertificate _value;
private Response _response;
private bool _completed;

internal DeleteCertificateOperation(KeyVaultPipeline pipeline, Response<DeletedCertificate> response)
{
_pipeline = pipeline;
_value = response.Value ?? throw new InvalidOperationException("The response does not contain a value.");
_response = response.GetRawResponse();
_operationInternal = new(_pipeline.Diagnostics, this, response.GetRawResponse(), nameof(DeleteCertificateOperation), new[]
{
new KeyValuePair<string, string>("secret", _value.Name), // Retained for backward compatibility.
new KeyValuePair<string, string>("certificate", _value.Name),
});

// The recoveryId is only returned if soft-delete is enabled.
// The recoveryId is only returned if soft delete is enabled.
if (_value.RecoveryId is null)
{
_completed = true;
// If soft delete is not enabled, deleting is immediate so set success accordingly.
_operationInternal.SetState(OperationState.Success(response.GetRawResponse()));
}
}

Expand All @@ -50,33 +54,20 @@ protected DeleteCertificateOperation() {}
public override DeletedCertificate Value => _value;

/// <inheritdoc/>
public override bool HasCompleted => _completed;
public override bool HasCompleted => _operationInternal.HasCompleted;

/// <inheritdoc/>
public override bool HasValue => true;

/// <inheritdoc/>
public override Response GetRawResponse() => _response;
public override Response GetRawResponse() => _operationInternal.RawResponse;

/// <inheritdoc/>
public override Response UpdateStatus(CancellationToken cancellationToken = default)
{
if (!_completed)
if (!HasCompleted)
{
using DiagnosticScope scope = _pipeline.CreateScope($"{nameof(DeleteCertificateOperation)}.{nameof(UpdateStatus)}");
scope.AddAttribute("secret", _value.Name);
scope.Start();

try
{
_response = _pipeline.GetResponse(RequestMethod.Get, cancellationToken, CertificateClient.DeletedCertificatesPath, _value.Name);
_completed = CheckCompleted(_response);
}
catch (Exception e)
{
scope.Failed(e);
throw;
}
return _operationInternal.UpdateStatus(cancellationToken);
}

return GetRawResponse();
Expand All @@ -85,22 +76,9 @@ public override Response UpdateStatus(CancellationToken cancellationToken = defa
/// <inheritdoc/>
public override async ValueTask<Response> UpdateStatusAsync(CancellationToken cancellationToken = default)
{
if (!_completed)
if (!HasCompleted)
{
using DiagnosticScope scope = _pipeline.CreateScope($"{nameof(DeleteCertificateOperation)}.{nameof(UpdateStatus)}");
scope.AddAttribute("secret", _value.Name);
scope.Start();

try
{
_response = await _pipeline.GetResponseAsync(RequestMethod.Get, cancellationToken, CertificateClient.DeletedCertificatesPath, _value.Name).ConfigureAwait(false);
_completed = await CheckCompletedAsync(_response).ConfigureAwait(false);
}
catch (Exception e)
{
scope.Failed(e);
throw;
}
return await _operationInternal.UpdateStatusAsync(cancellationToken).ConfigureAwait(false);
}

return GetRawResponse();
Expand All @@ -114,34 +92,27 @@ public override ValueTask<Response<DeletedCertificate>> WaitForCompletionAsync(C
public override ValueTask<Response<DeletedCertificate>> WaitForCompletionAsync(TimeSpan pollingInterval, CancellationToken cancellationToken) =>
this.DefaultWaitForCompletionAsync(pollingInterval, cancellationToken);

private async ValueTask<bool> CheckCompletedAsync(Response response)
async ValueTask<OperationState> IOperation.UpdateStateAsync(bool async, CancellationToken cancellationToken)
{
switch (response.Status)
{
case 200:
case 403: // Access denied but proof the certificate was deleted.
return true;

case 404:
return false;
Response response = async
? await _pipeline.GetResponseAsync(RequestMethod.Get, cancellationToken, CertificateClient.DeletedCertificatesPath, _value.Name).ConfigureAwait(false)
: _pipeline.GetResponse(RequestMethod.Get, cancellationToken, CertificateClient.DeletedCertificatesPath, _value.Name);

default:
throw await _pipeline.Diagnostics.CreateRequestFailedExceptionAsync(response).ConfigureAwait(false);
}
}
private bool CheckCompleted(Response response)
{
switch (response.Status)
{
case 200:
case 403: // Access denied but proof the certificate was deleted.
return true;
return OperationState.Success(response);

case 404:
return false;
return OperationState.Pending(response);

default:
throw _pipeline.Diagnostics.CreateRequestFailedException(response);
RequestFailedException ex = async
? await _pipeline.Diagnostics.CreateRequestFailedExceptionAsync(response).ConfigureAwait(false)
: _pipeline.Diagnostics.CreateRequestFailedException(response);

return OperationState.Failure(response, ex);
}
}
}
Expand Down
Loading

0 comments on commit 71054d5

Please sign in to comment.