test: RequestBodyExtractionDispatcher

Author: bruno-garcia

src/Sentry.AspNetCore/SentryAspNetCoreOptions.cs

@@ -21,7 +21,7 @@ public bool IncludeRequestPayload
         {
             get => MaxRequestBodySize != RequestSize.None;
             // As originally there was no truncation, setting to Large.
-            set => MaxRequestBodySize = value ? RequestSize.Large : RequestSize.None;
+            set => MaxRequestBodySize = value ? RequestSize.Always : RequestSize.None;
         }
 
         /// <summary>

src/Sentry/Extensibility/DiagnosticLoggerExtensions.cs

@@ -53,6 +53,13 @@ public static void LogWarning<TArg>(
             TArg arg)
             => logger.LogIfEnabled(SentryLevel.Warning, message, arg);
 
+        public static void LogWarning<TArg, TArg2>(
+            this IDiagnosticLogger logger,
+            string message,
+            TArg arg,
+            TArg2 arg2)
+            => logger.LogIfEnabled(SentryLevel.Warning, message, arg, arg2);
+
         public static void LogError(
             this IDiagnosticLogger logger,
             string message,

src/Sentry/Extensibility/RequestBodyExtractionDispatcher.cs

@@ -1,3 +1,4 @@
+using System;
 using System.Collections.Generic;
 
 namespace Sentry.Extensibility
@@ -11,8 +12,8 @@ public class RequestBodyExtractionDispatcher : IRequestPayloadExtractor
 
         public RequestBodyExtractionDispatcher(IEnumerable<IRequestPayloadExtractor> extractors, SentryOptions options, RequestSize size)
         {
-            Extractors = extractors;
-            _options = options;
+            Extractors = extractors ?? throw new ArgumentNullException(nameof(extractors));
+            _options = options ?? throw new ArgumentNullException(nameof(options));
             _size = size;
         }
 
@@ -23,47 +24,38 @@ public object ExtractPayload(IHttpRequest request)
                 return null;
             }
 
-            var extractRequestBody = false;
             switch (_size)
             {
                 case RequestSize.Small when request.ContentLength < 1_000:
-                    extractRequestBody = true;
-                    break;
                 case RequestSize.Medium when request.ContentLength < 10_000:
-                    extractRequestBody = true;
-                    break;
-                case RequestSize.Large:
-                    extractRequestBody = true;
+                case RequestSize.Always:
+                    _options.DiagnosticLogger.LogDebug("Attempting to read request body of size: {0}, configured max: {1}.",
+                        request.ContentLength, _size);
+
+                    foreach (var extractor in Extractors)
+                    {
+                        var data = extractor.ExtractPayload(request);
+
+                        if (data == null
+                            || data is string dataString
+                            && string.IsNullOrEmpty(dataString))
+                        {
+                            continue;
+                        }
+
+                        return data;
+                    }
                     break;
                 // Request body extraction is opt-in
                 case RequestSize.None:
                     _options.DiagnosticLogger.LogDebug("Skipping request body extraction.");
                     break;
                 default:
-                    _options.DiagnosticLogger.LogWarning("Unknown RequestSize {0}", _size);
+                    _options.DiagnosticLogger.LogWarning("Ignoring request with Size {0} and configuration RequestSize {1}",
+                        request.ContentLength, _size);
                     break;
             }
 
-            if (extractRequestBody)
-            {
-                _options.DiagnosticLogger.LogDebug("Attempting to read request body of size: {0}, configured max: {1}.",
-                    request.ContentLength, _size);
-
-                foreach (var extractor in Extractors)
-                {
-                    var data = extractor.ExtractPayload(request);
-
-                    if (data == null
-                        || data is string dataString
-                        && string.IsNullOrEmpty(dataString))
-                    {
-                        continue;
-                    }
-
-                    return data;
-                }
-            }
-
             return null;
         }
     }

src/Sentry/Extensibility/RequestSize.cs

@@ -19,8 +19,8 @@ public enum RequestSize
         /// </summary>
         Medium,
         /// <summary>
-        /// A large payload is extracted.
+        /// The SDK will always capture the request body. Sentry might truncate or reject the event if too large.
         /// </summary>
-        Large
+        Always
     }
 }

test/Sentry.AspNetCore.Tests/IntegrationMockedBackgroundWorker.cs

@@ -196,7 +196,7 @@ public void AllSettingsViaJson()
 #pragma warning disable 618
             Assert.True(options.IncludeRequestPayload);
 #pragma warning restore 618
