Skip to content

fix: throw better error when ensrainbow fails to serve json #1713

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
shrugs wants to merge 7 commits into
base: main
Choose a base branch
from
+4 −32
Open

fix: throw better error when ensrainbow fails to serve json #1713

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
22592cc
fix: throw better error when ensrainbow fails to serve json
shrugs Mar 4, 2026
9268bbf
fix lint
shrugs Mar 4, 2026
610afc3
fix: streamline error derivation
shrugs Mar 4, 2026
3b8c13b
fix: lint
shrugs Mar 4, 2026
7c8f51d
Merge branch 'main' into fix/ensrainbow-not-found-handling
shrugs Mar 4, 2026
dd8ecdf
fix: revert pnpm exec
shrugs Mar 4, 2026
94f51a5
fix: more accurate ensraibow client config endpoint testing
shrugs Mar 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions apps/ensindexer/src/lib/version-info.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,6 @@ import { existsSync, readdirSync, readFileSync } from "node:fs";
import { dirname, join } from "node:path";
import { fileURLToPath } from "node:url";

import { prettifyError } from "zod/v4";

import type { ENSIndexerVersionInfo, SerializedENSIndexerVersionInfo } from "@ensnode/ensnode-sdk";
import { makeENSIndexerVersionInfoSchema } from "@ensnode/ensnode-sdk/internal";

/**
* Get ENSIndexer version
*/
Expand Down
26 changes: 3 additions & 23 deletions packages/ensrainbow-sdk/src/client.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,30 +317,10 @@ describe("EnsRainbowApiClient", () => {
expect(response).toEqual(configData);
});

it("should throw with server error message when response is not ok", async () => {
mockFetch.mockResolvedValueOnce({
ok: false,
statusText: "Internal Server Error",
json: () =>
Promise.resolve({
error: "Database not ready",
errorCode: 500,
}),
});

await expect(client.config()).rejects.toThrow("Database not ready");
});

it("should throw with fallback message when error body has no error field", async () => {
mockFetch.mockResolvedValueOnce({
ok: false,
statusText: "Service Unavailable",
json: () => Promise.resolve({}),
});
it("should throw with fallback message when error body is not valid JSON", async () => {
mockFetch.mockResolvedValueOnce({ ok: false, statusText: "Not Found" });

await expect(client.config()).rejects.toThrow(
"Failed to fetch ENSRainbow config: Service Unavailable",
);
await expect(client.config()).rejects.toThrow(/Not Found/);
Comment on lines 317 to +323
Copy link

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change deletes the existing tests that ensure JSON error bodies are surfaced (server-provided error field) and that the fallback message is used when the JSON body lacks an error field. If config() is intended to remain compatible with those behaviors (and only add resilience for non-JSON responses), those assertions should remain alongside the new “invalid JSON” test to prevent regressions.

Copilot uses AI. Check for mistakes.
});
Comment on lines +320 to 324
Copy link

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The updated test doesn’t actually cover the “non-JSON error body causes response.json() to throw” case described in the PR. Because the mock response has no json() method, the implementation never attempts JSON parsing, so this would still pass even if a SyntaxError were thrown in a real fetch response.

Suggestion: mock json() to throw a SyntaxError (or return a rejected promise) and assert that client.config() throws the fallback error (and not the SyntaxError).

Copilot uses AI. Check for mistakes.
Comment on lines +320 to 324
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Keep this test, but add one for JSON error-body precedence

This case correctly covers the non-JSON fallback path. Please also add a companion test asserting that when { error: "..." } JSON is returned on non-OK responses, that message is used in the thrown error.

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@packages/ensrainbow-sdk/src/client.test.ts` around lines 320 - 324, Add a new
unit test next to the existing non-JSON fallback test in
packages/ensrainbow-sdk/src/client.test.ts that verifies JSON error-body
precedence: mock the fetch used by the tests (mockFetch.mockResolvedValueOnce)
to return an object with ok: false and a json method that resolves to { error:
"Your error message" } (and statusText if desired), then call client.config()
and assert the promise rejects with an error containing that JSON error string
(e.g., await expect(client.config()).rejects.toThrow(/Your error message/));
mirror the structure and naming of the existing test so it's clear this is the
JSON-precedence companion.

});
});
Expand Down
5 changes: 1 addition & 4 deletions packages/ensrainbow-sdk/src/client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -400,10 +400,7 @@ export class EnsRainbowApiClient implements EnsRainbow.ApiClient {
const response = await fetch(new URL("/v1/config", this.options.endpointUrl));

if (!response.ok) {
const errorData = (await response.json()) as { error?: string; errorCode?: number };
throw new Error(
errorData.error ?? `Failed to fetch ENSRainbow config: ${response.statusText}`,
);
throw new Error(`Failed to fetch ENSRainbow config: ${response.statusText}`);
Copy link

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

config() no longer attempts to read the server-provided JSON error body (e.g. { error: "Database not ready" }), so callers will always get the generic Failed to fetch... message even when the service returns a useful error string. This is a behavioral regression vs the previous implementation and doesn’t match the PR description (try/catch around response.json() for non-JSON responses).

Suggestion: in the !response.ok branch, keep parsing the JSON body inside a try/catch; if it parses and contains a string error message, throw that; if parsing fails (non-JSON), fall back to a message based on response.status/statusText.

Suggested change
throw new Error(`Failed to fetch ENSRainbow config: ${response.statusText}`);
try {
const data: unknown = await response.json();
if (
data &&
typeof data === "object" &&
"error" in data &&
typeof (data as { error?: unknown }).error === "string"
) {
throw new Error((data as { error: string }).error);
}
if (typeof data === "string") {
throw new Error(data);
}
} catch {
// Ignore JSON parsing errors and fall through to generic message
}
const statusText = response.statusText || `status ${response.status}`;
throw new Error(`Failed to fetch ENSRainbow config: ${statusText}`);

Copilot uses AI. Check for mistakes.
}
Comment on lines 402 to 404
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Preserve JSON error details before falling back to statusText

Line 403 now always uses statusText, which drops useful server error payloads for valid JSON error responses. This is a behavior regression from the previous error path and makes failures harder to diagnose.

Suggested fix 
   if (!response.ok) {
-      throw new Error(`Failed to fetch ENSRainbow config: ${response.statusText}`);
+      let errorMessage = response.statusText;
+      try {
+        const data = (await response.json()) as { error?: string };
+        if (typeof data.error === "string" && data.error.length > 0) {
+          errorMessage = data.error;
+        }
+      } catch (error) {
+        if (!(error instanceof SyntaxError)) {
+          throw error;
+        }
+      }
+      throw new Error(`Failed to fetch ENSRainbow config: ${errorMessage}`);
   }
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@packages/ensrainbow-sdk/src/client.ts` around lines 402 - 404, The current
error throw uses only response.statusText and discards any JSON error payload;
update the error path that throws for non-ok responses to read the response body
first and include its details: attempt to parse await response.json() (falling
back to await response.text() if parsing fails), extract a useful message (e.g.,
body.message || body.error || the raw text or JSON stringified body), and then
throw a new Error that includes response.status, response.statusText, and the
extracted error details; reference the existing response variable and the throw
that currently reads `Failed to fetch ENSRainbow config: ${response.statusText}`
and replace it with the richer message as described.


return response.json() as Promise<EnsRainbow.ENSRainbowPublicConfig>;
Expand Down