Skip to content

EmptyHttpResult is not straightforward to be used in type checking #45063

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

Closed
1 task done
Arclight3 opened this issue Nov 13, 2022 · 4 comments
Closed
1 task done

EmptyHttpResult is not straightforward to be used in type checking #45063

Arclight3 opened this issue Nov 13, 2022 · 4 comments
Assignees
Labels
old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels
Milestone

Comments

@Arclight3
Copy link
Contributor

Arclight3 commented Nov 13, 2022

Is there an existing issue for this?

  • I have searched the existing issues

Is your feature request related to a problem? Please describe the problem.

I created a generic endpoint filter

public class EndpointRequestResponseLoggingFilter<TRequest, TResponse> : IEndpointFilter

which has the purpose to log the request and response objects of an endpoint.

As a side note, I know that .NET 7 offers some similar functionality but it's not good enough for my needs (because it doesn't allow me to mark sensitive properties on the request/response object and filter them).

I made some extension methods to simplify the registration of the endpoint filter:

public static RouteHandlerBuilder AddRequestResponseLoggingFilter<TRequest, TResponse>(this RouteHandlerBuilder builder)
{
    builder.AddEndpointFilter<EndpointRequestResponseLoggingFilter<TRequest, TResponse>>();
    return builder;
}

public static RouteHandlerBuilder AddRequestLoggingFilter<TRequest>(this RouteHandlerBuilder builder)
{
    builder.AddEndpointFilter<EndpointRequestResponseLoggingFilter<TRequest, EmptyHttpResult>>();
    return builder;
}

public static RouteHandlerBuilder AddResponseLoggingFilter<TResponse>(this RouteHandlerBuilder builder)
{
    builder.AddEndpointFilter<EndpointRequestResponseLoggingFilter<EmptyHttpRequest, TResponse>>();
    return builder;
}

and I use them to register the filter on some endpoints:

app.MapPost("/myEndpoint", (MyRequest myRequest) => new MyResponse(1, "Message"))
    .AddRequestResponseLoggingFilter<MyRequest, MyResponse>();

app.MapPost("/myEndpointWithNoResponse", (MyRequest myRequest) => { })
    .AddRequestLoggingFilter<MyRequest>();

app.MapPost("/myEndpointWithNoRequest", () => new MyResponse(1, "abc"))
    .AddResponseLoggingFilter<MyResponse>();

Inside the filter I have this logic:

public virtual async ValueTask<object?> InvokeAsync(EndpointFilterInvocationContext context, EndpointFilterDelegate next)
{
    // Log request
    if (context.Arguments.Count > 0)
    {
        var request = context.Arguments[0];

        if (request is TRequest)
            // Filter sensitive properties on the request and log the rest of the object
        else if (request is null)
            _logger.LogError("Request: null");
        else
        {
            _logger.LogError("Expected request type '{providedRequestType}' but found type '{actualRequestType}'.", typeof(TRequest).FullName, request.GetType().FullName);
            // Filter sensitive properties on the request and log the rest of the object
        }
    }
    else if (typeof(TRequest) != typeof(EmptyHttpRequest))
        _logger.LogError("Expected request type '{providedRequestType}' but no request object was received.", typeof(TRequest).FullName);
    else
        _logger.LogInformation("Request: No request");

    var response = await next(context);

    // Log response
    if (response is TResponse)
        // Filter sensitive properties on the response and log the rest of the object
    else
    {
        if (response is null)
            _logger.LogError("Response: null");
        else
        {
            if (response.GetType().Name is nameof(EmptyHttpResult))
            {
                if (typeof(TResponse) != typeof(EmptyHttpResult))
                    _logger.LogError("Expected response type '{providedResponseType}' but no response was returned.", typeof(TResponse).FullName);
                else
                    _logger.LogInformation("Response: No response");
            }
           else
            {
                _logger.LogError("Expected response type '{providedResponseType}' but found type '{actualResponseType}'.", typeof(TResponse).FullName, response.GetType().FullName);
                // Filter sensitive properties on the response and log the rest of the object
            }
        }
    }

    return response;
}

As you can see, I am also doing some validations and type checking to make let the developer know that the type he registered on the endpoint filter is not the same as the actual object type.

The EmptyHttpRequest is a record I created to represent the situation when an endpoint doesn't have a request object.
The EmptyHttpResult is what I saw that the framework returns on the response when the endpoint has no response (e.g.: "myEndpointWithNoResponse"). So when I have an endpoint with no response I register the filter like this builder.AddEndpointFilter<EndpointRequestResponseLoggingFilter<TRequest, EmptyHttpResult>>()

The problem though is that I have to do the comparison like this: if (response.GetType().Name is nameof(EmptyHttpResult)) because the public typed exposed by the framework is Microsoft.AspNetCore.Http.HttpResults.EmptyHttpResult while the type of the response object is Microsoft.AspNetCore.Http.RequestDelegateFactory+EmptyHttpResult.

Describe the solution you'd like

I would like the response object to have the type Microsoft.AspNetCore.Http.HttpResults.EmptyHttpResult so I can do the comparison in a much simpler way: if (response is EmptyHttpResult).

Additional context

I am using the dotnet version: 7.0.100

@davidfowl
Copy link
Member

davidfowl commented Nov 13, 2022

Agreed, we should fix this and patch 7.0. I think there are maybe 2 high level approaches:

  1. Find a way to move Results.Empty where RequestDelegateFactory is
  2. Use reflection to codegen the call to Results.Empty

@davidfowl davidfowl added the old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels label Nov 13, 2022
@captainsafia
Copy link
Member

Find a way to move Results.Empty where RequestDelegateFactory is

Yeah, we considered this exact same scenario when implementing this. We weren't able to do this the first time around because of the challenges with codesharing identified in #41330.

Use reflection to codegen the call to Results.Empty

This might be a fine stopgap...

@davidfowl
Copy link
Member

This is worth patching too.

@captainsafia
Copy link
Member

This is fixed in .NET 7 and 8 now.

@ghost ghost locked as resolved and limited conversation to collaborators Apr 26, 2023
@mkArtakMSFT mkArtakMSFT modified the milestones: 7.0.x, 7.0.3 Nov 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels
Projects
Status: Done
Development

No branches or pull requests

5 participants