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

[Enhanced HTTP] Add handling for IActionResult #1475

Merged
merged 9 commits into from
May 3, 2023
Merged

[Enhanced HTTP] Add handling for IActionResult #1475

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
787b03c
commit with 3 prototyping designs
satvu Apr 17, 2023
171ebe9
cleanup
satvu Apr 17, 2023
e9f4cf3
working e2e
satvu Apr 17, 2023
25a76eb
undo changes to program.cs
satvu Apr 17, 2023
d9ab26e
refactor httpuriprovider
satvu Apr 18, 2023
5284dfc
resolve comments
satvu Apr 25, 2023
41a4fd7
program.cs cleanup
satvu Apr 25, 2023
01e768f
resolve comments
satvu Apr 25, 2023
b092ebe
make prop private
satvu May 2, 2023
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
10 changes: 10 additions & 0 deletions ...ker.Extensions.Http.AspNetCore/src/FunctionsMiddleware/FunctionsHttpProxyingMiddleware.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@

using System.Threading.Tasks;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Mvc;
using Microsoft.AspNetCore.Mvc.Abstractions;
using Microsoft.AspNetCore.Routing;
using Microsoft.Azure.Functions.Worker.Middleware;

namespace Microsoft.Azure.Functions.Worker.Extensions.Http.AspNetCore
Expand All @@ -27,6 +30,13 @@ public async Task Invoke(FunctionContext context, FunctionExecutionDelegate next

await next(context);

if (context.GetInvocationResult()?.Value is IActionResult actionResult)
{
ActionContext actionContext = new ActionContext(httpContext, httpContext.GetRouteData(), new ActionDescriptor());
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have plans to fill out ActionDescriptor?

Copy link
Member Author

Choose a reason for hiding this comment

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

Discussed at sync yesterday and no, we do not. Looking at similar code in the host side and we do not fill it out either.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we don't do it in host code I am fine leaving this empty then.

I do wonder what functionality we are excluding here? ActionDescriptor essentially holds a breakdown the MVC endpoint that is being ran. We should consider populating it in the future.

Copy link
Member

Choose a reason for hiding this comment

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

There's no action associated with the context and we have a terminating middleware, no action selection will be performed. As @satvu pointed out, that's the same pattern used by the host.


await actionResult.ExecuteResultAsync(actionContext);
}

// allows asp.net middleware to continue
_coordinator.CompleteFunctionInvocation(invocationId);
}
Expand Down
2 changes: 1 addition & 1 deletion extensions/Worker.Extensions.Http.AspNetCore/src/HostBuilderExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ public static IHostBuilder ConfigureAspNetCoreIntegration(this IHostBuilder buil
{
builder.ConfigureWebHostDefaults(webBuilder =>
{
webBuilder.UseUrls(HttpUriProvider.GetHttpUri().ToString());
webBuilder.UseUrls(HttpUriProvider.HttpUriString);
webBuilder.Configure(b =>
{
b.UseMiddleware<WorkerRequestServicesMiddleware>();
Expand Down
22 changes: 2 additions & 20 deletions extensions/Worker.Extensions.Http.AspNetCore/src/HttpUriProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,26 +7,8 @@ namespace Microsoft.Azure.Functions.Worker.Extensions.Http.AspNetCore
{
internal static class HttpUriProvider
{
private static Uri? _httpUri;
private static Lazy<Uri> HttpUri = new Lazy<Uri>(() => new Uri("http://localhost:" + Utilities.GetUnusedTcpPort().ToString()));

public static Lazy<int> HttpPort = new Lazy<int>(() => Utilities.GetUnusedTcpPort());

public static Uri GetHttpUri()
{
if (_httpUri is not null)
{
return _httpUri;
}

// TODO: replace local host string
var uriString = "http://localhost:" + HttpPort.Value.ToString();

return new Uri(uriString);
}

public static string GetHttpUriAsString()
{
return GetHttpUri().ToString();
}
public static string HttpUriString { get; } = HttpUri.Value.ToString();
}
}
4 changes: 2 additions & 2 deletions extensions/Worker.Extensions.Http.AspNetCore/src/WorkerBuilderExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,12 @@ public static IFunctionsWorkerApplicationBuilder UseAspNetCoreIntegration(this I

builder.Services.AddSingleton<IHttpCoordinator, DefaultHttpCoordinator>();

var port = Utilities.GetUnusedTcpPort().ToString();
Copy link
Member Author

Choose a reason for hiding this comment

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

wasn't being used

builder.Services.AddMvc();

builder.Services.Configure<WorkerOptions>((workerOption) =>
{
workerOption.InputConverters.RegisterAt<HttpContextConverter>(0);
workerOption.Capabilities.Add(Constants.HttpUriCapability, HttpUriProvider.GetHttpUri().ToString()); // testing host side, remove this const later
workerOption.Capabilities.Add(Constants.HttpUriCapability, HttpUriProvider.HttpUriString);
});

return builder;
Expand Down