feat(poracle): understand PoracleNG's problem+json errors ahead of the v2 move

CHANGELOG.md

@@ -7,6 +7,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Changed
+
+- **A refused alarm explains itself the same way whichever Poracle surface answered.** The v2 write path already read PoracleNG's newer RFC 9457 error bodies and named the individual field it refused; the older v1 path, still the one most installs use, was reading only the older shape and answering a validation refusal as though the server had broken. Both paths now share one reader, and where many fields are refused at once the message names the first few and counts the rest rather than rendering a dozen clauses into a snackbar ([#803](https://github.com/PGAN-Dev/PoracleWeb.NET/issues/803)).
 ### Added
 
 - **Every alarm card says in a sentence what its rule actually does.** A card gave you the species and a row of filter chips, and working out that a rule meant "Bulbasaur within 5 km, 90% IV or better, level 20 to 35" was a matter of decoding the chips. Poracle already writes that sentence -- it is the same wording the bot answers a `!pokemon` command with -- and was returning it on every read of this page, where it was thrown away. It now sits at the foot of each card, under a hairline, below the chips that still read first when you are scanning forty rules. Long ones are clamped to two lines with a control to open them. On the nine card types Poracle words well; fort-change cards keep their chips, because the sentence Poracle renders for them still has a raw JSON array in the middle of it. The line appears only when the language Poracle writes your alerts in is the language you are reading the site in, so a card never carries two languages at once -- and only on a Poracle new enough to send it, which older instances are not; in both cases the card is exactly what it was before ([#810](https://github.com/PGAN-Dev/PoracleWeb.NET/issues/810)).

Core/Pgan.PoracleWebNet.Core.Services/Pgan.PoracleWebNet.Core.Services.csproj

@@ -6,6 +6,13 @@
     <Nullable>enable</Nullable>
   </PropertyGroup>
 
+  <ItemGroup>
+    <!-- PoracleProblemDetails stays internal: it is how this assembly reads an error body, not a
+         contract. The tests reach it directly rather than through a proxy, the same arrangement
+         the API project already uses. -->
+    <InternalsVisibleTo Include="Pgan.PoracleWebNet.Tests" />
+  </ItemGroup>
+
   <ItemGroup>
     <PackageReference Include="Microsoft.Extensions.Caching.Memory" Version="10.0.11" />
     <PackageReference Include="Microsoft.Extensions.Configuration.Abstractions" Version="10.0.11" />

Core/Pgan.PoracleWebNet.Core.Services/PoracleProblemDetails.cs

@@ -1,3 +1,4 @@
+using System.Globalization;
 using System.Text;
 using System.Text.Json;
 
@@ -31,6 +32,9 @@ internal static class PoracleProblemDetails
     /// <summary>What to say when PoracleNG refused and explained nothing usable.</summary>
     public const string Unexplained = "Poracle rejected the alarm.";
 
+    /// <summary>Field errors quoted before the rest are summarised, to keep the message readable.</summary>
+    private const int MaxFieldErrors = 3;
+
     /// <summary>
     /// Reads an explanation out of a response body. Never throws: an unreadable body still has to produce
     /// something to show, and the alternative is a 500 for a request PoracleNG already described.
@@ -114,6 +118,9 @@ public static bool IsProblemJson(string? body)
         }
 
         var described = new StringBuilder();
+        var shown = 0;
+        var hidden = 0;
+
         foreach (var error in errors.EnumerateArray())
         {
             if (error.ValueKind != JsonValueKind.Object)
@@ -127,6 +134,14 @@ public static bool IsProblemJson(string? body)
                 continue;
             }
 
+            // A body refused on a dozen fields would otherwise render a dozen clauses into a snackbar.
+            // The first few name the problem; the count says there is more without spelling it out.
+            if (shown == MaxFieldErrors)
+            {
+                hidden++;
+                continue;
+            }
+
             var field = TrimBodyPointer(StringOf(error, "location"));
 
             if (described.Length > 0)
@@ -135,6 +150,12 @@ public static bool IsProblemJson(string? body)
             }
 
             described.Append(string.IsNullOrWhiteSpace(field) ? message : $"{field}: {message}");
+            shown++;
+        }
+
+        if (hidden > 0)
+        {
+            described.Append(CultureInfo.InvariantCulture, $" (and {hidden} more)");
         }
 
         return described.Length > 0 ? described.ToString() : null;

Core/Pgan.PoracleWebNet.Core.Services/PoracleTrackingProxy.cs

