From 448792c20cd707384849ffdf1ea835881d6a8cf1 Mon Sep 17 00:00:00 2001 From: Eric Sibly Date: Sun, 3 Mar 2024 19:40:32 -0800 Subject: [PATCH] Further ETag testing and changes. --- CHANGELOG.md | 1 + .../WebApis/ValueContentResult.cs | 31 ++++++++++++++----- src/CoreEx.AspNetCore/WebApis/WebApi.cs | 23 +++++++++++--- src/CoreEx.Database/DatabaseRecord.cs | 10 ++++-- src/CoreEx/Abstractions/ETagGenerator.cs | 16 ++++++++-- src/CoreEx/ExecutionContext.cs | 13 ++------ src/CoreEx/Http/HttpExtensions.cs | 2 +- src/CoreEx/Http/TypedHttpClientBase.cs | 1 - src/CoreEx/Json/IJsonPreFilterInspector.cs | 2 +- .../Framework/WebApis/WebApiTest.cs | 28 +++++++++++++++-- .../Framework/WebApis/WebApiWithResultTest.cs | 28 +++++++++++++++-- 11 files changed, 120 insertions(+), 35 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3752262c..9efe61a5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ Represents the **NuGet** versions. - *Fixed*: Validation extensions `Exists` and `ExistsAsync` which expect a non-null resultant value have been renamed to `ValueExists` and `ValueExistsAsync` to improve usability; also they are `IResult` aware and will act accordingly. - *Fixed*: The `ETag` HTTP handling has been updated to correctly output and expect the weak `W/"xxxx"` format. - *Fixed*: The `ETagGenerator` implementation has been further optimized to minimize unneccessary string allocations. +- *Fixed*: The `ValueContentResult` will only generate a response header ETag (`ETagGenerator`) for a `GET` or `HEAD` request. The underlying result `IETag.ETag` is used as-is where there is no query string; otherwise, generates as assumes query string will alter result (i.e. filtering, paging, sorting, etc.). The result `IETag.ETag` is unchanged so the consumer can still use as required for a further operation. - *Fixed*: The `SettingsBase` has been optimized. The internal recursion checking has been removed and as such an endless loop (`StackOverflowException`) may occur where misconfigured; given frequency of `IConfiguration` usage the resulting performance is deemed more important. Additionally, `prefixes` are now optional. - The existing support of referencing a settings property by name (`settings.GetValue("NamedProperty")`) and it using reflection to find before querying the `IConfiguration` has been removed. This was not a common, or intended usage, and was somewhat magical, and finally was non-performant. diff --git a/src/CoreEx.AspNetCore/WebApis/ValueContentResult.cs b/src/CoreEx.AspNetCore/WebApis/ValueContentResult.cs index 5193e6a5..ca8d4e71 100644 --- a/src/CoreEx.AspNetCore/WebApis/ValueContentResult.cs +++ b/src/CoreEx.AspNetCore/WebApis/ValueContentResult.cs @@ -10,7 +10,6 @@ using System; using System.Collections; using System.Collections.Generic; -using System.Diagnostics.CodeAnalysis; using System.Linq; using System.Net; using System.Net.Mime; @@ -149,13 +148,25 @@ public static bool TryCreateValueContentResult(T value, HttpStatusCode status throw new InvalidOperationException("Function has not returned a result; no AlternateStatusCode has been configured to return."); } + // Where there is etag support and it is null (assumes auto-generation) then generate from the full value JSON contents as the baseline value. + var isTextSerializationEnabled = ExecutionContext.HasCurrent && ExecutionContext.Current.IsTextSerializationEnabled; + var etag = value is IETag vetag ? vetag.ETag : null; + if (etag is null) + { + if (isTextSerializationEnabled) + ExecutionContext.Current.IsTextSerializationEnabled = false; + + etag = ETagGenerator.Generate(jsonSerializer, value); + if (value is IETag vetag2) + vetag2.ETag = etag; + } + // Where IncludeText is selected then enable before serialization occurs. if (requestOptions.IncludeText && ExecutionContext.HasCurrent) ExecutionContext.Current.IsTextSerializationEnabled = true; - // Serialize and generate the etag whilst also applying any filtering of the data where selected. + // Serialize the value performing any filtering as per the request options. string? json = null; - if (requestOptions.IncludeFields != null && requestOptions.IncludeFields.Length > 0) jsonSerializer.TryApplyFilter(val, requestOptions.IncludeFields, out json, JsonPropertyFilter.Include); else if (requestOptions.ExcludeFields != null && requestOptions.ExcludeFields.Length > 0) @@ -163,8 +174,13 @@ public static bool TryCreateValueContentResult(T value, HttpStatusCode status else json = jsonSerializer.Serialize(val); + // Generate the etag from the final JSON serialization and check for not-modified. var result = GenerateETag(requestOptions, val, json, jsonSerializer); + // Reset the text serialization flag. + if (ExecutionContext.HasCurrent) + ExecutionContext.Current.IsTextSerializationEnabled = isTextSerializationEnabled; + // Check for not-modified and return status accordingly. if (checkForNotModified && result.etag == requestOptions.ETag) { @@ -174,7 +190,7 @@ public static bool TryCreateValueContentResult(T value, HttpStatusCode status } // Create and return the ValueContentResult. - primaryResult = new ValueContentResult(result.json!, statusCode, result.etag, paging, location); + primaryResult = new ValueContentResult(result.json!, statusCode, result.etag ?? etag, paging, location); alternateResult = null; return true; } @@ -189,15 +205,16 @@ public static bool TryCreateValueContentResult(T value, HttpStatusCode status /// The etag and serialized JSON (where performed). internal static (string? etag, string? json) GenerateETag(WebApiRequestOptions requestOptions, T value, string? json, IJsonSerializer jsonSerializer) { + // Where not a GET or HEAD then no etag is generated; just use what we have. + if (!HttpMethods.IsGet(requestOptions.Request.Method) && !HttpMethods.IsHead(requestOptions.Request.Method)) + return (value is IETag etag ? etag.ETag : null, json); + // Where no query string and there is an etag then that value should be leveraged as the fast-path. if (!requestOptions.HasQueryString) { if (value is IETag etag && etag.ETag != null) return (etag.ETag, json); - if (ExecutionContext.HasCurrent && ExecutionContext.Current.ResultETag != null) - return (ExecutionContext.Current.ResultETag, json); - // Where there is a collection then we need to generate a hash that represents the collection. if (json is null && value is not string && value is IEnumerable coll) { diff --git a/src/CoreEx.AspNetCore/WebApis/WebApi.cs b/src/CoreEx.AspNetCore/WebApis/WebApi.cs index 4c5447a7..20a4446d 100644 --- a/src/CoreEx.AspNetCore/WebApis/WebApi.cs +++ b/src/CoreEx.AspNetCore/WebApis/WebApi.cs @@ -1,5 +1,6 @@ // Copyright (c) Avanade. Licensed under the MIT License. See https://github.com/Avanade/CoreEx +using CoreEx.Abstractions; using CoreEx.AspNetCore.Http; using CoreEx.Configuration; using CoreEx.Entities; @@ -710,17 +711,29 @@ private async Task PutInternalAsync(HttpRequest request, /// private ConcurrencyException? ConcurrencyETagMatching(WebApiParam wap, TValue getValue, TValue putValue, bool autoConcurrency) { - var et = putValue as IETag; - if (et != null || autoConcurrency) + var ptag = putValue as IETag; + if (ptag != null || autoConcurrency) { - string? etag = et?.ETag ?? wap.RequestOptions.ETag; + string? etag = wap.RequestOptions.ETag ?? ptag?.ETag; if (string.IsNullOrEmpty(etag)) return new ConcurrencyException($"An 'If-Match' header is required for an HTTP {wap.Request.Method} where the underlying entity supports concurrency (ETag)."); if (etag != null) { - var getEt = ValueContentResult.GenerateETag(wap.RequestOptions, getValue!, null, JsonSerializer); - if (etag != getEt.etag) + var gtag = getValue is IETag getag ? getag.ETag : null; + if (gtag is null) + { + var isTextSerializationEnabled = ExecutionContext.HasCurrent && ExecutionContext.Current.IsTextSerializationEnabled; + if (isTextSerializationEnabled) + ExecutionContext.Current.IsTextSerializationEnabled = false; + + gtag = ETagGenerator.Generate(JsonSerializer, getValue); + + if (ExecutionContext.HasCurrent) + ExecutionContext.Current.IsTextSerializationEnabled = isTextSerializationEnabled; + } + + if (etag != gtag) return new ConcurrencyException(); } } diff --git a/src/CoreEx.Database/DatabaseRecord.cs b/src/CoreEx.Database/DatabaseRecord.cs index 0d4ae422..ccc6e718 100644 --- a/src/CoreEx.Database/DatabaseRecord.cs +++ b/src/CoreEx.Database/DatabaseRecord.cs @@ -2,7 +2,6 @@ using CoreEx.Entities; using System; -using System.Data; using System.Data.Common; namespace CoreEx.Database @@ -36,7 +35,14 @@ public class DatabaseRecord(IDatabase database, DbDataReader dataReader) /// /// The ordinal index. /// The value. - public object? GetValue(int ordinal) => DataReader.IsDBNull(ordinal) ? default! : DataReader.GetValue(ordinal); + public object? GetValue(int ordinal) + { + if (DataReader.IsDBNull(ordinal)) + return default; + + var val = DataReader.GetValue(ordinal); + return val is DateTime dt ? Cleaner.Clean(dt, Database.DateTimeTransform) : val; + } /// /// Gets the named column value. diff --git a/src/CoreEx/Abstractions/ETagGenerator.cs b/src/CoreEx/Abstractions/ETagGenerator.cs index 9c7513b1..a112bf63 100644 --- a/src/CoreEx/Abstractions/ETagGenerator.cs +++ b/src/CoreEx/Abstractions/ETagGenerator.cs @@ -90,10 +90,22 @@ public static class ETagGenerator } /// - /// Parses an by removing double quotes character bookends; for example '"abc"' would be formatted as 'abc'. + /// Parses an by removing any weak prefix ('W/') double quotes character bookends; for example '"abc"' would be formatted as 'abc'. /// /// The to unformat. /// The unformatted value. - public static string? ParseETag(string? etag) => etag is not null && etag.Length > 1 && etag.StartsWith("\"", StringComparison.InvariantCultureIgnoreCase) && etag.EndsWith("\"", StringComparison.InvariantCultureIgnoreCase) ? etag[1..^1] : etag; + public static string? ParseETag(string? etag) + { + if (string.IsNullOrEmpty(etag)) + return null; + + if (etag.StartsWith('\"') && etag.EndsWith('\"')) + return etag[1..^1]; + + if (etag.StartsWith("W/\"") && etag.EndsWith('\"')) + return etag[2..^1]; + + return etag; + } } } \ No newline at end of file diff --git a/src/CoreEx/ExecutionContext.cs b/src/CoreEx/ExecutionContext.cs index f3fc1e6e..ed993c7e 100644 --- a/src/CoreEx/ExecutionContext.cs +++ b/src/CoreEx/ExecutionContext.cs @@ -132,7 +132,7 @@ public static object GetRequiredService(Type type) /// /// Gets the . /// - /// This is automatically set via the . + /// This is automatically set via the . public IServiceProvider? ServiceProvider { get; set; } /// @@ -151,11 +151,6 @@ public static object GetRequiredService(Type type) /// public bool IsTextSerializationEnabled { get; set; } - /// - /// Gets or sets the result entity tag (used where the value does not explicitly implement ). - /// - public string? ResultETag { get; set; } - /// /// Gets or sets the corresponding user name. /// @@ -202,15 +197,14 @@ public virtual ExecutionContext CreateCopy() { var ec = Create == null ? throw new InvalidOperationException($"The {nameof(Create)} function must not be null to create a copy.") : Create(); ec._timestamp = _timestamp; - ec._referenceDataContext = _referenceDataContext; ec._messages = _messages; ec._properties = _properties; + ec._referenceDataContext = _referenceDataContext; ec._roles = _roles; ec.ServiceProvider = ServiceProvider; ec.CorrelationId = CorrelationId; ec.OperationType = OperationType; ec.IsTextSerializationEnabled = IsTextSerializationEnabled; - ec.ResultETag = ResultETag; ec.UserName = UserName; ec.UserId = UserId; ec.TenantId = TenantId; @@ -229,7 +223,6 @@ public void Dispose() if (!_disposed) { _disposed = true; - Reset(); Dispose(true); } } @@ -243,7 +236,7 @@ public void Dispose() /// Releases the unmanaged resources used by the and optionally releases the managed resources. /// /// true to release both managed and unmanaged resources; false to release only unmanaged resources. - protected virtual void Dispose(bool disposing) { } + protected virtual void Dispose(bool disposing) => Reset(); #region Security diff --git a/src/CoreEx/Http/HttpExtensions.cs b/src/CoreEx/Http/HttpExtensions.cs index 5c18ee3c..d5eecd1d 100644 --- a/src/CoreEx/Http/HttpExtensions.cs +++ b/src/CoreEx/Http/HttpExtensions.cs @@ -89,7 +89,7 @@ public static HttpRequestMessage ApplyRequestOptions(this HttpRequestMessage htt /// The . /// The ETag value. /// The to support fluent-style method-chaining. - /// Automatically adds quoting to be ETag format compliant. + /// Automatically adds quoting to be ETag format compliant and sets the ETag as weak ('W/'). public static HttpRequestMessage ApplyETag(this HttpRequestMessage httpRequest, string? etag) { // Apply the ETag header. diff --git a/src/CoreEx/Http/TypedHttpClientBase.cs b/src/CoreEx/Http/TypedHttpClientBase.cs index fc3e508d..139ce006 100644 --- a/src/CoreEx/Http/TypedHttpClientBase.cs +++ b/src/CoreEx/Http/TypedHttpClientBase.cs @@ -105,7 +105,6 @@ private async Task CreateRequestInternalAsync(HttpMethod met // Access the query string. var uri = new Uri(requestUri, UriKind.RelativeOrAbsolute); - var ub = new UriBuilder(uri.IsAbsoluteUri ? uri : new Uri(new Uri("https://coreex"), requestUri)); var qs = HttpUtility.ParseQueryString(ub.Query); diff --git a/src/CoreEx/Json/IJsonPreFilterInspector.cs b/src/CoreEx/Json/IJsonPreFilterInspector.cs index be36e6a8..6342ea8b 100644 --- a/src/CoreEx/Json/IJsonPreFilterInspector.cs +++ b/src/CoreEx/Json/IJsonPreFilterInspector.cs @@ -9,7 +9,7 @@ namespace CoreEx.Json public interface IJsonPreFilterInspector { /// - /// Gets the underlying JSON object (as per the underlying implementation). + /// Gets the underlying JSON object (as per the underlying implementation). /// object Json { get; } diff --git a/tests/CoreEx.Test/Framework/WebApis/WebApiTest.cs b/tests/CoreEx.Test/Framework/WebApis/WebApiTest.cs index 32e78333..61df5b20 100644 --- a/tests/CoreEx.Test/Framework/WebApis/WebApiTest.cs +++ b/tests/CoreEx.Test/Framework/WebApis/WebApiTest.cs @@ -220,6 +220,24 @@ public void GetAsync_WithETagValueNotModified() [Test] public void GetAsync_WithGenETagValue() + { + using var test = FunctionTester.Create(); + var vcr = test.Type() + .Run(f => f.GetAsync(test.CreateHttpRequest(HttpMethod.Get, "https://unittest/testget"), r => Task.FromResult(new Person { Id = 1, Name = "Angela" }))) + .ToActionResultAssertor() + .AssertOK() + .Result as ValueContentResult; + + Assert.That(vcr, Is.Not.Null); + Assert.That(vcr!.ETag, Is.EqualTo("iVsGVb/ELj5dvXpe3ImuOy/vxLIJnUtU2b8nIfpX5PM=")); + + var p = test.JsonSerializer.Deserialize(vcr.Content!); + Assert.That(p, Is.Not.Null); + Assert.That(p.ETag, Is.EqualTo("iVsGVb/ELj5dvXpe3ImuOy/vxLIJnUtU2b8nIfpX5PM=")); + } + + [Test] + public void GetAsync_WithGenETagValue_QueryString() { using var test = FunctionTester.Create(); var vcr = test.Type() @@ -229,7 +247,11 @@ public void GetAsync_WithGenETagValue() .Result as ValueContentResult; Assert.That(vcr, Is.Not.Null); - Assert.That(vcr!.ETag, Is.EqualTo("F/BIL6G5jbvZxc4fnCc5BekkmFM9/BuXSSl/i5bQj5Q=")); + Assert.That(vcr!.ETag, Is.EqualTo("cpDn3xugV1xKSHF9AY4kQRNQ1yC/SU49xC66C92WZbE=")); + + var p = test.JsonSerializer.Deserialize(vcr.Content!); + Assert.That(p, Is.Not.Null); + Assert.That(p.ETag, Is.EqualTo("iVsGVb/ELj5dvXpe3ImuOy/vxLIJnUtU2b8nIfpX5PM=")); } [Test] @@ -237,7 +259,7 @@ public void GetAsync_WithGenETagValueNotModified() { using var test = FunctionTester.Create(); var hr = test.CreateHttpRequest(HttpMethod.Get, "https://unittest/testget?fruit=apples"); - hr.Headers.Add(HeaderNames.IfMatch, "\\W\"F/BIL6G5jbvZxc4fnCc5BekkmFM9/BuXSSl/i5bQj5Q=\""); + hr.Headers.Add(HeaderNames.IfMatch, "\\W\"cpDn3xugV1xKSHF9AY4kQRNQ1yC/SU49xC66C92WZbE=\""); test.Type() .Run(f => f.GetAsync(hr, r => Task.FromResult(new Person { Id = 1, Name = "Angela" }))) @@ -650,7 +672,7 @@ public void PatchAsync_AutoConcurrency_Matched() .Run(f => f.PatchAsync(hr, get: _ => Task.FromResult(new Person { Id = 13, Name = "Deano" }), put: _ => Task.FromResult(new Person { Id = 13, Name = "Gazza" }), simulatedConcurrency: true)) .ToActionResultAssertor() .AssertOK() - .AssertValue(new Person { Id = 13, Name = "Gazza" }); + .AssertValue(new Person { Id = 13, Name = "Gazza", ETag = "tEEokPXk+4Q5MoiGqyAs1+6A00e2ww59Zm57LJgvBcg=" }); } private static HttpRequest CreatePatchRequest(UnitTestEx.NUnit.Internal.FunctionTester test, string? json, string? etag = null) diff --git a/tests/CoreEx.Test/Framework/WebApis/WebApiWithResultTest.cs b/tests/CoreEx.Test/Framework/WebApis/WebApiWithResultTest.cs index 120df7f2..a5566ac7 100644 --- a/tests/CoreEx.Test/Framework/WebApis/WebApiWithResultTest.cs +++ b/tests/CoreEx.Test/Framework/WebApis/WebApiWithResultTest.cs @@ -72,6 +72,24 @@ public void GetWithResultAsync_WithETagValueNotModified() [Test] public void GetWithResultAsync_WithGenETagValue() + { + using var test = FunctionTester.Create(); + var vcr = test.Type() + .Run(f => f.GetWithResultAsync(test.CreateHttpRequest(HttpMethod.Get, "https://unittest/testget"), r => Task.FromResult(Result.Ok(new Person { Id = 1, Name = "Angela" })))) + .ToActionResultAssertor() + .AssertOK() + .Result as ValueContentResult; + + Assert.That(vcr, Is.Not.Null); + Assert.That(vcr!.ETag, Is.EqualTo("iVsGVb/ELj5dvXpe3ImuOy/vxLIJnUtU2b8nIfpX5PM=")); + + var p = test.JsonSerializer.Deserialize(vcr.Content!); + Assert.That(p, Is.Not.Null); + Assert.That(p.ETag, Is.EqualTo("iVsGVb/ELj5dvXpe3ImuOy/vxLIJnUtU2b8nIfpX5PM=")); + } + + [Test] + public void GetWithResultAsync_WithGenETagValue_QueryString() { using var test = FunctionTester.Create(); var vcr = test.Type() @@ -81,7 +99,11 @@ public void GetWithResultAsync_WithGenETagValue() .Result as ValueContentResult; Assert.That(vcr, Is.Not.Null); - Assert.That(vcr!.ETag, Is.EqualTo("F/BIL6G5jbvZxc4fnCc5BekkmFM9/BuXSSl/i5bQj5Q=")); + Assert.That(vcr!.ETag, Is.EqualTo("cpDn3xugV1xKSHF9AY4kQRNQ1yC/SU49xC66C92WZbE=")); + + var p = test.JsonSerializer.Deserialize(vcr.Content!); + Assert.That(p, Is.Not.Null); + Assert.That(p.ETag, Is.EqualTo("iVsGVb/ELj5dvXpe3ImuOy/vxLIJnUtU2b8nIfpX5PM=")); } [Test] @@ -89,7 +111,7 @@ public void GetWithResultAsync_WithGenETagValueNotModified() { using var test = FunctionTester.Create(); var hr = test.CreateHttpRequest(HttpMethod.Get, "https://unittest/testget?fruit=apples"); - hr.Headers.Add(HeaderNames.IfMatch, "\\W\"F/BIL6G5jbvZxc4fnCc5BekkmFM9/BuXSSl/i5bQj5Q=\""); + hr.Headers.Add(HeaderNames.IfMatch, "\\W\"cpDn3xugV1xKSHF9AY4kQRNQ1yC/SU49xC66C92WZbE=\""); test.Type() .Run(f => f.GetWithResultAsync(hr, r => Task.FromResult(Result.Ok(new Person { Id = 1, Name = "Angela" })))) @@ -523,7 +545,7 @@ public void PatchWithResultAsync_AutoConcurrency_Matched() .Run(f => f.PatchWithResultAsync(hr, get: _ => Task.FromResult(Result.Ok(new Person { Id = 13, Name = "Deano" })), put: _ => Task.FromResult(Result.Ok(new Person { Id = 13, Name = "Gazza" })), simulatedConcurrency: true)) .ToActionResultAssertor() .AssertOK() - .AssertValue(new Person { Id = 13, Name = "Gazza" }); + .AssertValue(new Person { Id = 13, Name = "Gazza", ETag = "tEEokPXk+4Q5MoiGqyAs1+6A00e2ww59Zm57LJgvBcg=" }); } [Test]