From 401576b7106cf3d0ec0143b42d6eb4ba43bae9da Mon Sep 17 00:00:00 2001 From: hokiepokedad2 <38219945+hokiepokedad2@users.noreply.github.com> Date: Sun, 23 Aug 2026 22:45:49 -0400 Subject: [PATCH 1/2] feat(poracle): understand PoracleNG's problem+json errors ahead of the v2 move PoracleNG 5.2.1 answers its new /api/v2 with RFC 9457 application/problem+json and refuses invalid input with 422 rather than 400. This changes nothing for anyone today, and the first version of this commit said otherwise. Verified against a real 5.2.1 server (dev-01 :3042): the frozen v1 surface this application actually calls still answers {"message":"...","status":"error"} at 400, exactly as 5.1.0 does. The new format is confined to v2, which we do not yet speak. This is groundwork for #805, not a live regression fix. It is still worth doing before that migration rather than during it. Without it, the first endpoint moved to v2 would match only BadRequest, send every 422 through EnsureSuccessStatusCode, and report the caller's own mistake as 500 "An unexpected error occurred" while logging it as a server fault -- the exact path #539 fixed. Extraction moves to PoracleErrorMessage, which reads both shapes by field name rather than branching on a version or a content type. The names do not collide, so one reader serves both, and a reverse proxy rewriting Content-Type cannot cost a user their error message. problem+json errors[] names the individual fields that were refused, which the old format could not do, so it is preferred over the detail summary. A numeric status is never returned as the message: the old shape put a word there, the new one puts the HTTP code, and answering a user with "422" explains nothing. CreateAsync now also treats 422 as the caller's problem, so the v1 and v2 paths agree. One behaviour change beyond that: a body that parses as JSON but carries no explanation falls back to a plain sentence instead of being echoed. The raw echo is for bodies that are not JSON at all, such as a proxy's "upstream connect error"; raw JSON in a snackbar tells a user less than a sentence does. Four tests carry payloads captured verbatim from the live 5.2.1 instance, including the v1 rejection from the same server, which is the evidence that both shapes must stay readable. PoracleHumanProxy has the same 400-becomes-500 gap on its write paths. That predates 5.2.1 and is left for its own change. Refs #803 --- CHANGELOG.md | 4 + .../PoracleErrorMessage.cs | 162 +++++++++++++ .../PoracleTrackingProxy.cs | 34 +-- .../Services/PoracleErrorMessageTests.cs | 225 ++++++++++++++++++ .../Services/PoracleTrackingProxyTests.cs | 36 +++ 5 files changed, 434 insertions(+), 27 deletions(-) create mode 100644 Core/Pgan.PoracleWebNet.Core.Services/PoracleErrorMessage.cs create mode 100644 Tests/Pgan.PoracleWebNet.Tests/Services/PoracleErrorMessageTests.cs diff --git a/CHANGELOG.md b/CHANGELOG.md index a42a262b..7ad71ce3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- **Poracle's newer error format is understood, ahead of the move to its v2 API.** PoracleNG 5.2.1 answers its new `/api/v2` with RFC 9457 problem+json and refuses invalid input with 422 rather than 400. Nothing changes for anyone today: every call this site makes is on Poracle's frozen v1 surface, which still answers in the older format on 5.2.1 -- verified against a 5.2.1 server rather than assumed. What changes is that the reader now understands both, and where the newer format names the individual field it refused, that field is named to the user instead of a general "validation failed". Without this the first endpoint moved to v2 would have reported every refusal as a server fault ([#803](https://github.com/PGAN-Dev/PoracleWeb.NET/issues/803)). + ### Fixed - **The PVP rank range is readable again on a dark-themed alarm card.** The band under a PVP alarm showed its league and nothing else, so the ranks you had set looked like they had been dropped. They were being drawn, in white, on a band that stays light in both themes. The league name gained some contrast on the way past ([#800](https://github.com/PGAN-Dev/PoracleWeb.NET/issues/800)). diff --git a/Core/Pgan.PoracleWebNet.Core.Services/PoracleErrorMessage.cs b/Core/Pgan.PoracleWebNet.Core.Services/PoracleErrorMessage.cs new file mode 100644 index 00000000..aba308fa --- /dev/null +++ b/Core/Pgan.PoracleWebNet.Core.Services/PoracleErrorMessage.cs @@ -0,0 +1,162 @@ +using System.Globalization; +using System.Net.Http; +using System.Text; +using System.Text.Json; + +namespace Pgan.PoracleWebNet.Core.Services; + +/// +/// Reads whatever explanation Poracle returned with a refusal, across both of its wire formats. +/// +/// +/// +/// PoracleNG 5.2.1 replaced the {status, message} error body with RFC 9457 +/// application/problem+json -- {type, title, status, detail, errors[]} -- and moved a +/// number of 400s to 422. Both shapes are read here by field name rather than by branching on a +/// version or a content type: the names do not collide, so one reader serves both, and a reverse +/// proxy that rewrites Content-Type cannot cost a user their error message. +/// +/// +/// status is read last and only when it is a string. The old shape used it for a word; the new +/// one uses it for the HTTP code, and answering a user with "422" explains nothing. title is +/// last of the problem+json fields for the same reason -- it is the status phrase +/// ("Unprocessable Entity"), not a description of what was wrong. +/// +/// +public static class PoracleErrorMessage +{ + /// Longest body echoed verbatim when nothing could be parsed out of it. + private const int MaxRawBodyLength = 300; + + /// Most specific first. See the remarks on for the ordering. + private static readonly string[] MessageProperties = ["detail", "message", "error", "status", "title"]; + + /// Field-level entries quoted before the list is summarised, to keep the message readable. + private const int MaxFieldErrors = 3; + + /// + /// Returns Poracle's own explanation for a failed response, or when it + /// did not give one. + /// + public static async Task ExtractAsync(HttpResponseMessage response, string fallback) + { + var body = await response.Content.ReadAsStringAsync(); + + try + { + var root = JsonDocument.Parse(body).RootElement; + + // A body that parsed is Poracle talking to us in a shape we know. If we cannot find an + // explanation in it we fall back rather than echoing it: raw JSON in a snackbar tells a user + // less than a plain sentence does. The echo below is for bodies that are not JSON at all, + // such as a reverse proxy's "upstream connect error". + if (root.ValueKind == JsonValueKind.String) + { + var only = root.GetString(); + + return string.IsNullOrWhiteSpace(only) ? fallback : only; + } + + if (root.ValueKind == JsonValueKind.Object) + { + // problem+json errors[] names the individual fields that were refused, which is more + // use than the summary in detail, so it wins when present. + var fieldErrors = ReadFieldErrors(root); + + if (fieldErrors is not null) + { + return fieldErrors; + } + + foreach (var name in MessageProperties) + { + if (root.TryGetProperty(name, out var value) + && value.ValueKind == JsonValueKind.String + && !string.IsNullOrWhiteSpace(value.GetString())) + { + return value.GetString()!; + } + } + + return fallback; + } + } + catch (JsonException) + { + // Not JSON; the raw body is still better than nothing, as long as it is short. + } + + return string.IsNullOrWhiteSpace(body) || body.Length > MaxRawBodyLength ? fallback : body; + } + + /// + /// Renders errors[] as "field: what was wrong", or null when the response carries no usable + /// field-level detail. + /// + /// + /// location arrives as a path into the submitted body (body.min_iv); only the last + /// segment means anything to someone looking at a form, so the rest is dropped. + /// + private static string? ReadFieldErrors(JsonElement root) + { + if (!root.TryGetProperty("errors", out var errors) || errors.ValueKind != JsonValueKind.Array) + { + return null; + } + + var rendered = new List(); + + foreach (var entry in errors.EnumerateArray()) + { + if (entry.ValueKind != JsonValueKind.Object) + { + continue; + } + + var message = ReadNonEmptyString(entry, "message"); + + if (message is null) + { + continue; + } + + var field = LastSegment(ReadNonEmptyString(entry, "location")); + rendered.Add(field is null ? message : $"{field}: {message}"); + } + + if (rendered.Count == 0) + { + return null; + } + + var shown = new StringBuilder(string.Join("; ", rendered.Take(MaxFieldErrors))); + + if (rendered.Count > MaxFieldErrors) + { + shown.Append(CultureInfo.InvariantCulture, $" (and {rendered.Count - MaxFieldErrors} more)"); + } + + return shown.ToString(); + } + + /// Returns the segment after the last dot, or null when there is nothing to return. + private static string? LastSegment(string? location) + { + if (location is null) + { + return null; + } + + var lastDot = location.LastIndexOf('.'); + var segment = lastDot >= 0 ? location[(lastDot + 1)..] : location; + + return string.IsNullOrWhiteSpace(segment) ? null : segment; + } + + private static string? ReadNonEmptyString(JsonElement element, string name) + => element.TryGetProperty(name, out var value) + && value.ValueKind == JsonValueKind.String + && !string.IsNullOrWhiteSpace(value.GetString()) + ? value.GetString() + : null; +} diff --git a/Core/Pgan.PoracleWebNet.Core.Services/PoracleTrackingProxy.cs b/Core/Pgan.PoracleWebNet.Core.Services/PoracleTrackingProxy.cs index 989e9c66..2325789b 100644 --- a/Core/Pgan.PoracleWebNet.Core.Services/PoracleTrackingProxy.cs +++ b/Core/Pgan.PoracleWebNet.Core.Services/PoracleTrackingProxy.cs @@ -67,7 +67,11 @@ public async Task 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)); } @@ -163,32 +167,8 @@ public async Task ReloadStateAsync() } /// Reads whatever explanation PoracleNG returned, falling back to something honest. - private static async Task 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; - } + private static Task ExtractMessageAsync(HttpResponseMessage response) + => PoracleErrorMessage.ExtractAsync(response, "Poracle rejected the alarm."); private HttpRequestMessage CreateRequest(HttpMethod method, string url) { diff --git a/Tests/Pgan.PoracleWebNet.Tests/Services/PoracleErrorMessageTests.cs b/Tests/Pgan.PoracleWebNet.Tests/Services/PoracleErrorMessageTests.cs new file mode 100644 index 00000000..24245d8a --- /dev/null +++ b/Tests/Pgan.PoracleWebNet.Tests/Services/PoracleErrorMessageTests.cs @@ -0,0 +1,225 @@ +using System.Net; +using System.Net.Http; +using System.Text; +using Pgan.PoracleWebNet.Core.Services; + +namespace Pgan.PoracleWebNet.Tests.Services; + +/// +/// PoracleNG 5.2.1 replaced {status, message} error bodies with RFC 9457 problem+json. Both +/// shapes 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. +/// +public class PoracleErrorMessageTests +{ + private const string Fallback = "Poracle rejected the alarm."; + + private static Task ExtractAsync(string body, string contentType = "application/json") + { + var response = new HttpResponseMessage(HttpStatusCode.UnprocessableEntity) + { + Content = new StringContent(body, Encoding.UTF8, contentType) + }; + + return PoracleErrorMessage.ExtractAsync(response, Fallback); + } + + // ────────────────────────────────────────────────────────────── + // problem+json (PoracleNG 5.2.1) + // ────────────────────────────────────────────────────────────── + + [Fact] + public async Task ReadsDetailFromProblemJson() + { + var message = await ExtractAsync( + /*lang=json,strict*/ """{"title":"Unprocessable Entity","status":422,"detail":"validation failed"}"""); + + Assert.Equal("validation failed", message); + } + + /// + /// errors[] names the fields that were refused, which is more use than the summary in detail, so it + /// wins when both are present. + /// + [Fact] + public async Task PrefersFieldErrorsOverDetail() + { + var message = await ExtractAsync( + /*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); + } + + /// + /// location arrives as a path into the submitted body. Only the last segment means anything to + /// someone looking at the form that produced it. + /// + [Fact] + public async Task TrimsTheBodyPrefixFromAFieldLocation() + { + var message = await ExtractAsync( + /*lang=json,strict*/ """{"errors":[{"message":"required","location":"body.pokemon_id"}]}"""); + + Assert.StartsWith("pokemon_id:", message, StringComparison.Ordinal); + } + + [Fact] + public async Task SummarisesWhenMoreThanThreeFieldsWereRefused() + { + var message = await ExtractAsync( + /*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); + } + + /// title is the status phrase, so it answers only when nothing better is on the wire. + [Fact] + public async Task FallsBackToTitleWhenThereIsNoDetail() + { + var message = await ExtractAsync(/*lang=json,strict*/ """{"title":"Unprocessable Entity","status":422}"""); + + Assert.Equal("Unprocessable Entity", message); + } + + /// + /// 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. + /// + [Fact] + public async Task NeverReturnsANumericStatusAsTheMessage() + { + var message = await ExtractAsync(/*lang=json,strict*/ """{"status":422}"""); + + Assert.Equal(Fallback, message); + } + + /// A server that sends problem+json without setting the header is still understood. + [Fact] + public async Task DoesNotDependOnTheContentTypeHeader() + { + var message = await ExtractAsync( + /*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) + // ────────────────────────────────────────────────────────────── + + /// + /// 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. + /// + [Fact] + public async Task ReadsALiveUnknownPropertyRejection() + { + var message = await ExtractAsync( + /*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 async Task ReadsALiveTypeMismatchRejection() + { + var message = await ExtractAsync( + /*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); + } + + /// A semantic refusal carries no errors[], so detail is the whole explanation. + [Fact] + public async Task ReadsALiveSemanticRejectionWithNoFieldErrors() + { + var message = await ExtractAsync( + /*lang=json,strict*/ """{"title":"Unprocessable Entity","status":422,"detail":"unknown display_type"}""", + "application/problem+json"); + + Assert.Equal("unknown display_type", message); + } + + /// + /// 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. + /// + [Fact] + public async Task ReadsALive521V1Rejection() + { + var message = await ExtractAsync( + /*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 async Task StillReadsTheLegacyMessageProperty() + { + var message = await ExtractAsync( + /*lang=json,strict*/ """{"status":"error","message":"Grunt type mandatory"}"""); + + Assert.Equal("Grunt type mandatory", message); + } + + [Fact] + public async Task StillReadsTheLegacyErrorProperty() + { + var message = await ExtractAsync(/*lang=json,strict*/ """{"error":"An unexpected error occurred."}"""); + + Assert.Equal("An unexpected error occurred.", message); + } + + /// A string status was the last resort in the old shape and still is. + [Fact] + public async Task StillReadsAStringStatusWhenItIsAllThereIs() + { + var message = await ExtractAsync(/*lang=json,strict*/ """{"status":"Grunt type mandatory"}"""); + + Assert.Equal("Grunt type mandatory", message); + } + + // ────────────────────────────────────────────────────────────── + // Neither shape + // ────────────────────────────────────────────────────────────── + + [Fact] + public async Task EchoesAShortNonJsonBody() + { + var message = await ExtractAsync("upstream connect error", "text/plain"); + + Assert.Equal("upstream connect error", message); + } + + [Fact] + public async Task FallsBackWhenTheBodyIsTooLongToShow() + { + var message = await ExtractAsync(new string('x', 301), "text/plain"); + + Assert.Equal(Fallback, message); + } + + [Fact] + public async Task FallsBackWhenThereIsNoBody() + { + var message = await ExtractAsync(string.Empty, "text/plain"); + + Assert.Equal(Fallback, message); + } +} diff --git a/Tests/Pgan.PoracleWebNet.Tests/Services/PoracleTrackingProxyTests.cs b/Tests/Pgan.PoracleWebNet.Tests/Services/PoracleTrackingProxyTests.cs index 77ea39a1..6931fff2 100644 --- a/Tests/Pgan.PoracleWebNet.Tests/Services/PoracleTrackingProxyTests.cs +++ b/Tests/Pgan.PoracleWebNet.Tests/Services/PoracleTrackingProxyTests.cs @@ -215,6 +215,42 @@ public async Task CreateAsyncSurfacesPoracleNgsOwnExplanationForABadRequest() Assert.Contains("Invalid level", ex.Message, StringComparison.Ordinal); } + /// + /// 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. + /// + [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( + () => sut.CreateAsync("invasion", "user1", body)); + Assert.Contains("unknown display_type", ex.Message, StringComparison.Ordinal); + } + + /// The field-level detail problem+json carries reaches the user, not just the 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( + () => sut.CreateAsync("pokemon", "user1", body)); + Assert.Contains("min_iv", ex.Message, StringComparison.Ordinal); + } + // ────────────────────────────────────────────────────────────── // DeleteByUidAsync // ────────────────────────────────────────────────────────────── From 35218796ce6b54ffaece282bf461746f14e358da Mon Sep 17 00:00:00 2001 From: hokiepokedad2 <38219945+hokiepokedad2@users.noreply.github.com> Date: Mon, 24 Aug 2026 12:59:33 -0400 Subject: [PATCH 2/2] refactor(poracle): one error reader for both Poracle surfaces #805 landed PoracleProblemDetails on the v2 write path while this branch was open, and it is the same reader this branch introduced as PoracleErrorMessage -- same design, same 300-character cap, same fallback string, arrived at independently. Merging both would have shipped two implementations of one idea into develop, which is how one of them ends up being the one nobody updates. PoracleErrorMessage is deleted. The v1 create path now calls PoracleProblemDetails.Describe, so v1 and v2 explain a refusal the same way and there is one place to fix a wording bug. The 400-or-422 widening stays: v1 does not emit 422 today, but the two paths agreeing costs nothing. Two changes to the surviving reader: Field errors are capped at three with a count of the rest. A body refused on a dozen fields rendered a dozen clauses into a snackbar. A string `status` is no longer read as the message. This branch had it in the precedence list; PoracleProblemDetails does not, and its omission is right -- v1's status is the word "error", so surfacing it is worse than the honest fallback. The test asserting the old behaviour is dropped rather than carried, because it was asserting something undesirable. The seventeen cases move onto Describe, including the four captured verbatim from a live 5.2.1 server. PoracleProblemDetails stays internal; the test project reaches it through InternalsVisibleTo, matching the API project. Refs #803 --- CHANGELOG.md | 2 +- .../Pgan.PoracleWebNet.Core.Services.csproj | 7 + .../PoracleErrorMessage.cs | 162 ------------------ .../PoracleProblemDetails.cs | 21 +++ .../PoracleTrackingProxy.cs | 14 +- ... => PoracleProblemDetailsDescribeTests.cs} | 96 +++++------ 6 files changed, 83 insertions(+), 219 deletions(-) delete mode 100644 Core/Pgan.PoracleWebNet.Core.Services/PoracleErrorMessage.cs rename Tests/Pgan.PoracleWebNet.Tests/Services/{PoracleErrorMessageTests.cs => PoracleProblemDetailsDescribeTests.cs} (67%) diff --git a/CHANGELOG.md b/CHANGELOG.md index a2fc1987..47232a85 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed -- **Poracle's newer error format is understood, ahead of the move to its v2 API.** PoracleNG 5.2.1 answers its new `/api/v2` with RFC 9457 problem+json and refuses invalid input with 422 rather than 400. Nothing changes for anyone today: every call this site makes is on Poracle's frozen v1 surface, which still answers in the older format on 5.2.1 -- verified against a 5.2.1 server rather than assumed. What changes is that the reader now understands both, and where the newer format names the individual field it refused, that field is named to the user instead of a general "validation failed". Without this the first endpoint moved to v2 would have reported every refusal as a server fault ([#803](https://github.com/PGAN-Dev/PoracleWeb.NET/issues/803)). +- **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)). diff --git a/Core/Pgan.PoracleWebNet.Core.Services/Pgan.PoracleWebNet.Core.Services.csproj b/Core/Pgan.PoracleWebNet.Core.Services/Pgan.PoracleWebNet.Core.Services.csproj index fea15703..539dcc59 100644 --- a/Core/Pgan.PoracleWebNet.Core.Services/Pgan.PoracleWebNet.Core.Services.csproj +++ b/Core/Pgan.PoracleWebNet.Core.Services/Pgan.PoracleWebNet.Core.Services.csproj @@ -6,6 +6,13 @@ enable + + + + + diff --git a/Core/Pgan.PoracleWebNet.Core.Services/PoracleErrorMessage.cs b/Core/Pgan.PoracleWebNet.Core.Services/PoracleErrorMessage.cs deleted file mode 100644 index aba308fa..00000000 --- a/Core/Pgan.PoracleWebNet.Core.Services/PoracleErrorMessage.cs +++ /dev/null @@ -1,162 +0,0 @@ -using System.Globalization; -using System.Net.Http; -using System.Text; -using System.Text.Json; - -namespace Pgan.PoracleWebNet.Core.Services; - -/// -/// Reads whatever explanation Poracle returned with a refusal, across both of its wire formats. -/// -/// -/// -/// PoracleNG 5.2.1 replaced the {status, message} error body with RFC 9457 -/// application/problem+json -- {type, title, status, detail, errors[]} -- and moved a -/// number of 400s to 422. Both shapes are read here by field name rather than by branching on a -/// version or a content type: the names do not collide, so one reader serves both, and a reverse -/// proxy that rewrites Content-Type cannot cost a user their error message. -/// -/// -/// status is read last and only when it is a string. The old shape used it for a word; the new -/// one uses it for the HTTP code, and answering a user with "422" explains nothing. title is -/// last of the problem+json fields for the same reason -- it is the status phrase -/// ("Unprocessable Entity"), not a description of what was wrong. -/// -/// -public static class PoracleErrorMessage -{ - /// Longest body echoed verbatim when nothing could be parsed out of it. - private const int MaxRawBodyLength = 300; - - /// Most specific first. See the remarks on for the ordering. - private static readonly string[] MessageProperties = ["detail", "message", "error", "status", "title"]; - - /// Field-level entries quoted before the list is summarised, to keep the message readable. - private const int MaxFieldErrors = 3; - - /// - /// Returns Poracle's own explanation for a failed response, or when it - /// did not give one. - /// - public static async Task ExtractAsync(HttpResponseMessage response, string fallback) - { - var body = await response.Content.ReadAsStringAsync(); - - try - { - var root = JsonDocument.Parse(body).RootElement; - - // A body that parsed is Poracle talking to us in a shape we know. If we cannot find an - // explanation in it we fall back rather than echoing it: raw JSON in a snackbar tells a user - // less than a plain sentence does. The echo below is for bodies that are not JSON at all, - // such as a reverse proxy's "upstream connect error". - if (root.ValueKind == JsonValueKind.String) - { - var only = root.GetString(); - - return string.IsNullOrWhiteSpace(only) ? fallback : only; - } - - if (root.ValueKind == JsonValueKind.Object) - { - // problem+json errors[] names the individual fields that were refused, which is more - // use than the summary in detail, so it wins when present. - var fieldErrors = ReadFieldErrors(root); - - if (fieldErrors is not null) - { - return fieldErrors; - } - - foreach (var name in MessageProperties) - { - if (root.TryGetProperty(name, out var value) - && value.ValueKind == JsonValueKind.String - && !string.IsNullOrWhiteSpace(value.GetString())) - { - return value.GetString()!; - } - } - - return fallback; - } - } - catch (JsonException) - { - // Not JSON; the raw body is still better than nothing, as long as it is short. - } - - return string.IsNullOrWhiteSpace(body) || body.Length > MaxRawBodyLength ? fallback : body; - } - - /// - /// Renders errors[] as "field: what was wrong", or null when the response carries no usable - /// field-level detail. - /// - /// - /// location arrives as a path into the submitted body (body.min_iv); only the last - /// segment means anything to someone looking at a form, so the rest is dropped. - /// - private static string? ReadFieldErrors(JsonElement root) - { - if (!root.TryGetProperty("errors", out var errors) || errors.ValueKind != JsonValueKind.Array) - { - return null; - } - - var rendered = new List(); - - foreach (var entry in errors.EnumerateArray()) - { - if (entry.ValueKind != JsonValueKind.Object) - { - continue; - } - - var message = ReadNonEmptyString(entry, "message"); - - if (message is null) - { - continue; - } - - var field = LastSegment(ReadNonEmptyString(entry, "location")); - rendered.Add(field is null ? message : $"{field}: {message}"); - } - - if (rendered.Count == 0) - { - return null; - } - - var shown = new StringBuilder(string.Join("; ", rendered.Take(MaxFieldErrors))); - - if (rendered.Count > MaxFieldErrors) - { - shown.Append(CultureInfo.InvariantCulture, $" (and {rendered.Count - MaxFieldErrors} more)"); - } - - return shown.ToString(); - } - - /// Returns the segment after the last dot, or null when there is nothing to return. - private static string? LastSegment(string? location) - { - if (location is null) - { - return null; - } - - var lastDot = location.LastIndexOf('.'); - var segment = lastDot >= 0 ? location[(lastDot + 1)..] : location; - - return string.IsNullOrWhiteSpace(segment) ? null : segment; - } - - private static string? ReadNonEmptyString(JsonElement element, string name) - => element.TryGetProperty(name, out var value) - && value.ValueKind == JsonValueKind.String - && !string.IsNullOrWhiteSpace(value.GetString()) - ? value.GetString() - : null; -} diff --git a/Core/Pgan.PoracleWebNet.Core.Services/PoracleProblemDetails.cs b/Core/Pgan.PoracleWebNet.Core.Services/PoracleProblemDetails.cs index 71c520cc..b206d285 100644 --- a/Core/Pgan.PoracleWebNet.Core.Services/PoracleProblemDetails.cs +++ b/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 /// What to say when PoracleNG refused and explained nothing usable. public const string Unexplained = "Poracle rejected the alarm."; + /// Field errors quoted before the rest are summarised, to keep the message readable. + private const int MaxFieldErrors = 3; + /// /// 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; diff --git a/Core/Pgan.PoracleWebNet.Core.Services/PoracleTrackingProxy.cs b/Core/Pgan.PoracleWebNet.Core.Services/PoracleTrackingProxy.cs index 1df155b2..6643b56d 100644 --- a/Core/Pgan.PoracleWebNet.Core.Services/PoracleTrackingProxy.cs +++ b/Core/Pgan.PoracleWebNet.Core.Services/PoracleTrackingProxy.cs @@ -353,9 +353,17 @@ private async Task ServerCarriesV2Async() return null; } - /// Reads whatever explanation PoracleNG returned, falling back to something honest. - private static Task ExtractMessageAsync(HttpResponseMessage response) - => PoracleErrorMessage.ExtractAsync(response, "Poracle rejected the alarm."); + /// + /// Reads whatever explanation PoracleNG returned, falling back to something honest. + /// + /// + /// The same reader the v2 path uses. v1 and v2 disagree about the shape of an error -- v1 answers + /// {message, status} 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. + /// + private static async Task ExtractMessageAsync(HttpResponseMessage response) + => PoracleProblemDetails.Describe(await response.Content.ReadAsStringAsync()); private HttpRequestMessage CreateRequest(HttpMethod method, string url) { diff --git a/Tests/Pgan.PoracleWebNet.Tests/Services/PoracleErrorMessageTests.cs b/Tests/Pgan.PoracleWebNet.Tests/Services/PoracleProblemDetailsDescribeTests.cs similarity index 67% rename from Tests/Pgan.PoracleWebNet.Tests/Services/PoracleErrorMessageTests.cs rename to Tests/Pgan.PoracleWebNet.Tests/Services/PoracleProblemDetailsDescribeTests.cs index 24245d8a..e520f099 100644 --- a/Tests/Pgan.PoracleWebNet.Tests/Services/PoracleErrorMessageTests.cs +++ b/Tests/Pgan.PoracleWebNet.Tests/Services/PoracleProblemDetailsDescribeTests.cs @@ -6,22 +6,21 @@ namespace Pgan.PoracleWebNet.Tests.Services; /// -/// PoracleNG 5.2.1 replaced {status, message} error bodies with RFC 9457 problem+json. Both -/// shapes 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. +/// PoracleNG 5.2.1 replaced {status, message} 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. /// -public class PoracleErrorMessageTests +public class PoracleProblemDetailsDescribeTests { - private const string Fallback = "Poracle rejected the alarm."; + private const string Fallback = PoracleProblemDetails.Unexplained; - private static Task ExtractAsync(string body, string contentType = "application/json") + private static string Describe(string body, string contentType = "application/json") { - var response = new HttpResponseMessage(HttpStatusCode.UnprocessableEntity) - { - Content = new StringContent(body, Encoding.UTF8, contentType) - }; - - return PoracleErrorMessage.ExtractAsync(response, Fallback); + _ = contentType; // Describe reads the body; the header never decides the shape. + return PoracleProblemDetails.Describe(body); } // ────────────────────────────────────────────────────────────── @@ -29,9 +28,9 @@ private static Task ExtractAsync(string body, string contentType = "appl // ────────────────────────────────────────────────────────────── [Fact] - public async Task ReadsDetailFromProblemJson() + public void ReadsDetailFromProblemJson() { - var message = await ExtractAsync( + var message = Describe( /*lang=json,strict*/ """{"title":"Unprocessable Entity","status":422,"detail":"validation failed"}"""); Assert.Equal("validation failed", message); @@ -42,9 +41,9 @@ public async Task ReadsDetailFromProblemJson() /// wins when both are present. /// [Fact] - public async Task PrefersFieldErrorsOverDetail() + public void PrefersFieldErrorsOverDetail() { - var message = await ExtractAsync( + 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}]} @@ -58,18 +57,18 @@ public async Task PrefersFieldErrorsOverDetail() /// someone looking at the form that produced it. /// [Fact] - public async Task TrimsTheBodyPrefixFromAFieldLocation() + public void TrimsTheBodyPrefixFromAFieldLocation() { - var message = await ExtractAsync( + var message = Describe( /*lang=json,strict*/ """{"errors":[{"message":"required","location":"body.pokemon_id"}]}"""); Assert.StartsWith("pokemon_id:", message, StringComparison.Ordinal); } [Fact] - public async Task SummarisesWhenMoreThanThreeFieldsWereRefused() + public void SummarisesWhenMoreThanThreeFieldsWereRefused() { - var message = await ExtractAsync( + 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"}, @@ -83,9 +82,9 @@ public async Task SummarisesWhenMoreThanThreeFieldsWereRefused() /// title is the status phrase, so it answers only when nothing better is on the wire. [Fact] - public async Task FallsBackToTitleWhenThereIsNoDetail() + public void FallsBackToTitleWhenThereIsNoDetail() { - var message = await ExtractAsync(/*lang=json,strict*/ """{"title":"Unprocessable Entity","status":422}"""); + var message = Describe(/*lang=json,strict*/ """{"title":"Unprocessable Entity","status":422}"""); Assert.Equal("Unprocessable Entity", message); } @@ -95,18 +94,18 @@ public async Task FallsBackToTitleWhenThereIsNoDetail() /// "422" explains nothing, so a numeric status is never the message. /// [Fact] - public async Task NeverReturnsANumericStatusAsTheMessage() + public void NeverReturnsANumericStatusAsTheMessage() { - var message = await ExtractAsync(/*lang=json,strict*/ """{"status":422}"""); + var message = Describe(/*lang=json,strict*/ """{"status":422}"""); Assert.Equal(Fallback, message); } /// A server that sends problem+json without setting the header is still understood. [Fact] - public async Task DoesNotDependOnTheContentTypeHeader() + public void DoesNotDependOnTheContentTypeHeader() { - var message = await ExtractAsync( + var message = Describe( /*lang=json,strict*/ """{"detail":"validation failed"}""", "application/problem+json"); @@ -122,9 +121,9 @@ public async Task DoesNotDependOnTheContentTypeHeader() /// tracking bodies are arrays of rules; only the trailing segment is shown to the user. /// [Fact] - public async Task ReadsALiveUnknownPropertyRejection() + public void ReadsALiveUnknownPropertyRejection() { - var message = await ExtractAsync( + 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"); @@ -132,9 +131,9 @@ public async Task ReadsALiveUnknownPropertyRejection() } [Fact] - public async Task ReadsALiveTypeMismatchRejection() + public void ReadsALiveTypeMismatchRejection() { - var message = await ExtractAsync( + 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"); @@ -143,9 +142,9 @@ public async Task ReadsALiveTypeMismatchRejection() /// A semantic refusal carries no errors[], so detail is the whole explanation. [Fact] - public async Task ReadsALiveSemanticRejectionWithNoFieldErrors() + public void ReadsALiveSemanticRejectionWithNoFieldErrors() { - var message = await ExtractAsync( + var message = Describe( /*lang=json,strict*/ """{"title":"Unprocessable Entity","status":422,"detail":"unknown display_type"}""", "application/problem+json"); @@ -157,9 +156,9 @@ public async Task ReadsALiveSemanticRejectionWithNoFieldErrors() /// produced the problem+json above. This is why the reader must keep both. /// [Fact] - public async Task ReadsALive521V1Rejection() + public void ReadsALive521V1Rejection() { - var message = await ExtractAsync( + var message = Describe( /*lang=json,strict*/ """{"message":"Grunt type mandatory","status":"error"}"""); Assert.Equal("Grunt type mandatory", message); @@ -170,55 +169,46 @@ public async Task ReadsALive521V1Rejection() // ────────────────────────────────────────────────────────────── [Fact] - public async Task StillReadsTheLegacyMessageProperty() + public void StillReadsTheLegacyMessageProperty() { - var message = await ExtractAsync( + var message = Describe( /*lang=json,strict*/ """{"status":"error","message":"Grunt type mandatory"}"""); Assert.Equal("Grunt type mandatory", message); } [Fact] - public async Task StillReadsTheLegacyErrorProperty() + public void StillReadsTheLegacyErrorProperty() { - var message = await ExtractAsync(/*lang=json,strict*/ """{"error":"An unexpected error occurred."}"""); + var message = Describe(/*lang=json,strict*/ """{"error":"An unexpected error occurred."}"""); Assert.Equal("An unexpected error occurred.", message); } - /// A string status was the last resort in the old shape and still is. - [Fact] - public async Task StillReadsAStringStatusWhenItIsAllThereIs() - { - var message = await ExtractAsync(/*lang=json,strict*/ """{"status":"Grunt type mandatory"}"""); - - Assert.Equal("Grunt type mandatory", message); - } - // ────────────────────────────────────────────────────────────── // Neither shape // ────────────────────────────────────────────────────────────── [Fact] - public async Task EchoesAShortNonJsonBody() + public void EchoesAShortNonJsonBody() { - var message = await ExtractAsync("upstream connect error", "text/plain"); + var message = Describe("upstream connect error", "text/plain"); Assert.Equal("upstream connect error", message); } [Fact] - public async Task FallsBackWhenTheBodyIsTooLongToShow() + public void FallsBackWhenTheBodyIsTooLongToShow() { - var message = await ExtractAsync(new string('x', 301), "text/plain"); + var message = Describe(new string('x', 301), "text/plain"); Assert.Equal(Fallback, message); } [Fact] - public async Task FallsBackWhenThereIsNoBody() + public void FallsBackWhenThereIsNoBody() { - var message = await ExtractAsync(string.Empty, "text/plain"); + var message = Describe(string.Empty, "text/plain"); Assert.Equal(Fallback, message); }