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.

Sign up for GitHub

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

Jump to bottom

Implement CreateHttpManagementPayload API in Durable Worker Extension #2929

Merged
merged 15 commits into from
Oct 23, 2024
Merged

Implement CreateHttpManagementPayload API in Durable Worker Extension #2929

merged 15 commits into from
Oct 23, 2024

Conversation

nytian
Copy link
Collaborator

@nytian nytian commented Oct 7, 2024 •
edited
Loading

Issue describing the changes in this PR

Fix issue #2810

This PR adds the implementation of the CreateHttpManagementPayload API in the Durable Worker Extension. The implementation works as follows:

  • If an HttpRequestData object is provided, the HTTP management payload is created using the base URL derived from this request data.
  • If no HttpRequestData is available, the base URL is obtained from the Functions Host configuration, which is sent and converted by DurableTaskClient.

Pull request checklist

  • My changes do not require documentation changes
    • Otherwise: Documentation PR is ready to merge and referenced in pending_docs.md
  • My changes should not be added to the release notes for the next release
    • Otherwise: I've added my notes to release_notes.md
  • My changes do not need to be backported to a previous version
    • Otherwise: Backport tracked by issue/PR #issue_or_pr
  • I have added all required tests (Unit tests, E2E tests)
  • My changes do not require any extra work to be leveraged by OutOfProc SDKs
    • Otherwise: That work is being tracked here: #issue_or_pr_in_each_sdk
  • My changes do not change the version of the WebJobs.Extensions.DurableTask package
    • Otherwise: major or minor version updates are reflected in /src/Worker.Extensions.DurableTask/AssemblyInfo.cs
  • My changes do not add EventIds to our EventSource logs
    • Otherwise: Ensure the EventIds are within the supported range in our existing Windows infrastructure. You may validate this with a deployed app's telemetry. You may also extend the range by completing a PR such as this one.
  • My changes should be added to v3.x branch.
    • Otherwise: This change only applies to Durable Functions v2.x and will not be merged to branch v3.x.

nytian
nytian commented Oct 8, 2024
View reviewed changes
test/Worker.Extensions.DurableTask.Tests/FunctionsDurableTaskClientTests.cs Outdated
/// 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 •
edited
Loading

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.

cgillum
cgillum approved these changes Oct 8, 2024
View reviewed changes
Copy link
Member

@cgillum cgillum left a comment

Choose a reason for hiding this comment

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

LGTM! A few small suggestions.

src/WebJobs.Extensions.DurableTask/Bindings/BindingHelper.cs Outdated Show resolved Hide resolved
test/Worker.Extensions.DurableTask.Tests/FunctionsDurableTaskClientTests.cs Outdated Show resolved Hide resolved
test/Worker.Extensions.DurableTask.Tests/FunctionsDurableTaskClientTests.cs Outdated
/// 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
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.

test/Worker.Extensions.DurableTask.Tests/FunctionsDurableTaskClientTests.cs Outdated Show resolved Hide resolved
davidmrdavid
davidmrdavid reviewed Oct 9, 2024
View reviewed changes
Copy link
Contributor

@davidmrdavid davidmrdavid left a comment

Choose a reason for hiding this comment

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

A few questions, looks great. I appreciate the test!

src/WebJobs.Extensions.DurableTask/Bindings/BindingHelper.cs Outdated Show resolved Hide resolved
src/Worker.Extensions.DurableTask/FunctionsDurableTaskClient.cs
}

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
Collaborator 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.

src/Worker.Extensions.DurableTask/DurableTaskClientExtensions.cs Outdated Show resolved Hide resolved
src/Worker.Extensions.DurableTask/DurableTaskClientExtensions.cs
Comment on lines -150 to +199
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");
}
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.

src/Worker.Extensions.DurableTask/DurableTaskClientExtensions.cs Show resolved Hide resolved
src/Worker.Extensions.DurableTask/DurableTaskClientExtensions.cs Outdated Show resolved Hide resolved
@cgillum cgillum self-requested a review October 10, 2024 22:01
@nytian nytian marked this pull request as draft October 21, 2024 19:19
@nytian nytian marked this pull request as ready for review October 21, 2024 20:23
@nytian nytian requested a review from davidmrdavid October 21, 2024 21:17
@nytian nytian marked this pull request as draft October 21, 2024 21:56
@nytian nytian marked this pull request as ready for review October 21, 2024 22:30
cgillum
cgillum reviewed Oct 22, 2024
View reviewed changes
test/Worker.Extensions.DurableTask.Tests/FunctionsDurableTaskClientTests.cs Outdated
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?

cgillum
cgillum approved these changes Oct 22, 2024
View reviewed changes
Copy link
Member

@cgillum cgillum left a comment

Choose a reason for hiding this comment

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

Just the one suggestion on the test code, but otherwise LGTM!

@nytian nytian merged commit a7c6d69 into dev Oct 23, 2024
14 checks passed
@nytian nytian deleted the nytian/http_payload_worker branch October 23, 2024 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidmrdavid davidmrdavid davidmrdavid approved these changes

@cgillum cgillum cgillum approved these changes

@jviau jviau Awaiting requested review from jviau

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nytian @cgillum @davidmrdavid