Handle plain-text upstream API errors

[PrzemyslawKlys]

Summary

• avoid JSON ApiError parsing for plain-text or non-JSON upstream responses
• preserve the real HTTP status/body for plain-text 503 gateway failures
• align plain-text 400 tests with the existing ValidationException mapping

Verification

• dotnet test SectigoCertificateManager.Tests/SectigoCertificateManager.Tests.csproj --framework net8.0 --filter ApiErrorHandlerTests
• dotnet test SectigoCertificateManager.Tests/SectigoCertificateManager.Tests.csproj --framework net472 --filter ApiErrorHandlerTests

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

[claude]

Code Review

Overall this is a solid, focused fix for a real problem: plain-text gateway errors (e.g. 503 from Istio/Envoy) were previously swallowed into a noisy "Failed to parse ApiError" message. The approach – check Content-Type and body heuristics before attempting JSON deserialization – is pragmatic and correct for the common cases.

---

Bugs / Correctness

1. Empty body with application/json Content-Type will still throw a parse error

In ShouldAttemptJsonParse, the Content-Type check returns true before the empty-body guard:

if (!string.IsNullOrWhiteSpace(mediaType)) {
    if (normalizedMediaType.Contains("json", ...))
        return true;   // <-- returns true even when body is empty/null
}

if (string.IsNullOrWhiteSpace(body)) {
    return false;      // <-- never reached when Content-Type is JSON
}

If a server responds with Content-Type: application/json and an empty body, JsonSerializer.Deserialize is called on "", throws a JsonException, and the caller still gets "Failed to parse ApiError" noise. Reorder the empty-body check to be first:

private static bool ShouldAttemptJsonParse(HttpResponseMessage response, string body) {
    if (string.IsNullOrWhiteSpace(body))
        return false;

    string? mediaType = response.Content.Headers.ContentType?.MediaType;
    if (!string.IsNullOrWhiteSpace(mediaType) &&
        mediaType.Contains("json", StringComparison.OrdinalIgnoreCase))
        return true;

    ReadOnlySpan<char> trimmed = body.AsSpan().TrimStart();
    return !trimmed.IsEmpty && (trimmed[0] == '{' || trimmed[0] == '[');
}

---

Minor Issues

2. Redundant EndsWith("+json") branch

normalizedMediaType.Contains("json", StringComparison.OrdinalIgnoreCase) ||
normalizedMediaType.EndsWith("+json", StringComparison.OrdinalIgnoreCase)

Any string that ends with "+json" already contains "json", so the EndsWith branch is dead code. The Contains check alone is sufficient.

3. The normalizedMediaType local variable is misleading and unnecessary

string normalizedMediaType = mediaType!;

This is just a copy of mediaType – nothing is normalised. The null-forgiving operator (!) is also redundant because IsNullOrWhiteSpace already proved it's non-null. Use mediaType directly or give the variable a clearer name if you want to keep it.

---

Tests

4. No test for the Content-Type-based detection path

The new ShouldAttemptJsonParse has two detection modes – Content-Type header and body-sniffing – but tests only cover the body-sniffing path. Consider adding a test where the response has Content-Type: application/json with a valid JSON body to confirm the Content-Type path correctly opts in to JSON parsing.

5. PlainTextErrorBody_DoesNotAddJsonParseNoise could also assert the ErrorCode

The 503 test verifies the message content but doesn't assert ex.ErrorCode. Since the code sets Code = (ApiErrorCode)(int)response.StatusCode, a quick Assert.Equal((ApiErrorCode)503, ex.ErrorCode) would make the contract explicit, similar to the 400 test.

---

Summary

Severity	Issue
Bug	Empty body + application/json Content-Type still produces parse-error noise
Minor	EndsWith("+json") is dead code
Minor	normalizedMediaType variable name / unnecessary null-forgiving operator
Suggestion	Add Content-Type detection test
Suggestion	Assert ErrorCode in the 503 test

The core logic is sound and the tests for the primary scenarios are correct. Fixing the empty-body edge case would make this fully robust.

[claude]

Code Review

This is a clean, well-targeted fix. The core problem — plain-text gateway responses (like Istio/Envoy 503s) causing a misleading "Failed to parse ApiError" message — is real and the approach to solve it is sound. Overall the PR is in good shape. A few observations below.

---

ApiErrorHandler.cs

ShouldAttemptJsonParse — logic is correct, a couple of edge-case notes:

The ordering matters and works correctly:

1. Empty/whitespace body → skip parse (correct; IsNullOrWhiteSpace catches both "" and " ")
2. Content-Type: application/json or variants (application/problem+json, text/json) → attempt parse
3. Heuristic first-char {/[ check as fallback

One thing to be aware of: the first-char heuristic won't match a top-level JSON string or number response (e.g., "error message" or 404). In practice the Sectigo API always returns an object, so this is fine, but worth a brief comment in the method.

Pre-existing micro-allocation (not introduced here, but worth noting):

error = JsonSerializer.Deserialize<ApiError>(body, new JsonSerializerOptions(JsonSerializerDefaults.Web));

A new JsonSerializerOptions instance is created on every error response. Consider caching it as a static readonly field to avoid repeated allocations — especially relevant if error paths are hit frequently (e.g., polling under load).

---

ApiErrorHandlerTests.cs

EmptyJsonContentTypeBody_DoesNotAddJsonParseNoise — test name is slightly misleading:

The test constructs an empty body with Content-Type: application/json, but the early-exit guard string.IsNullOrWhiteSpace(body) fires before the Content-Type check. So this test is actually validating the empty-body guard, not the JSON Content-Type path specifically. It still passes and the behavior is correct, but a name like EmptyBody_DoesNotAttemptJsonParse would better reflect what's being exercised.

ExceptionMessageTruncatesLongBody change is correct:

Changing the expected type from ApiException to ValidationException is the right fix. Previously, the JSON parse on an all-a body would throw and produce an ApiException(UnknownError), bypassing the status-code dispatch. Now the status code (400 BadRequest) correctly drives the exception type.

Test coverage is good — the four new/updated tests cover the key scenarios (plain-text 400, plain-text 503, empty JSON body, and JSON-typed body with valid ApiError). The ExceptionMessageIncludesStatusCodeAndBody test implicitly covers the first-char heuristic for JSON-like bodies without a Content-Type header.

---

Summary

Area	Assessment
Bug fix correctness	Correct — resolves the plain-text upstream error noise
ShouldAttemptJsonParse design	Clean; heuristic is appropriate for this API
Test coverage	Good — all main code paths exercised
Breaking changes	None
Minor suggestions	Cache JsonSerializerOptions; clarify one test name

Good change overall.