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

Implement CreateHttpManagementPayload API in Durable Worker Extension #2929

Merged
merged 15 commits into from
Oct 23, 2024
Merged
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ public string DurableOrchestrationClientToString(IDurableOrchestrationClient cli
ConnectionName = attr.ConnectionName,
RpcBaseUrl = localRpcAddress,
RequiredQueryStringParameters = this.config.HttpApiHandler.GetUniversalQueryStrings(),
BaseUrl = this.config.HttpApiHandler.GetBaseUrl(),
nytian marked this conversation as resolved.
Show resolved Hide resolved
});
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ public ValueTask<ConversionResult> ConvertAsync(ConverterContext context)
}

DurableTaskClient client = this.clientProvider.GetClient(endpoint, inputData?.taskHubName, inputData?.connectionName);
client = new FunctionsDurableTaskClient(client, inputData!.requiredQueryStringParameters);
client = new FunctionsDurableTaskClient(client, inputData!.requiredQueryStringParameters, inputData!.baseUrl);
return new ValueTask<ConversionResult>(ConversionResult.Success(client));
}
catch (Exception innerException)
Expand All @@ -62,5 +62,5 @@ public ValueTask<ConversionResult> ConvertAsync(ConverterContext context)
}

// Serializer is case-sensitive and incoming JSON properties are camel-cased.
private record DurableClientInputData(string rpcBaseUrl, string taskHubName, string connectionName, string requiredQueryStringParameters);
private record DurableClientInputData(string rpcBaseUrl, string taskHubName, string connectionName, string requiredQueryStringParameters, string baseUrl);
}
63 changes: 58 additions & 5 deletions src/Worker.Extensions.DurableTask/DurableTaskClientExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -120,8 +120,37 @@ public static HttpResponseData CreateCheckStatusResponse(
return response;
}

/// <summary>
/// Creates an HTTP management payload for the specified orchestration instance.
/// </summary>
/// <param name="client">The <see cref="DurableTaskClient"/>.</param>
/// <param name="instanceId">The ID of the orchestration instance.</param>
/// <param name="request">Optional HTTP request data to use for creating the base URL.</param>
/// <returns>An object containing instance control URLs.</returns>
/// <exception cref="ArgumentException">Thrown when instanceId is null or empty.</exception>
/// <exception cref="InvalidOperationException">Thrown when a valid base URL cannot be determined.</exception>
public static object CreateHttpManagementPayload(
nytian marked this conversation as resolved.
Show resolved Hide resolved
this DurableTaskClient client,
string instanceId,
HttpRequestData? request = null)
{
if (string.IsNullOrEmpty(instanceId))
{
throw new ArgumentException("InstanceId cannot be null or empty.", nameof(instanceId));
}

try
{
return SetHeadersAndGetPayload(client, request, null, instanceId);
}
catch (InvalidOperationException ex)
{
throw new InvalidOperationException("Failed to create HTTP management payload. " + ex.Message, ex);
}
nytian marked this conversation as resolved.
Show resolved Hide resolved
}