@@ -97,7 +97,11 @@ public async Task<TrackingCreateResult> CreateAsync(string type, string userId,
         // an HttpRequestException that the global handler flattened into 500 "An unexpected error
         // occurred", so the user was told the server broke instead of what was wrong with their input,
         // and it was logged as a fault. Pass the explanation through as a 400. See #539.
-        if (response.StatusCode == HttpStatusCode.BadRequest)
+        //
+        // 422 is the same refusal wearing a different number: PoracleNG 5.2.1 moved several validation
+        // 400s to 422 when it adopted RFC 9457. Matching only 400 would have re-opened #539 on every
+        // one of them.
+        if (response.StatusCode is HttpStatusCode.BadRequest or HttpStatusCode.UnprocessableEntity)
         {
             throw new AlarmValidationException(await ExtractMessageAsync(response));
         }
@@ -349,33 +353,17 @@ private async Task<bool> ServerCarriesV2Async()
         return null;
     }
 
-    /// <summary>Reads whatever explanation PoracleNG returned, falling back to something honest.</summary>
+    /// <summary>
+    /// Reads whatever explanation PoracleNG returned, falling back to something honest.
+    /// </summary>
+    /// <remarks>
+    /// The same reader the v2 path uses. v1 and v2 disagree about the shape of an error -- v1 answers
+    /// <c>{message, status}</c> and v2 answers RFC 9457 problem+json -- but one reader covers both,
+    /// because the field names do not collide. Two readers would be two places to fix a wording bug,
+    /// and one of them would eventually be the one nobody updated.
+    /// </remarks>
     private static async Task<string> ExtractMessageAsync(HttpResponseMessage response)
