Send POST and PUT parameters in the request body #241

.changeset/params-in-request-body.md

@@ -0,0 +1,11 @@
+---
+"trello-cli": patch
+---
+
+Fix `card:update`'s `--description` flag, `card:create`'s `--description` flag,
+`card:comment`'s `--text` flag, `board:create`'s `--description` flag, and the
+interactive TUI's multiline description editor to send parameters in the JSON
+request body instead of the URL query string for POST and PUT requests. This
+resolves HTTP 414 errors that occurred when long text (around 900 Chinese
+characters or more) pushed the percent-encoded URL past Trello's ~8KB limit,
+causing updates to fail entirely.

.changeset/pin-oclif-core.md

@@ -0,0 +1,8 @@
+---
+"trello-cli": patch
+---
+
+Pin `@oclif/core` to `4.13.3` instead of a caret range. Starting with
+`4.13.4`, the package ships a module format incompatible with the
+current build setup; `4.13.3` is the last version confirmed to work
+correctly.

packages/trello-cli/package.json

@@ -20,7 +20,7 @@
     "/oclif.manifest.json"
   ],
   "dependencies": {
-    "@oclif/core": "^4.8.0",
+    "@oclif/core": "4.13.3",
     "@oclif/plugin-autocomplete": "^3.2.40",
     "@oclif/plugin-help": "^6.2.37",
     "@oclif/plugin-plugins": "^5.4.55",

packages/trello-cli/src/BaseCommand.ts

@@ -3,6 +3,7 @@ import Config from "@trello-cli/config";
 import Cache from "@trello-cli/cache";
 import * as path from "path";
 import { TrelloClient } from "trello.js";
+import { sendParamsInBody } from "./paramsInBody";
 import { parse } from "json2csv";
 import { run } from "./index";
 
@@ -70,10 +71,12 @@ export abstract class BaseCommand<T extends typeof Command> extends Command {
       const token = await this.trelloConfig.getToken();
       const appKey = await this.trelloConfig.getApiKey();
 
-      this.client = new TrelloClient({
-        key: appKey,
-        token: token,
-      });
+      this.client = sendParamsInBody(
+        new TrelloClient({
+          key: appKey,
+          token: token,
+        })
+      );
 
       this.cache = new Cache(
         path.join(this.configDir, this.profile),

packages/trello-cli/src/commands/interactive.tsx

@@ -3,6 +3,7 @@ import Config from "@trello-cli/config";
 import Cache from "@trello-cli/cache";
 import * as path from "path";
 import { TrelloClient } from "trello.js";
+import { sendParamsInBody } from "../paramsInBody";
 import { render } from "ink";
 import React from "react";
 import { App } from "../tui/App";
@@ -58,10 +59,12 @@ export default class Interactive extends Command {
       return;
     }
 
-    const client = new TrelloClient({
-      key: appKey,
-      token: token,
-    });
+    const client = sendParamsInBody(
+      new TrelloClient({
+        key: appKey,
+        token: token,
+      })
+    );
 
     const cache = new Cache(
       path.join(configDir, profile),

packages/trello-cli/src/paramsInBody.ts

@@ -0,0 +1,61 @@
+import { RequestConfig, TrelloClient } from "trello.js";
+
+const METHODS_THAT_ACCEPT_A_BODY = ["post", "put"];
+
+// trello.js serialises every parameter into the query string, and Trello rejects
+// URLs longer than roughly 8KB with a 414. Percent-encoding turns each CJK
+// character into nine, so a description of around 880 Chinese characters is
+// already enough to blow that limit.
+//
+// Trello accepts POST and PUT parameters in a JSON body just as readily, so
+// writes travel in the body instead. Requests that already carry one — the
+// multipart attachment upload, and the endpoints trello.js models with `data` —
+// are left alone.
+//
+// Values keep their native JSON types here rather than being stringified the way
+// the query serialiser would. Arrays, booleans, numbers and date strings were
+// all checked against the API and behave identically. Object-valued parameters
+// (`coordinates`, `cover`) were inconclusive, but no command sends one.
+export function moveParamsIntoBody(config: RequestConfig): RequestConfig {
+  const method = (config.method ?? "get").toLowerCase();
+
+  if (!METHODS_THAT_ACCEPT_A_BODY.includes(method) || config.data !== undefined) {
+    return config;
+  }
+
+  if (!config.params) {
+    return config;
+  }
+
+  // The query serialiser skips empty values, so the body has to as well.
+  const data = Object.fromEntries(
+    Object.entries(config.params).filter(
+      ([, value]) => value !== undefined && value !== null
+    )
+  );
+
+  if (Object.keys(data).length === 0) {
+    return config;
+  }
+
+  return { ...config, params: {}, data };
+}
+
+export function sendParamsInBody(client: TrelloClient): TrelloClient {
+  // sendRequest is overloaded, which a spread call cannot satisfy directly.
+  const sendRequest = client.sendRequest as (
+    this: TrelloClient,
+    config: RequestConfig,
+    ...rest: any[]
+  ) => unknown;
+
+  client.sendRequest = function (
+    this: TrelloClient,
+    config: RequestConfig,
+    ...rest: any[]
+  ) {
+    return sendRequest.call(this, moveParamsIntoBody(config), ...rest);
+  } as typeof client.sendRequest;
+
+  return client;
+}

packages/trello-cli/test/paramsInBody.test.ts

@@ -0,0 +1,149 @@
+import { TrelloClient } from "trello.js";
+import { moveParamsIntoBody, sendParamsInBody } from "../src/paramsInBody";
+
+const longDescription = "這是一段很長的中文描述內容。".repeat(200);
+
+describe("moveParamsIntoBody", () => {
+  it("moves POST params into the body", () => {
+    const config = moveParamsIntoBody({
+      url: "/cards",
+      method: "POST",
+      params: { name: "Card", desc: "Description" },
+    });
+
+    expect(config.params).toEqual({});
+    expect(config.data).toEqual({ name: "Card", desc: "Description" });
+  });
+
+  it("moves PUT params into the body", () => {
+    const config = moveParamsIntoBody({
+      url: "/cards/card123",
+      method: "PUT",
+      params: { desc: "Description" },
+    });
+
+    expect(config.params).toEqual({});
+    expect(config.data).toEqual({ desc: "Description" });
+  });
+
+  it("keeps a long description out of the query string", () => {
+    const config = moveParamsIntoBody({
+      url: "/cards/card123",
+      method: "PUT",
+      params: { desc: longDescription },
+    });
+
+    expect(config.params).toEqual({});
+    expect(config.data).toEqual({ desc: longDescription });
+  });
+
+  it("recognises lowercase methods", () => {
+    const config = moveParamsIntoBody({
+      url: "/cards",
+      method: "post",
+      params: { name: "Card" },
+    });
+
+    expect(config.data).toEqual({ name: "Card" });
+  });
+
+  it("leaves GET params in the query string", () => {
+    const params = { fields: "name" };
+    const config = moveParamsIntoBody({ url: "/cards/card123", method: "GET", params });
+
+    expect(config.params).toBe(params);
+    expect(config.data).toBeUndefined();
+  });
+
+  it("leaves DELETE params in the query string", () => {
+    const params = { value: "label123" };
+    const config = moveParamsIntoBody({ url: "/cards/card123/idLabels", method: "DELETE", params });
+
+    expect(config.params).toBe(params);
+    expect(config.data).toBeUndefined();
+  });
+
+  it("leaves a request that already carries a body alone", () => {
+    const params = { name: "attachment.png" };
+    const data = { pipe: () => undefined };
+    const config = moveParamsIntoBody({
+      url: "/cards/card123/attachments",
+      method: "POST",
+      params,
+      data,
+    });
+
+    expect(config.params).toBe(params);
+    expect(config.data).toBe(data);
+  });
+
+  it("drops undefined and null params, as the query serialiser does", () => {
+    const config = moveParamsIntoBody({
+      url: "/cards/card123",
+      method: "PUT",
+      params: { desc: "Description", name: undefined, due: null, closed: false },
+    });
+
+    expect(config.data).toEqual({ desc: "Description", closed: false });
+  });
+
+  it("leaves a request with no usable params alone", () => {
+    const params = { name: undefined };
+    const config = moveParamsIntoBody({ url: "/cards/card123", method: "PUT", params });
+
+    expect(config.params).toBe(params);
+    expect(config.data).toBeUndefined();
+  });
+
+  it("leaves a request without params alone", () => {
+    const config = moveParamsIntoBody({ url: "/cards/card123", method: "DELETE" });
+
+    expect(config.params).toBeUndefined();
+    expect(config.data).toBeUndefined();
+  });
+
+  it("does not mutate the config it is given", () => {
+    const original = {
+      url: "/cards/card123",
+      method: "PUT",
+      params: { desc: "Description" },
+    };
+    moveParamsIntoBody(original);
+
+    expect(original.params).toEqual({ desc: "Description" });
+    expect(original).not.toHaveProperty("data");
+  });
+});
+
+describe("sendParamsInBody", () => {
+  it("routes sendRequest through the transform and returns its result", async () => {
+    const sendRequest = jest.fn().mockResolvedValue({ id: "card123" });
+    const client = { sendRequest } as unknown as TrelloClient;
+
+    const returned = sendParamsInBody(client);
+    const result = await client.sendRequest({
+      url: "/cards/card123",
+      method: "PUT",
+      params: { desc: longDescription },
+    });
+
+    expect(returned).toBe(client);
+    expect(result).toEqual({ id: "card123" });
+    expect(sendRequest).toHaveBeenCalledTimes(1);
+    expect(sendRequest.mock.calls[0][0]).toMatchObject({
+      params: {},
+      data: { desc: longDescription },
+    });
+  });
+
+  it("passes the callback argument through", async () => {
+    const sendRequest = jest.fn().mockResolvedValue(undefined);
+    const client = { sendRequest } as unknown as TrelloClient;
+    const callback = jest.fn();
+
+    sendParamsInBody(client);
+    await client.sendRequest({ url: "/cards", method: "POST", params: { name: "Card" } }, callback);
+
+    expect(sendRequest.mock.calls[0][1]).toBe(callback);
+  });
+});