From 2783e26b67aa711829a52897a6773c534b978a08 Mon Sep 17 00:00:00 2001 From: Aditya Singh Date: Wed, 5 Aug 2026 09:13:33 -0700 Subject: [PATCH] Report a login failure from the CLI as a readable error Creating a dataset without valid credentials produced two unhelpful outcomes. The server error array was stringified whole, so the one line that matters was buried in a server side stacktrace, and a response carrying a null createDataset field made createDataset resolve to undefined, which only failed later as "Path must be a string, received undefined" from the path join in the upload command. Keep only the GraphQL messages, raise a LoginError naming the command that fixes it when the failure is an authentication one, and reject a response that carries no accession number. The upload command now prints that message and stops instead of throwing an uncaught error. Fixes #3523 --- cli/src/commands/upload.ts | 6 +- cli/src/graphq.test.ts | 133 +++++++++++++++++++++++++++++++++++++ cli/src/graphq.ts | 67 +++++++++++++++---- 3 files changed, 191 insertions(+), 15 deletions(-) create mode 100644 cli/src/graphq.test.ts diff --git a/cli/src/commands/upload.ts b/cli/src/commands/upload.ts index 1f590716d4..1a1567f67e 100644 --- a/cli/src/commands/upload.ts +++ b/cli/src/commands/upload.ts @@ -10,7 +10,7 @@ import type { CommandOptions } from "@cliffy/command" import { getRepoAccess } from "./git-credential.ts" import { readConfig } from "../config.ts" import { createDatasetAffirmed } from "./create-dataset.ts" -import { CreateDatasetAffirmedError } from "../error.ts" +import { CreateDatasetAffirmedError, LoginError } from "../error.ts" import validatorConfig from "../validator-config.json" with { type: "json" } async function getRepoDir(url: URL): Promise { @@ -108,7 +108,9 @@ export async function uploadAction( try { datasetId = await createDatasetAffirmed(options) } catch (err) { - if (err instanceof CreateDatasetAffirmedError) { + if ( + err instanceof CreateDatasetAffirmedError || err instanceof LoginError + ) { console.log(err.message) return } diff --git a/cli/src/graphq.test.ts b/cli/src/graphq.test.ts new file mode 100644 index 0000000000..0ce78f5d2e --- /dev/null +++ b/cli/src/graphq.test.ts @@ -0,0 +1,133 @@ +import { assertEquals, assertRejects } from "@std/assert" +import { join } from "@std/path" +import { createDataset, getLatestSnapshotVersion } from "./graphq.ts" +import { LoginError, ResponseError } from "./error.ts" +import { mockFetch } from "./tests/fetch-stub.ts" + +// A trimmed down copy of what the server returns when the request carries no +// usable credentials. The real payload also includes a long server stacktrace. +const notLoggedInErrors = [ + { + message: "You must be logged in to create a dataset.", + locations: [{ line: 3, column: 3 }], + path: ["createDataset"], + extensions: { + code: "INTERNAL_SERVER_ERROR", + stacktrace: ["Error: You must be logged in to create a dataset."], + }, + }, +] + +/** + * Run a test body with the CLI config pointed at a throwaway directory + * holding a valid looking API key + * @param fn The test body to run + */ +async function withStubbedConfig(fn: () => Promise) { + const configHome = await Deno.makeTempDir() + await Deno.mkdir(join(configHome, "openneuro"), { recursive: true }) + await Deno.writeTextFile( + join(configHome, "openneuro", "config.json"), + JSON.stringify({ "https://openneuro.org": "an-api-key" }), + ) + const previousConfigHome = Deno.env.get("XDG_CONFIG_HOME") + const previousUrl = Deno.env.get("OPENNEURO_URL") + Deno.env.set("XDG_CONFIG_HOME", configHome) + Deno.env.delete("OPENNEURO_URL") + try { + await fn() + } finally { + if (previousConfigHome === undefined) { + Deno.env.delete("XDG_CONFIG_HOME") + } else { + Deno.env.set("XDG_CONFIG_HOME", previousConfigHome) + } + if (previousUrl !== undefined) { + Deno.env.set("OPENNEURO_URL", previousUrl) + } + await Deno.remove(configHome, { recursive: true }) + } +} + +/** + * Run a test body with fetch answering every request with one JSON body + * @param body The response body to return + * @param fn The test body to run + */ +async function withResponse(body: unknown, fn: () => Promise) { + const fetchStub = mockFetch(new Response(JSON.stringify(body))) + try { + await fn() + } finally { + fetchStub.restore() + } +} + +Deno.test("createDataset() returns the new accession number", async () => { + await withStubbedConfig(async () => { + await withResponse( + { data: { createDataset: { id: "ds000001" } } }, + async () => { + assertEquals(await createDataset(true, false), "ds000001") + }, + ) + }) +}) + +Deno.test("createDataset() rejects a response with no accession number", async () => { + await withStubbedConfig(async () => { + // The server can answer with a null field and no top level error. The + // resulting undefined accession number used to travel all the way to the + // path join in the upload command and fail there instead. + await withResponse({ data: { createDataset: null } }, async () => { + await assertRejects( + () => createDataset(true, false), + ResponseError, + "The server did not return an accession number for the new dataset.", + ) + }) + }) +}) + +Deno.test("createDataset() reports missing credentials as a LoginError", async () => { + await withStubbedConfig(async () => { + await withResponse({ errors: notLoggedInErrors }, async () => { + const error = await assertRejects( + () => createDataset(true, false), + LoginError, + "You must be logged in to create a dataset.", + ) + // The message has to say what to do next, not just what went wrong + assertEquals(error.message.includes("openneuro login"), true) + // The server stacktrace is noise for someone running a command + assertEquals(error.message.includes("stacktrace"), false) + }) + }) +}) + +Deno.test("createDataset() keeps only the messages for other failures", async () => { + await withStubbedConfig(async () => { + await withResponse( + { errors: [{ message: "Dataset does not exist" }] }, + async () => { + const error = await assertRejects( + () => createDataset(true, false), + ResponseError, + ) + assertEquals(error.message, "Dataset does not exist") + }, + ) + }) +}) + +Deno.test("getLatestSnapshotVersion() keeps only the error messages", async () => { + await withStubbedConfig(async () => { + await withResponse({ errors: notLoggedInErrors }, async () => { + const error = await assertRejects( + () => getLatestSnapshotVersion("ds000001"), + LoginError, + ) + assertEquals(error.message.includes("stacktrace"), false) + }) + }) +}) diff --git a/cli/src/graphq.ts b/cli/src/graphq.ts index c9d8000020..422c9b18f2 100644 --- a/cli/src/graphq.ts +++ b/cli/src/graphq.ts @@ -3,7 +3,45 @@ */ import { getConfig } from "./config.ts" -import { QueryError, ResponseError } from "./error.ts" +import { LoginError, QueryError, ResponseError } from "./error.ts" + +interface GraphQLError { + message: string + locations?: { line: number; column: number }[] + path?: string[] + extensions?: unknown +} + +/** + * Patterns that identify a GraphQL error caused by missing credentials + */ +const authenticationErrorPattern = + /logged in|log in|not authori[sz]ed|unauthori[sz]ed|authentication/i + +/** + * Throw the most useful error available for a failed GraphQL response + * + * GraphQL reports failures as an array of objects that carry a server side + * stack trace. Printing the whole array buries the one line that matters, so + * only the messages are kept, and a failure caused by missing credentials is + * reported as a LoginError explaining how to fix it. + * @param errors The errors array from a GraphQL response + */ +function throwGraphQLError(errors: GraphQLError[]): never { + const message = errors + .map((error) => error?.message) + .filter((message) => typeof message === "string" && message.length > 0) + .join("\n") + if (!message) { + throw new ResponseError(JSON.stringify(errors)) + } + if (authenticationErrorPattern.test(message)) { + throw new LoginError( + `${message}\nRun \`openneuro login\` to authenticate. If you have already logged in, your API key may no longer be valid, so generate a new one and run \`openneuro login\` again.`, + ) + } + throw new ResponseError(message) +} function request(query: string, variables = {}): Promise { const config = getConfig() @@ -29,14 +67,9 @@ interface CreateDatasetMutationResponse { data?: { createDataset: { id: string - } + } | null } - errors?: { - message: string - locations: { line: number; column: number }[] - path: string[] - extensions: unknown - }[] + errors?: GraphQLError[] } /** @@ -55,13 +88,21 @@ export async function createDataset( }) const body: CreateDatasetMutationResponse = await res.json() if (body.errors) { - throw new ResponseError(JSON.stringify(body.errors)) + throwGraphQLError(body.errors) } - if (body.data) { - return body?.data?.createDataset?.id - } else { + if (!body.data) { throw new QueryError("Invalid response") } + const datasetId = body.data.createDataset?.id + if (!datasetId) { + // An undefined accession number is used to build the local repository + // path, so it fails much later with a path error that says nothing about + // the request that actually failed. + throw new ResponseError( + "The server did not return an accession number for the new dataset.", + ) + } + return datasetId } const prepareUploadMutation = ` @@ -107,7 +148,7 @@ export async function getLatestSnapshotVersion(datasetId: string) { const res = await request(query, { datasetId }) const body = await res.json() if (body.errors) { - throw new ResponseError(JSON.stringify(body.errors)) + throwGraphQLError(body.errors) } if (body.data) { return body.data.dataset.latestSnapshot.tag