Report a login failure from the CLI as a readable error

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<string> {
@@ -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
       }

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<void>) {
+  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<void>) {
+  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)
+    })
+  })
+})

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<Response> {
   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