-    {
-        var body = await response.Content.ReadAsStringAsync();
-
-        try
-        {
-            var root = JsonDocument.Parse(body).RootElement;
-            foreach (var name in new[] { "message", "error", "status" })
-            {
-                if (root.TryGetProperty(name, out var value)
-                    && value.ValueKind == JsonValueKind.String
-                    && !string.IsNullOrWhiteSpace(value.GetString()))
-                {
-                    return value.GetString()!;
-                }
-            }
-        }
-        catch (JsonException)
-        {
-            // Not JSON; the raw body is still better than nothing, as long as it is short.
-        }
-
-        return string.IsNullOrWhiteSpace(body) || body.Length > 300
-            ? "Poracle rejected the alarm."
-            : body;
-    }
+        => PoracleProblemDetails.Describe(await response.Content.ReadAsStringAsync());
 
     private HttpRequestMessage CreateRequest(HttpMethod method, string url)
     {

Tests/Pgan.PoracleWebNet.Tests/Services/PoracleProblemDetailsDescribeTests.cs

@@ -0,0 +1,215 @@
+using System.Net;
+using System.Net.Http;
+using System.Text;
+using Pgan.PoracleWebNet.Core.Services;
+
+namespace Pgan.PoracleWebNet.Tests.Services;
+
+/// <summary>
+/// PoracleNG 5.2.1 replaced <c>{status, message}</c> error bodies with RFC 9457 problem+json on its v2
+/// surface, while v1 kept the old shape. Both have to keep working: a 5.1.0 server is still supported,
+/// so a reader that understood only the new shape would take the explanation away from exactly the
+/// installs that have it today.
+///
+/// These cases came from the v1 side of the proxy and now exercise the same reader the v2 path uses.
+/// </summary>
+public class PoracleProblemDetailsDescribeTests
+{
+    private const string Fallback = PoracleProblemDetails.Unexplained;
+
+    private static string Describe(string body, string contentType = "application/json")
+    {
+        _ = contentType; // Describe reads the body; the header never decides the shape.
+        return PoracleProblemDetails.Describe(body);
+    }
+
+    // ──────────────────────────────────────────────────────────────
+    // problem+json (PoracleNG 5.2.1)
+    // ──────────────────────────────────────────────────────────────
+
+    [Fact]
+    public void ReadsDetailFromProblemJson()
+    {
+        var message = Describe(
+            /*lang=json,strict*/ """{"title":"Unprocessable Entity","status":422,"detail":"validation failed"}""");
+
+        Assert.Equal("validation failed", message);
+    }
+
+    /// <summary>
+    /// errors[] names the fields that were refused, which is more use than the summary in detail, so it
+    /// wins when both are present.
+    /// </summary>
+    [Fact]
+    public void PrefersFieldErrorsOverDetail()
+    {
+        var message = Describe(
+            /*lang=json,strict*/ """
+            {"title":"Unprocessable Entity","status":422,"detail":"validation failed",
+             "errors":[{"message":"expected number <= 100","location":"body.min_iv","value":200}]}
+            """);
+
+        Assert.Equal("min_iv: expected number <= 100", message);
+    }
+
+    /// <summary>
+    /// location arrives as a path into the submitted body. Only the last segment means anything to
+    /// someone looking at the form that produced it.
+    /// </summary>
+    [Fact]
+    public void TrimsTheBodyPrefixFromAFieldLocation()
+    {
+        var message = Describe(
+            /*lang=json,strict*/ """{"errors":[{"message":"required","location":"body.pokemon_id"}]}""");
+
+        Assert.StartsWith("pokemon_id:", message, StringComparison.Ordinal);
+    }
+
+    [Fact]
+    public void SummarisesWhenMoreThanThreeFieldsWereRefused()
+    {
+        var message = Describe(
+            /*lang=json,strict*/ """
+            {"errors":[{"message":"a","location":"body.one"},{"message":"b","location":"body.two"},
+                       {"message":"c","location":"body.three"},{"message":"d","location":"body.four"},
+                       {"message":"e","location":"body.five"}]}
+            """);
+
+        Assert.Contains("one: a", message, StringComparison.Ordinal);
+        Assert.Contains("(and 2 more)", message, StringComparison.Ordinal);
+        Assert.DoesNotContain("five", message, StringComparison.Ordinal);
+    }
+
+    /// <summary>title is the status phrase, so it answers only when nothing better is on the wire.</summary>
+    [Fact]
+    public void FallsBackToTitleWhenThereIsNoDetail()
+    {
+        var message = Describe(/*lang=json,strict*/ """{"title":"Unprocessable Entity","status":422}""");
+
+        Assert.Equal("Unprocessable Entity", message);
+    }
+
+    /// <summary>
+    /// The old shape put a word in status; problem+json puts the HTTP code there. Answering a user with
+    /// "422" explains nothing, so a numeric status is never the message.
+    /// </summary>
+    [Fact]
+    public void NeverReturnsANumericStatusAsTheMessage()
+    {
+        var message = Describe(/*lang=json,strict*/ """{"status":422}""");
+
+        Assert.Equal(Fallback, message);
+    }
+
+    /// <summary>A server that sends problem+json without setting the header is still understood.</summary>
+    [Fact]
+    public void DoesNotDependOnTheContentTypeHeader()
+    {
+        var message = Describe(
+            /*lang=json,strict*/ """{"detail":"validation failed"}""",
+            "application/problem+json");
+
+        Assert.Equal("validation failed", message);
+    }
+
+    // ──────────────────────────────────────────────────────────────
+    // Captured verbatim from a live PoracleNG 5.2.1 (dev-01 :3042)
+    // ──────────────────────────────────────────────────────────────
+
+    /// <summary>
+    /// v2 rejects unknown properties. Note the location carries an array index -- body[0].x -- because
+    /// tracking bodies are arrays of rules; only the trailing segment is shown to the user.
+    /// </summary>
+    [Fact]
+    public void ReadsALiveUnknownPropertyRejection()
+    {
+        var message = Describe(
+            /*lang=json,strict*/ """{"title":"Unprocessable Entity","status":422,"detail":"validation failed","errors":[{"message":"unexpected property","location":"body[0].bogus_field","value":{"bogus_field":1,"pokemon_id":25}}]}""",
+            "application/problem+json");
+
+        Assert.Equal("bogus_field: unexpected property", message);
+    }
+
+    [Fact]
+    public void ReadsALiveTypeMismatchRejection()
+    {
+        var message = Describe(
+            /*lang=json,strict*/ """{"title":"Unprocessable Entity","status":422,"detail":"validation failed","errors":[{"message":"expected integer","location":"body[0].pokemon_id","value":"twenty-five"}]}""",
+            "application/problem+json");
+
+        Assert.Equal("pokemon_id: expected integer", message);
+    }
+
+    /// <summary>A semantic refusal carries no errors[], so detail is the whole explanation.</summary>
+    [Fact]
+    public void ReadsALiveSemanticRejectionWithNoFieldErrors()
+    {
+        var message = Describe(
+            /*lang=json,strict*/ """{"title":"Unprocessable Entity","status":422,"detail":"unknown display_type"}""",
+            "application/problem+json");
+
+        Assert.Equal("unknown display_type", message);
+    }
+
+    /// <summary>
+    /// 5.2.1's frozen v1 surface still answers in the old shape -- captured from the same server that
+    /// produced the problem+json above. This is why the reader must keep both.
+    /// </summary>
+    [Fact]
+    public void ReadsALive521V1Rejection()
+    {
+        var message = Describe(
+            /*lang=json,strict*/ """{"message":"Grunt type mandatory","status":"error"}""");
+
+        Assert.Equal("Grunt type mandatory", message);
+    }
+
+    // ──────────────────────────────────────────────────────────────
+    // {status, message} (PoracleNG 5.1.0) — must keep working
+    // ──────────────────────────────────────────────────────────────
+
+    [Fact]
+    public void StillReadsTheLegacyMessageProperty()
+    {
+        var message = Describe(
+            /*lang=json,strict*/ """{"status":"error","message":"Grunt type mandatory"}""");
+
+        Assert.Equal("Grunt type mandatory", message);
+    }
+
+    [Fact]
+    public void StillReadsTheLegacyErrorProperty()
+    {
+        var message = Describe(/*lang=json,strict*/ """{"error":"An unexpected error occurred."}""");
+
+        Assert.Equal("An unexpected error occurred.", message);
+    }
+
+    // ──────────────────────────────────────────────────────────────
+    // Neither shape
+    // ──────────────────────────────────────────────────────────────
+
+    [Fact]
+    public void EchoesAShortNonJsonBody()
+    {
+        var message = Describe("upstream connect error", "text/plain");
+
+        Assert.Equal("upstream connect error", message);
+    }
+
+    [Fact]
+    public void FallsBackWhenTheBodyIsTooLongToShow()
+    {
+        var message = Describe(new string('x', 301), "text/plain");
+
+        Assert.Equal(Fallback, message);
+    }
+
+    [Fact]
+    public void FallsBackWhenThereIsNoBody()
+    {
+        var message = Describe(string.Empty, "text/plain");
+
+        Assert.Equal(Fallback, message);
+    }
+}

Tests/Pgan.PoracleWebNet.Tests/Services/PoracleTrackingProxyTests.cs

@@ -235,6 +235,42 @@ public async Task CreateAsyncSurfacesPoracleNgsOwnExplanationForABadRequest()
         Assert.Contains("Invalid level", ex.Message, StringComparison.Ordinal);
     }
 
