From 5a489b27ec3e87a2d3d2062d4bf3f14eab2f8db4 Mon Sep 17 00:00:00 2001 From: Brennan Conroy Date: Fri, 12 May 2023 10:04:50 -0700 Subject: [PATCH 1/2] Update HttpLoggingAttribute API --- .../HttpLogging/src/HttpLoggingAttribute.cs | 65 +++++++++++++++---- ...gingEndpointConventionBuilderExtensions.cs | 17 ++++- .../HttpLogging/src/HttpLoggingMiddleware.cs | 4 +- .../HttpLogging/src/PublicAPI.Unshipped.txt | 8 ++- .../test/HttpLoggingAttributeTests.cs | 26 ++++++++ ...tpLoggingEndpointConventionBuilderTests.cs | 4 +- .../test/HttpLoggingMiddlewareTests.cs | 2 +- 7 files changed, 104 insertions(+), 22 deletions(-) create mode 100644 src/Middleware/HttpLogging/test/HttpLoggingAttributeTests.cs diff --git a/src/Middleware/HttpLogging/src/HttpLoggingAttribute.cs b/src/Middleware/HttpLogging/src/HttpLoggingAttribute.cs index 9761063552b0..79b9dbd88cc8 100644 --- a/src/Middleware/HttpLogging/src/HttpLoggingAttribute.cs +++ b/src/Middleware/HttpLogging/src/HttpLoggingAttribute.cs @@ -13,32 +13,73 @@ public sealed class HttpLoggingAttribute : Attribute /// Initializes an instance of the class. /// /// Specifies what fields to log for the endpoint. - /// Specifies the maximum number of bytes to be logged for the request body. A value of -1 means use the default setting in . - /// Specifies the maximum number of bytes to be logged for the response body. A value of -1 means use the default setting in . - /// Thrown when or is less than -1. - public HttpLoggingAttribute(HttpLoggingFields loggingFields, int requestBodyLogLimit = -1, int responseBodyLogLimit = -1) + public HttpLoggingAttribute(HttpLoggingFields loggingFields) { LoggingFields = loggingFields; - - ArgumentOutOfRangeException.ThrowIfLessThan(requestBodyLogLimit, -1); - ArgumentOutOfRangeException.ThrowIfLessThan(responseBodyLogLimit, -1); - - RequestBodyLogLimit = requestBodyLogLimit; - ResponseBodyLogLimit = responseBodyLogLimit; } + private int _responseBodyLogLimit; + private int _requestBodyLogLimit; + /// /// Specifies what fields to log. /// public HttpLoggingFields LoggingFields { get; } + /// + /// Indicates whether has been set. + /// + public bool IsRequestBodyLogLimitSet { get; private set; } + /// /// Specifies the maximum number of bytes to be logged for the request body. /// - public int RequestBodyLogLimit { get; } + /// Thrown when set to a value less than 0. + /// Thrown when getting if it hasn't been set to a value. Check first. + public int RequestBodyLogLimit + { + get + { + if (IsRequestBodyLogLimitSet) + { + return _requestBodyLogLimit; + } + + throw new InvalidOperationException("TODO"); + } + set + { + ArgumentOutOfRangeException.ThrowIfLessThan(value, 0, nameof(RequestBodyLogLimit)); + _requestBodyLogLimit = value; + IsRequestBodyLogLimitSet = true; + } + } + + /// + /// Indicates whether has been set. + /// + public bool IsResponseBodyLogLimitSet { get; private set; } /// /// Specifies the maximum number of bytes to be logged for the response body. /// - public int ResponseBodyLogLimit { get; } + /// Thrown when set to a value less than 0. + /// Thrown when getting if it hasn't been set to a value. Check first. + public int ResponseBodyLogLimit + { + get + { + if (IsResponseBodyLogLimitSet) + { + return _responseBodyLogLimit; + } + throw new InvalidOperationException("TODO"); + } + set + { + ArgumentOutOfRangeException.ThrowIfLessThan(value, 0, nameof(ResponseBodyLogLimit)); + _responseBodyLogLimit = value; + IsResponseBodyLogLimitSet = true; + } + } } diff --git a/src/Middleware/HttpLogging/src/HttpLoggingEndpointConventionBuilderExtensions.cs b/src/Middleware/HttpLogging/src/HttpLoggingEndpointConventionBuilderExtensions.cs index 8d914503ec9a..cc08f900f16a 100644 --- a/src/Middleware/HttpLogging/src/HttpLoggingEndpointConventionBuilderExtensions.cs +++ b/src/Middleware/HttpLogging/src/HttpLoggingEndpointConventionBuilderExtensions.cs @@ -19,11 +19,22 @@ public static class HttpLoggingEndpointConventionBuilderExtensions /// Sets the for this endpoint. A value of -1 means use the default setting in . /// Sets the for this endpoint. A value of -1 means use the default setting in . /// The original convention builder parameter. - /// Thrown when or is less than -1. - public static TBuilder WithHttpLogging(this TBuilder builder, HttpLoggingFields loggingFields, int requestBodyLogLimit = -1, int responseBodyLogLimit = -1) where TBuilder : IEndpointConventionBuilder + /// Thrown when or is less than 0. + public static TBuilder WithHttpLogging(this TBuilder builder, HttpLoggingFields loggingFields, int? requestBodyLogLimit = null, int? responseBodyLogLimit = null) where TBuilder : IEndpointConventionBuilder { // Construct outside build.Add lambda to allow exceptions to be thrown immediately - var metadata = new HttpLoggingAttribute(loggingFields, requestBodyLogLimit, responseBodyLogLimit); + var metadata = new HttpLoggingAttribute(loggingFields); + + if (requestBodyLogLimit is not null) + { + ArgumentOutOfRangeException.ThrowIfLessThan(requestBodyLogLimit.Value, 0, nameof(requestBodyLogLimit)); + metadata.RequestBodyLogLimit = requestBodyLogLimit.Value; + } + if (responseBodyLogLimit is not null) + { + ArgumentOutOfRangeException.ThrowIfLessThan(responseBodyLogLimit.Value, 0, nameof(responseBodyLogLimit)); + metadata.ResponseBodyLogLimit = responseBodyLogLimit.Value; + } builder.Add(endpointBuilder => { diff --git a/src/Middleware/HttpLogging/src/HttpLoggingMiddleware.cs b/src/Middleware/HttpLogging/src/HttpLoggingMiddleware.cs index 05d0b92f7996..98fc0afcb56e 100644 --- a/src/Middleware/HttpLogging/src/HttpLoggingMiddleware.cs +++ b/src/Middleware/HttpLogging/src/HttpLoggingMiddleware.cs @@ -110,7 +110,7 @@ private async Task InvokeInternal(HttpContext context) out var encoding)) { var requestBodyLogLimit = options.RequestBodyLogLimit; - if (loggingAttribute?.RequestBodyLogLimit is int) + if (loggingAttribute?.IsRequestBodyLogLimitSet is true) { requestBodyLogLimit = loggingAttribute.RequestBodyLogLimit; } @@ -161,7 +161,7 @@ private async Task InvokeInternal(HttpContext context) originalBodyFeature = context.Features.Get()!; var responseBodyLogLimit = options.ResponseBodyLogLimit; - if (loggingAttribute?.ResponseBodyLogLimit is int) + if (loggingAttribute?.IsRequestBodyLogLimitSet is true) { responseBodyLogLimit = loggingAttribute.ResponseBodyLogLimit; } diff --git a/src/Middleware/HttpLogging/src/PublicAPI.Unshipped.txt b/src/Middleware/HttpLogging/src/PublicAPI.Unshipped.txt index d2f14db0a9ab..d0353a9d828e 100644 --- a/src/Middleware/HttpLogging/src/PublicAPI.Unshipped.txt +++ b/src/Middleware/HttpLogging/src/PublicAPI.Unshipped.txt @@ -1,8 +1,12 @@ #nullable enable Microsoft.AspNetCore.Builder.HttpLoggingEndpointConventionBuilderExtensions Microsoft.AspNetCore.HttpLogging.HttpLoggingAttribute -Microsoft.AspNetCore.HttpLogging.HttpLoggingAttribute.HttpLoggingAttribute(Microsoft.AspNetCore.HttpLogging.HttpLoggingFields loggingFields, int requestBodyLogLimit = -1, int responseBodyLogLimit = -1) -> void +Microsoft.AspNetCore.HttpLogging.HttpLoggingAttribute.HttpLoggingAttribute(Microsoft.AspNetCore.HttpLogging.HttpLoggingFields loggingFields) -> void +Microsoft.AspNetCore.HttpLogging.HttpLoggingAttribute.IsRequestBodyLogLimitSet.get -> bool +Microsoft.AspNetCore.HttpLogging.HttpLoggingAttribute.IsResponseBodyLogLimitSet.get -> bool Microsoft.AspNetCore.HttpLogging.HttpLoggingAttribute.LoggingFields.get -> Microsoft.AspNetCore.HttpLogging.HttpLoggingFields Microsoft.AspNetCore.HttpLogging.HttpLoggingAttribute.RequestBodyLogLimit.get -> int +Microsoft.AspNetCore.HttpLogging.HttpLoggingAttribute.RequestBodyLogLimit.set -> void Microsoft.AspNetCore.HttpLogging.HttpLoggingAttribute.ResponseBodyLogLimit.get -> int -static Microsoft.AspNetCore.Builder.HttpLoggingEndpointConventionBuilderExtensions.WithHttpLogging(this TBuilder builder, Microsoft.AspNetCore.HttpLogging.HttpLoggingFields loggingFields, int requestBodyLogLimit = -1, int responseBodyLogLimit = -1) -> TBuilder +Microsoft.AspNetCore.HttpLogging.HttpLoggingAttribute.ResponseBodyLogLimit.set -> void +static Microsoft.AspNetCore.Builder.HttpLoggingEndpointConventionBuilderExtensions.WithHttpLogging(this TBuilder builder, Microsoft.AspNetCore.HttpLogging.HttpLoggingFields loggingFields, int? requestBodyLogLimit = null, int? responseBodyLogLimit = null) -> TBuilder diff --git a/src/Middleware/HttpLogging/test/HttpLoggingAttributeTests.cs b/src/Middleware/HttpLogging/test/HttpLoggingAttributeTests.cs new file mode 100644 index 000000000000..d8a641b5c177 --- /dev/null +++ b/src/Middleware/HttpLogging/test/HttpLoggingAttributeTests.cs @@ -0,0 +1,26 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +namespace Microsoft.AspNetCore.HttpLogging.Tests; + +public class HttpLoggingAttributeTests +{ + [Fact] + public void ThrowsForInvalidOptions() + { + var ex = Assert.Throws(() => new HttpLoggingAttribute(HttpLoggingFields.None) { RequestBodyLogLimit = -1 }); + Assert.Equal(nameof(HttpLoggingAttribute.RequestBodyLogLimit), ex.ParamName); + + ex = Assert.Throws(() => new HttpLoggingAttribute(HttpLoggingFields.None) { ResponseBodyLogLimit = -1 }); + Assert.Equal(nameof(HttpLoggingAttribute.ResponseBodyLogLimit), ex.ParamName); + } + + [Fact] + public void ThrowsWhenAccessingFieldsThatAreNotSet() + { + var attribute = new HttpLoggingAttribute(HttpLoggingFields.None); + + Assert.Throws(() => attribute.RequestBodyLogLimit); + Assert.Throws(() => attribute.ResponseBodyLogLimit); + } +} diff --git a/src/Middleware/HttpLogging/test/HttpLoggingEndpointConventionBuilderTests.cs b/src/Middleware/HttpLogging/test/HttpLoggingEndpointConventionBuilderTests.cs index 45d65df5a822..158fbcf1a196 100644 --- a/src/Middleware/HttpLogging/test/HttpLoggingEndpointConventionBuilderTests.cs +++ b/src/Middleware/HttpLogging/test/HttpLoggingEndpointConventionBuilderTests.cs @@ -42,11 +42,11 @@ public void WithHttpLogging_ThrowsForInvalidLimits() // Act & Assert var ex = Assert.Throws(() => - testConventionBuilder.WithHttpLogging(HttpLoggingFields.None, requestBodyLogLimit: -2)); + testConventionBuilder.WithHttpLogging(HttpLoggingFields.None, requestBodyLogLimit: -1)); Assert.Equal("requestBodyLogLimit", ex.ParamName); ex = Assert.Throws(() => - testConventionBuilder.WithHttpLogging(HttpLoggingFields.None, responseBodyLogLimit: -2)); + testConventionBuilder.WithHttpLogging(HttpLoggingFields.None, responseBodyLogLimit: -1)); Assert.Equal("responseBodyLogLimit", ex.ParamName); } } diff --git a/src/Middleware/HttpLogging/test/HttpLoggingMiddlewareTests.cs b/src/Middleware/HttpLogging/test/HttpLoggingMiddlewareTests.cs index 26aa5e905f78..80ecc848b6f1 100644 --- a/src/Middleware/HttpLogging/test/HttpLoggingMiddlewareTests.cs +++ b/src/Middleware/HttpLogging/test/HttpLoggingMiddlewareTests.cs @@ -1337,7 +1337,7 @@ private IHost CreateApp(HttpLoggingFields defaultFields = HttpLoggingFields.All) return "testing"; }).WithHttpLogging((HttpLoggingFields.Request & ~HttpLoggingFields.RequestScheme) | (HttpLoggingFields.Response & ~HttpLoggingFields.ResponseStatusCode)); - endpoint.MapGet("/attr_restrictedsize", [HttpLogging(HttpLoggingFields.Request | HttpLoggingFields.Response, requestBodyLogLimit: 3, responseBodyLogLimit: 6)] async (HttpContext c) => + endpoint.MapGet("/attr_restrictedsize", [HttpLogging(HttpLoggingFields.Request | HttpLoggingFields.Response, RequestBodyLogLimit = 3, ResponseBodyLogLimit = 6)] async (HttpContext c) => { await c.Request.Body.ReadAsync(new byte[100]); return "testing"; From 32870dae3380935960cdb309241212deaef4e698 Mon Sep 17 00:00:00 2001 From: Brennan Date: Fri, 12 May 2023 10:16:18 -0700 Subject: [PATCH 2/2] Apply suggestions from code review --- src/Middleware/HttpLogging/src/HttpLoggingAttribute.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Middleware/HttpLogging/src/HttpLoggingAttribute.cs b/src/Middleware/HttpLogging/src/HttpLoggingAttribute.cs index 79b9dbd88cc8..a30283a302eb 100644 --- a/src/Middleware/HttpLogging/src/HttpLoggingAttribute.cs +++ b/src/Middleware/HttpLogging/src/HttpLoggingAttribute.cs @@ -45,7 +45,7 @@ public int RequestBodyLogLimit return _requestBodyLogLimit; } - throw new InvalidOperationException("TODO"); + throw new InvalidOperationException($"{nameof(RequestBodyLogLimit)} was not set. Check {nameof(IsRequestBodyLogLimitSet)} before accessing this property."); } set { @@ -73,7 +73,7 @@ public int ResponseBodyLogLimit { return _responseBodyLogLimit; } - throw new InvalidOperationException("TODO"); + throw new InvalidOperationException($"{nameof(ResponseBodyLogLimit)} was not set. Check {nameof(IsResponseBodyLogLimitSet)} before accessing this property."); } set {