-            Assert.Equal(RequestSize.Large, options.MaxRequestBodySize);
+            Assert.Equal(RequestSize.Always, options.MaxRequestBodySize);
             Assert.True(options.SendDefaultPii);
             Assert.True(options.IncludeActivityData);
             Assert.Equal(LogLevel.Error, options.MinimumBreadcrumbLevel);

test/Sentry.AspNetCore.Tests/ScopeExtensionsTests.cs

@@ -21,7 +21,7 @@ public class ScopeExtensionsTests
 #pragma warning disable 618
                 IncludeRequestPayload = true,
 #pragma warning restore 618
-                MaxRequestBodySize = RequestSize.Large
+                MaxRequestBodySize = RequestSize.Always
             };
 
         public ScopeExtensionsTests()

test/Sentry.Tests/Extensibility/RequestBodyExtractionDispatcherTests.cs

@@ -0,0 +1,96 @@
+using System;
+using System.Collections.Generic;
+using NSubstitute;
+using NSubstitute.ReturnsExtensions;
+using Sentry.Extensibility;
+using Xunit;
+
+namespace Sentry.Tests.Extensibility
+{
+    public class RequestBodyExtractionDispatcherTests
+    {
+        private class Fixture
+        {
+            public SentryOptions SentryOptions { get; set; } = new SentryOptions();
+            public RequestSize RequestSize { get; set; } = RequestSize.Small;
+            public IHttpRequest HttpRequest { get; set; } = Substitute.For<IHttpRequest>();
+            public IRequestPayloadExtractor Extractor { get; set; } = Substitute.For<IRequestPayloadExtractor>();
+            public IEnumerable<IRequestPayloadExtractor> Extractors { get; set; }
+
+            public Fixture()
+            {
+                HttpRequest.ContentLength.Returns(10);
+                Extractor.ExtractPayload(Arg.Any<IHttpRequest>()).Returns("Result");
+                Extractors = new[] { Extractor };
+            }
+
+            public RequestBodyExtractionDispatcher GetSut() => new RequestBodyExtractionDispatcher(Extractors, SentryOptions, RequestSize);
+        }
+
+        private readonly Fixture _fixture = new Fixture();
+
+        [Fact]
+        public void ExtractPayload_DefaultFixture_ReadsMockPayload()
+        {
+            var sut = _fixture.GetSut();
+
+            var actual = sut.ExtractPayload(_fixture.HttpRequest);
+            Assert.Same("Result", actual);
+        }
+
+        [Fact]
+        public void ExtractPayload_ExtractorEmptyString_ReturnsNull()
+        {
+            _fixture.Extractor.ExtractPayload(Arg.Any<IHttpRequest>()).Returns(string.Empty);
+            var sut = _fixture.GetSut();
+
+            Assert.Null(sut.ExtractPayload(_fixture.HttpRequest));
+        }
+
+        [Fact]
+        public void ExtractPayload_ExtractorNull_ReturnsNull()
+        {
+            _fixture.Extractor.ExtractPayload(Arg.Any<IHttpRequest>()).ReturnsNull();
+            var sut = _fixture.GetSut();
+
+            Assert.Null(sut.ExtractPayload(_fixture.HttpRequest));
+        }
+
+        [Theory]
+        [InlineData(RequestSize.None, 1, false)]
+        [InlineData(RequestSize.Small, 999, true)]
+        [InlineData(RequestSize.Small, 10_000, false)]
+        [InlineData(RequestSize.Medium, 9999, true)]
+        [InlineData(RequestSize.Medium, 100_000, false)]
+        [InlineData(RequestSize.Always, int.MaxValue, true)] // 2 GB event? No problem...
+        public void ExtractPayload_RequestSizeSmall_ContentLength(RequestSize requestSize, int contentLength, bool readBody)
+        {
+            _fixture.RequestSize = requestSize;
+            _fixture.HttpRequest.ContentLength.Returns(contentLength);
+            var sut = _fixture.GetSut();
+
+            Assert.Equal(readBody, sut.ExtractPayload(_fixture.HttpRequest) != null);
+        }
+
+        [Fact]
+        public void ExtractPayload_NullRequest_ReturnsNull()
+        {
+            var sut = _fixture.GetSut();
+            Assert.Null(sut.ExtractPayload(null));
+        }
+
+        [Fact]
+        public void Ctor_NullExtractors_ThrowsArgumentNullException()
+        {
+            _fixture.Extractors = null;
+            Assert.Throws<ArgumentNullException>(() => _fixture.GetSut());
+        }
+
+        [Fact]
+        public void Ctor_NullOptions_ThrowsArgumentNullException()
+        {
+            _fixture.SentryOptions = null;
+            Assert.Throws<ArgumentNullException>(() => _fixture.GetSut());
+        }
+    }
+}