+    /// <summary>
+    /// PoracleNG 5.2.1 moved several validation 400s to 422 when it adopted RFC 9457. Matching only 400
+    /// sent every one of them through EnsureSuccessStatusCode instead, which is the exact path #539 fixed:
+    /// the user is told the server broke rather than what was wrong with their alarm.
+    /// </summary>
+    [Fact]
+    public async Task CreateAsyncSurfacesPoracleNgsOwnExplanationForAnUnprocessableEntity()
+    {
+        var handler = new MockHttpMessageHandler(
+            HttpStatusCode.UnprocessableEntity,
+            /*lang=json,strict*/ """{"title":"Unprocessable Entity","status":422,"detail":"unknown display_type"}""");
+        var sut = CreateSut(handler);
+
+        var body = JsonDocument.Parse("{}").RootElement;
+
+        var ex = await Assert.ThrowsAsync<AlarmValidationException>(
+            () => sut.CreateAsync("invasion", "user1", body));
+        Assert.Contains("unknown display_type", ex.Message, StringComparison.Ordinal);
+    }
+
+    /// <summary>The field-level detail problem+json carries reaches the user, not just the summary.</summary>
+    [Fact]
+    public async Task CreateAsyncSurfacesTheFieldThatWasRefused()
+    {
+        var handler = new MockHttpMessageHandler(
+            HttpStatusCode.UnprocessableEntity,
+            /*lang=json,strict*/ """{"detail":"validation failed","errors":[{"message":"expected number <= 100","location":"body.min_iv"}]}""");
+        var sut = CreateSut(handler);
+
+        var body = JsonDocument.Parse("{}").RootElement;
+
+        var ex = await Assert.ThrowsAsync<AlarmValidationException>(
+            () => sut.CreateAsync("pokemon", "user1", body));
+        Assert.Contains("min_iv", ex.Message, StringComparison.Ordinal);
+    }
+
     // ──────────────────────────────────────────────────────────────
     // DeleteByUidAsync
     // ──────────────────────────────────────────────────────────────