private static object SetHeadersAndGetPayload(
DurableTaskClient client, HttpRequestData request, HttpResponseData response, string instanceId)
DurableTaskClient client, HttpRequestData? request, HttpResponseData? response, string instanceId)
{
static string BuildUrl(string url, params string?[] queryValues)
{
Expand All @@ -143,12 +172,31 @@ static string BuildUrl(string url, params string?[] queryValues)
// request headers into consideration and generate the base URL accordingly.
// More info: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Forwarded.
// One potential workaround is to set ASPNETCORE_FORWARDEDHEADERS_ENABLED to true.
string baseUrl = request.Url.GetLeftPart(UriPartial.Authority);

// If HttpRequestData is provided, use its URL; otherwise, get the baseUrl from the DurableTaskClient.
// The base URL could be null if:
// 1. The DurableTaskClient isn't a FunctionsDurableTaskClient (which would have the baseUrl from bindings)
// 2. There's no valid HttpRequestData provided
string? baseUrl = ((request != null) ? request.Url.GetLeftPart(UriPartial.Authority) : GetBaseUrl(client))
?? throw new InvalidOperationException("Base URL is null. Either use Functions bindings or provide an HTTP request to create the HttpPayload.");
nytian marked this conversation as resolved.
Show resolved Hide resolved
bool isFromRequest = request != null;

string formattedInstanceId = Uri.EscapeDataString(instanceId);
string instanceUrl = $"{baseUrl}/runtime/webhooks/durabletask/instances/{formattedInstanceId}";

// The baseUrl differs depending on the source. Eg:
// - From request: http://localhost:7071/
nytian marked this conversation as resolved.
Show resolved Hide resolved
// - From durable client: http://localhost:7071/runtime/webhooks/durabletask
// We adjust the instanceUrl construction accordingly.
string instanceUrl = isFromRequest
? $"{baseUrl}/runtime/webhooks/durabletask/instances/{formattedInstanceId}"
: $"{baseUrl}/instances/{formattedInstanceId}";
string? commonQueryParameters = GetQueryParams(client);
response.Headers.Add("Location", BuildUrl(instanceUrl, commonQueryParameters));
response.Headers.Add("Content-Type", "application/json");

if (response != null)
{
response.Headers.Add("Location", BuildUrl(instanceUrl, commonQueryParameters));
response.Headers.Add("Content-Type", "application/json");
}
Comment on lines -150 to +197
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is response possibly null now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SetHeadersAndGetPayload is also invoked by the existing method CreateCheckStatusResponse, which provides an HTTP response to SetHeadersAndGetPayload. In the new API, CreateHttpManagementPayload, there won’t be an HTTP response parameter. Therefore, when SetHeadersAndGetPayload is called by CreateHttpManagementPayload, the response will be null; however, if it’s called by CreateCheckStatusResponse, the response will not be null.


return new
{
Expand All @@ -172,4 +220,9 @@ private static ObjectSerializer GetObjectSerializer(HttpResponseData response)
{
return client is FunctionsDurableTaskClient functions ? functions.QueryString : null;
}

private static string? GetBaseUrl(DurableTaskClient client)
{
return client is FunctionsDurableTaskClient functions ? functions.BaseUrl : null;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,16 @@ internal sealed class FunctionsDurableTaskClient : DurableTaskClient
{
private readonly DurableTaskClient inner;

public FunctionsDurableTaskClient(DurableTaskClient inner, string? queryString)
public FunctionsDurableTaskClient(DurableTaskClient inner, string? queryString, string? baseUrl)
: base(inner.Name)
{
this.inner = inner;
this.QueryString = queryString;
this.BaseUrl = baseUrl;
}

public string? QueryString { get; }

public string? BaseUrl { get; }
public override DurableEntityClient Entities => this.inner.Entities;

public override ValueTask DisposeAsync()
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
using System.Security.Claims;
using Microsoft.Azure.Functions.Worker.Http;
using Microsoft.DurableTask.Client;
using Microsoft.DurableTask.Client.Grpc;
using Moq;
Expand All @@ -9,7 +11,7 @@
/// </summary>
public class FunctionsDurableTaskClientTests
{
private FunctionsDurableTaskClient GetTestFunctionsDurableTaskClient()
private FunctionsDurableTaskClient GetTestFunctionsDurableTaskClient(string? baseUrl = null)
{
// construct mock client

Expand All @@ -22,7 +24,7 @@
It.IsAny<string>(), It.IsAny<TerminateInstanceOptions>(), It.IsAny<CancellationToken>())).Returns(completedTask);

DurableTaskClient durableClient = durableClientMock.Object;
FunctionsDurableTaskClient client = new FunctionsDurableTaskClient(durableClient, queryString: null);
FunctionsDurableTaskClient client = new FunctionsDurableTaskClient(durableClient, queryString: null, baseUrl: baseUrl);
return client;
}

Expand Down Expand Up @@ -53,5 +55,93 @@
await client.TerminateInstanceAsync(instanceId, options);
await client.TerminateInstanceAsync(instanceId, options, token);
}

/// <summary>
/// Test that the `CreateHttpManagementPayload` method returns the expected payload structure without HttpRequestData.
/// </summary>
[Fact]
public void CreateHttpManagementPayload_WithBaseUrl()
{
string BaseUrl = "http://localhost:7071/runtime/webhooks/durabletask";
nytian marked this conversation as resolved.
Show resolved Hide resolved
FunctionsDurableTaskClient client = this.GetTestFunctionsDurableTaskClient(BaseUrl);
string instanceId = "testInstanceIdWithHostBaseUrl";

dynamic payload = client.CreateHttpManagementPayload(instanceId);

AssertHttpManagementPayload(payload, BaseUrl, instanceId);
}

/// <summary>
/// Test that the `CreateHttpManagementPayload` method returns the expected payload structure with HttpRequestData.
/// </summary>
[Fact]
public void CreateHttpManagementPayload_WithHttpRequestData()
{
string requestUrl = "http://localhost:7075/orchestrators/E1_HelloSequence";
FunctionsDurableTaskClient client = this.GetTestFunctionsDurableTaskClient();
string instanceId = "testInstanceIdWithRequest";
var context = new TestFunctionContext();
var request = new TestHttpRequestData(context, new Uri(requestUrl));

dynamic payload = client.CreateHttpManagementPayload(instanceId, request);

AssertHttpManagementPayload(payload, "http://localhost:7075/runtime/webhooks/durabletask", instanceId);
}

private static void AssertHttpManagementPayload(dynamic payload, string BaseUrl, string instanceId)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we replace dynamic with HttpManagementPayload here, and in the calling functions?

{
Assert.Equal(instanceId, payload.id);
Assert.Equal($"{BaseUrl}/instances/{instanceId}", payload.purgeHistoryDeleteUri);
Assert.Equal($"{BaseUrl}/instances/{instanceId}/raiseEvent/{{eventName}}", payload.sendEventPostUri);
Assert.Equal($"{BaseUrl}/instances/{instanceId}", payload.statusQueryGetUri);
Assert.Equal($"{BaseUrl}/instances/{instanceId}/terminate?reason={{{{text}}}}", payload.terminatePostUri);
Assert.Equal($"{BaseUrl}/instances/{instanceId}/suspend?reason={{{{text}}}}", payload.suspendPostUri);
Assert.Equal($"{BaseUrl}/instances/{instanceId}/resume?reason={{{{text}}}}", payload.resumePostUri);
}
}

/// <summary>
/// A minimal implementation of FunctionContext for testing purposes.
/// It's used to create a TestHttpRequestData instance, which requires a FunctionContext.
/// </summary>
public class TestFunctionContext : FunctionContext
Copy link
Collaborator Author

@nytian nytian Oct 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’ve added these classes to simulate HttpResponseData for testing the CreateHttpManagementPayload method with a HttpResponseData instance, specifically for the CreateHttpManagementPayload_WithHttpRequestData() test. Let me know if you think this approach is overly complex or if there’s a more efficient way to handle this testing. I’m open to removing these classes if needed.:)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the manual way to do it, which is fine. A way to do it with less code would be to use Moq (which can generate these kinds of fakes dynamically), but no need to change what you've already done.

{
public override string InvocationId => throw new NotImplementedException();
public override string FunctionId => throw new NotImplementedException();
public override TraceContext TraceContext => throw new NotImplementedException();
public override BindingContext BindingContext => throw new NotImplementedException();
public override RetryContext RetryContext => throw new NotImplementedException();
public override IServiceProvider InstanceServices { get; set; } = new EmptyServiceProvider();
public override FunctionDefinition FunctionDefinition => throw new NotImplementedException();
public override IDictionary<object, object> Items { get; set; } = new Dictionary<object, object>();
public override IInvocationFeatures Features => throw new NotImplementedException();
}

/// <summary>
/// A minimal implementation of IServiceProvider for testing purposes.
/// </summary>
public class EmptyServiceProvider : IServiceProvider
{
public object GetService(Type serviceType) => null;

Check warning on line 125 in test/Worker.Extensions.DurableTask.Tests/FunctionsDurableTaskClientTests.cs

View workflow job for this annotation

GitHub Actions / build

Possible null reference return.

Check warning on line 125 in test/Worker.Extensions.DurableTask.Tests/FunctionsDurableTaskClientTests.cs

View workflow job for this annotation

GitHub Actions / build

Possible null reference return.

Check warning on line 125 in test/Worker.Extensions.DurableTask.Tests/FunctionsDurableTaskClientTests.cs

View workflow job for this annotation

GitHub Actions / build

Possible null reference return.

Check warning on line 125 in test/Worker.Extensions.DurableTask.Tests/FunctionsDurableTaskClientTests.cs

View workflow job for this annotation

GitHub Actions / build

Possible null reference return.

Check warning on line 125 in test/Worker.Extensions.DurableTask.Tests/FunctionsDurableTaskClientTests.cs

View workflow job for this annotation

GitHub Actions / Analyze (csharp)

Possible null reference return.

Check warning on line 125 in test/Worker.Extensions.DurableTask.Tests/FunctionsDurableTaskClientTests.cs

View workflow job for this annotation

GitHub Actions / build

Possible null reference return.
nytian marked this conversation as resolved.
Show resolved Hide resolved
}

// <summary>
/// A test implementation of HttpRequestData used for unit testing.
/// This class allows us to create instances of HttpRequestData, which is normally abstract.
/// </summary>
public class TestHttpRequestData : HttpRequestData
{
public TestHttpRequestData(FunctionContext functionContext, Uri url) : base(functionContext)
{
Url = url;
}

public override Stream Body => throw new NotImplementedException();
public override HttpHeadersCollection Headers => throw new NotImplementedException();
public override IReadOnlyCollection<IHttpCookie> Cookies => throw new NotImplementedException();
public override Uri Url { get; }
public override IEnumerable<ClaimsIdentity> Identities => throw new NotImplementedException();
public override string Method => throw new NotImplementedException();
public override HttpResponseData CreateResponse() => throw new NotImplementedException();
}
}
Loading