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

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/WebJobs.Extensions.DurableTask/Bindings/BindingHelper.cs
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(),
HttpBaseUrl = this.config.HttpApiHandler.GetBaseUrl(),
});
}

Expand Down Expand Up @@ -130,6 +131,14 @@ private class OrchestrationClientInputData
/// </summary>
[JsonProperty("rpcBaseUrl")]
public string? RpcBaseUrl { get; set; }

/// <summary>
/// The base URL of the Azure Functions host, used in the out-of-proc model.
/// This URL is sent by the client binding object to the Durable Worker extension,
/// allowing the extension to know the host's base URL for constructing complete URLs.
nytian marked this conversation as resolved.
Show resolved Hide resolved
/// </summary>
[JsonProperty("HttpBaseUrl")]
public string? HttpBaseUrl { get; set; }
}
}
}

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

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!.httpBaseUrl);
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 httpBaseUrl);
}
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(
Copy link
Contributor

Choose a reason for hiding this comment

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

Quick nit - I noticed this returns object which is very general. In the WebJobs extension, we used to return HttpManagementPayload as the type, see here:

HttpManagementPayload IDurableOrchestrationClient.CreateHttpManagementPayload(string instanceId)

Should we not also introduced a worker extension version of this type, so that we can return a more precise return type?

Copy link
Contributor

Choose a reason for hiding this comment

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

cc/ @cgillum, to confirm. Curious if it makes sense to return object for this particular API type, not sure if there's something I'm missing

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, that's a good point. It made sense to use object and anonymous types in the case where we were serializing the object into the HTTP response. However, for the purposes of this new, more general purpose API, it's better to create a new HttpManagementPayload class so that users of this method have a strongly-typed object they can work with. Thanks @davidmrdavid for bringing this to my attention.

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);
}
Comment on lines +146 to +149
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of catching and re-throwing the InvalidOperationException. Why don't we just throw a single exception inside of SetHeadersAndGetPayload and let that exception propagate out of this method? We may need to move the "Failed to create HTTP management payload" message to "SetHeadersAndGetPayload", but that seems 'ok' to me.

}

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.");
Comment on lines +180 to +181
Copy link
Contributor

Choose a reason for hiding this comment

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

this is nitpicky, but I feel this is a bit difficult to read due the combination of ternary expressions the <condition> ? <true-case> : <false-case> syntax with the nullable syntax <value> ?? <code-to-execute-if-value-is-null>.

I just feel that there's a lot happening in very little space :) . I would using either the ternary expression, or the nullable syntax, but not both at the same time. The other can be expressed as a simple if-statement.

Not a blocking concern, just a stylistic nit. Feel free to dismiss.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haha I just used this so that I could save more space. I will change this to if-statement to make it more clear. Thanks!

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/
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand what it means to be "from request". Can you please clarify?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are two ways to obtain the BaseUrl: (1) from HttpRequestData, which refers to the request in this context. And this usually happen when cx already have a Http bindings, or (2) from the Functions host, if HttpRequestData is not provided, as this information is passed through the DurableClient bindings.

Let me try to update this comment to be more clear.

Eg, calling CreateHttpManagementPayload with HttpRequestData like this:

image

// - 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 +199
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
Contributor 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.HttpBaseUrl : 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? httpBaseUrl)
: base(inner.Name)
{
this.inner = inner;
this.QueryString = queryString;
this.HttpBaseUrl = httpBaseUrl;
}

public string? QueryString { get; }

public string? HttpBaseUrl { get; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it allowed for this string to ever be null? If not, I'd recommend removing the ? from the string type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the DurableTaskClient isn’t an instance of FunctionsDurableTaskClient, then HttpBaseUrl could be null, as it’s only an attribute of FunctionsDurableTaskClient. I’m not entirely sure in what scenario the DurableTaskClient wouldn’t be a FunctionsDurableTaskClient—perhaps it could happen if bindings aren’t being used. That’s why I added an exception for cases where the base URL creation fails.

public override DurableEntityClient Entities => this.inner.Entities;

public override ValueTask DisposeAsync()
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
using Microsoft.Azure.Functions.Worker.Http;
using Microsoft.DurableTask.Client;
using Microsoft.DurableTask.Client.Grpc;
using Moq;

namespace Microsoft.Azure.Functions.Worker.Tests
Expand All @@ -9,7 +9,7 @@ namespace Microsoft.Azure.Functions.Worker.Tests
/// </summary>
public class FunctionsDurableTaskClientTests
{
private FunctionsDurableTaskClient GetTestFunctionsDurableTaskClient()
private FunctionsDurableTaskClient GetTestFunctionsDurableTaskClient(string? baseUrl = null)
{
// construct mock client

Expand All @@ -22,7 +22,7 @@ private FunctionsDurableTaskClient GetTestFunctionsDurableTaskClient()
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, httpBaseUrl: baseUrl);
return client;
}

Expand Down Expand Up @@ -53,5 +53,51 @@ public async void TerminateDoesNotThrow()
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()
{
const string BaseUrl = "http://localhost:7071/runtime/webhooks/durabletask";
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()
{
const string requestUrl = "http://localhost:7075/orchestrators/E1_HelloSequence";
FunctionsDurableTaskClient client = this.GetTestFunctionsDurableTaskClient();
string instanceId = "testInstanceIdWithRequest";

// Create mock HttpRequestData object.
var mockFunctionContext = new Mock<FunctionContext>();
var mockHttpRequestData = new Mock<HttpRequestData>(mockFunctionContext.Object);
mockHttpRequestData.SetupGet(r => r.Url).Returns(new Uri(requestUrl));

dynamic payload = client.CreateHttpManagementPayload(instanceId, mockHttpRequestData.Object);

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

private static void AssertHttpManagementPayload(dynamic payload, string BaseUrl, string instanceId)
{
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);
}
}
}
Loading