Default IsProblem returning false for a 404

[christianacca]

I'm using YARP to proxy requests to an azure functions app. When the functions app returns a 404, the YARP middleware is setting Response.ContentLength to a value of 0 even though Response.HasStarted is returning false at this point in the pipeline.

The consequence is the default IsProblem function is returning false.

According to comment you made here, the check for ContentLength is redundant. So would it be possible to change the check to the following?:

if (context.Response.ContentLength > 0) {
    return false;
}

For reference, the current code:

if (context.Response.ContentLength.HasValue)
{
    return false;
}

Happy to create a PR for this

[khellang]

According to comment you made here, the check for ContentLength is redundant. So would it be possible to change the check to the following?:

if (context.Response.ContentLength > 0) {
    return false;
}

Honestly I'd be fine with just removing the ContentLength and ContentType checks entirely. The reason they exist in the first place is because Microsoft has them in their code. But removing them might cause some weird edge case somewhere to break, so it's probably best to just keep the change as small as possible.

[khellang]

Actually, I'm not even sure this warrants a change at all. If you're proxying a response from upstream and that response had an empty body, it does make sense that YARP would set Content-Length to 0. If you want to intercept this response and produce a problem response in this case, I think it's fine to just customize the behavior for your scenario. The behavior is pretty easy to configure by setting the IsProblem predicate.

[khellang]

Or you could customize it on the YARP-side by adding a response transform 😅

[christianacca]

Good point about creating a response transform!

I was hoping that the dev ux was such that dropping in ProblemDetailsMiddleware and YARP together would "just work". It took a little while to find that it was the ContentLength check in the default IsProblem had the consequence of a 404 from the Azure functions app to not be treated as a problem.

I also suspect I'm a little on the OCD spectrum when it comes to even verifying that a ProblemDetails response is being returned in this case. I strongly suspect that my peers in the software house I work for would not have noticed :-0. Hence the desire for out-of-the-box experience to "just work"

If you want to intercept this response and produce a problem response in this case, I think it's fine to just customize the behavior for your scenario. The behavior is pretty easy to configure by setting the IsProblem predicate.

Yeah that's what I've done but I felt sad that I had to duplicate the existing default code, particularly the IsProblemStatusCode:

    public static void ConfigureProblemDetails(ProblemDetailsOptions c) {
        // snip
        c.IsProblem = IsProblem;

        bool IsProblem(HttpContext context) {
            
            // note: this function is a copy of https://github.com/khellang/Middleware/blob/70ac2fb251baabc750c5a7e40e619cfc59c12e1f/src/ProblemDetails/ProblemDetailsOptionsSetup.cs#L79-L97
            // we're having to copy it to modify the check for ContentLength as YARP is setting ContentLength to zero
            // even though it has not started sending the response yet
            
            if (!IsProblemStatusCode(context.Response.StatusCode)) {
                return false;
            }
        
            // if (context.Response.ContentLength.HasValue)
            if (context.Response.ContentLength > 0) {
                return false;
            }

            if (string.IsNullOrEmpty(context.Response.ContentType)) {
                return true;
            }

            return false;
        }

        bool IsProblemStatusCode(int? statusCode) {
            return statusCode switch
            {
                >= 600 => false,
                < 400 => false,
                null => false,
                _ => true
            };
        }
    }

[khellang]

I felt sad that I had to duplicate the existing default code, particularly the IsProblemStatusCode

Yeah, I get that, but this code will never change, so I don't think it would be a big maintenance burden. If I were you, I'd just replace the entire IsProblem method with the equivalent of IsProblemStatusCode. If it makes you feel any better, we can make IsProblemStatusCode public?

[christianacca]

:-) Yes making IsProblemStatusCode public would be a perfect compromise I reckon. Shall I submit a PR?

[khellang]

Shall I submit a PR?

Sure, go for it! 😃

[christianacca]

If I bump the minor version (eg 6.3.0 -> 6.4.0), will that trigger the publish of the nuget package?

[khellang]

No, I haven't gotten around to automatic push to NuGet, so have to push the binaries manually still :(