fix(server): accept parentIssueId alias in GET /issues (#4032)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Agents coordinate through the server API. They find sub-tasks by filtering the company issues list by parent. > - `GET /api/companies/:companyId/issues` accepts `?parentId=`. Many callers send `?parentIssueId=` instead, which the handler never read. > - The mismatch is silent. The filter is dropped and the full company list comes back, so agents fetch everything and filter client-side. Issue #3846 reports this. > - `parentIssueId` is not an arbitrary spelling. It is the field name the wakeup payloads in this same route file already use, so callers expect it. > - This pull request accepts `parentIssueId` as an alias for `parentId` at the route boundary, on both the issues list and `issues/count`. > - The benefit is that parent filtering works for both spellings, and the list and its count cannot disagree. ## Linked Issues or Issue Description Fixes #3846 Related: #3870 proposes the same alias for the list route. ## What Changed - `server/src/routes/issues.ts`: `listFilters.parentId` in `GET /companies/:companyId/issues` now reads `req.query.parentId ?? req.query.parentIssueId`. - `server/src/routes/issues.ts`: `blockedCountFilters.parentId` in `GET /companies/:companyId/issues/count` reads the same alias, so the list and its count agree. - `server/src/__tests__/issues-parent-id-alias.test.ts`: new regression test for alias resolution, precedence, and absence. ## Verification - Run `pnpm run test:run -- server/src/__tests__/issues-parent-id-alias.test.ts`. - The test covers four query shapes: `?parentId=`, `?parentIssueId=`, both present (short form wins), and neither present (filter unset). - Existing callers are unaffected. The UI client `ui/src/api/issues.ts` only sets `parentId`. Nullish coalescing falls back only when the primary key is absent. - The service layer applies the filter with `if (filters?.parentId)` in `server/src/services/issues.ts`. This pull request does not change it. ## Risks - Low risk. The change only widens accepted query input. Both spellings resolve, and the short form still wins. - `?parentId=` with an empty value stays falsy and unfiltered, exactly as before. - This route has no validation middleware, and these list filters are not in the published OpenAPI surface. No contract needs an update. ## Model Used - Claude Opus 5 (`claude-opus-5`), extended thinking with tool use, run by the maintainer's triage agent. It rebased the original commit onto current `master`, extended the alias to `issues/count`, and wrote the regression test. @scokeepa authored the original one-line route change. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change and contains no internal Paperclip ticket id or instance-derived details - [ ] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [ ] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: josangmun <cmeia.ai02@cmeia.co.kr> Co-authored-by: Andrew Aymeloglu <aaymeloglu@gmail.com>
This commit is contained in:
scokeepa
GitHub
parent
2495e29f7f
commit
2ebc236b08
|
|
@ -0,0 +1,141 @@
|
|||
import { randomUUID } from "node:crypto";
|
||||
import express, { type Router } from "express";
|
||||
import { isNotNull } from "drizzle-orm";
|
||||
import {
|
||||
companies,
|
||||
companyMemberships,
|
||||
createDb,
|
||||
issues,
|
||||
principalPermissionGrants,
|
||||
} from "@paperclipai/db";
|
||||
import { afterAll, afterEach, beforeAll, describe } from "vitest";
|
||||
import { errorHandler } from "../../middleware/index.js";
|
||||
import { ensureHumanRoleDefaultGrants } from "../../services/principal-access-compatibility.js";
|
||||
import {
|
||||
getEmbeddedPostgresTestSupport,
|
||||
startEmbeddedPostgresTestDatabase,
|
||||
} from "./embedded-postgres.js";
|
||||
|
||||
type Db = ReturnType<typeof createDb>;
|
||||
|
||||
const embeddedPostgresSupport = await getEmbeddedPostgresTestSupport();
|
||||
|
||||
/** `describe` on hosts that can run embedded Postgres, `describe.skip` elsewhere. */
|
||||
export const describeEmbeddedPostgres = embeddedPostgresSupport.supported ? describe : describe.skip;
|
||||
|
||||
export type EmbeddedPostgresContext = { readonly db: Db };
|
||||
|
||||
/**
|
||||
* Registers the embedded Postgres lifecycle for the enclosing `describe`. Read
|
||||
* `.db` from inside a test; it is unavailable until `beforeAll` has run.
|
||||
*/
|
||||
export function useEmbeddedPostgres(
|
||||
prefix: string,
|
||||
opts: { resetEach?: (db: Db) => Promise<void> } = {},
|
||||
): EmbeddedPostgresContext {
|
||||
let db: Db | null = null;
|
||||
let tempDb: Awaited<ReturnType<typeof startEmbeddedPostgresTestDatabase>> | null = null;
|
||||
|
||||
beforeAll(async () => {
|
||||
tempDb = await startEmbeddedPostgresTestDatabase(prefix);
|
||||
db = createDb(tempDb.connectionString);
|
||||
}, 20_000);
|
||||
|
||||
if (opts.resetEach) {
|
||||
const resetEach = opts.resetEach;
|
||||
afterEach(async () => {
|
||||
if (db) await resetEach(db);
|
||||
});
|
||||
}
|
||||
|
||||
afterAll(async () => {
|
||||
await tempDb?.cleanup();
|
||||
tempDb = null;
|
||||
db = null;
|
||||
});
|
||||
|
||||
return {
|
||||
get db() {
|
||||
if (!db) throw new Error("embedded Postgres is not started yet — read .db inside a test");
|
||||
return db;
|
||||
},
|
||||
};
|
||||
}
|
||||
|
||||
export type BoardActor = {
|
||||
type: "board";
|
||||
source: string;
|
||||
userId: string;
|
||||
companyIds: string[];
|
||||
memberships: { companyId: string; membershipRole: string; status: string }[];
|
||||
isInstanceAdmin: boolean;
|
||||
};
|
||||
|
||||
export type SeededCompany = { companyId: string; userId: string; actor: BoardActor };
|
||||
|
||||
/** Creates a company whose board user holds company-scoped read and write grants. */
|
||||
export async function seedCompanyWithBoardAccess(db: Db, name: string): Promise<SeededCompany> {
|
||||
const companyId = randomUUID();
|
||||
const userId = `user-${randomUUID()}`;
|
||||
await db.insert(companies).values({
|
||||
id: companyId,
|
||||
name: `${name} ${companyId}`,
|
||||
issuePrefix: `T${companyId.replaceAll("-", "").slice(0, 6).toUpperCase()}`,
|
||||
requireBoardApprovalForNewAgents: false,
|
||||
});
|
||||
await db.insert(companyMemberships).values({
|
||||
companyId,
|
||||
principalType: "user",
|
||||
principalId: userId,
|
||||
status: "active",
|
||||
membershipRole: "owner",
|
||||
updatedAt: new Date(),
|
||||
});
|
||||
await ensureHumanRoleDefaultGrants(db, {
|
||||
companyId,
|
||||
principalId: userId,
|
||||
membershipRole: "owner",
|
||||
grantedByUserId: null,
|
||||
});
|
||||
return {
|
||||
companyId,
|
||||
userId,
|
||||
actor: {
|
||||
type: "board",
|
||||
source: "session",
|
||||
userId,
|
||||
companyIds: [companyId],
|
||||
memberships: [{ companyId, membershipRole: "owner", status: "active" }],
|
||||
isInstanceAdmin: false,
|
||||
},
|
||||
};
|
||||
}
|
||||
|
||||
type RouterFactory = (db: Db, storage: never) => Router;
|
||||
|
||||
/** Mounts route modules under `/api` behind a fixed actor, with the real error handler. */
|
||||
export function routeApp(db: Db, actor: BoardActor, ...routerFactories: RouterFactory[]) {
|
||||
const app = express();
|
||||
app.use(express.json());
|
||||
app.use((req, _res, next) => {
|
||||
(req as any).actor = actor;
|
||||
next();
|
||||
});
|
||||
for (const factory of routerFactories) {
|
||||
app.use("/api", factory(db, {} as never));
|
||||
}
|
||||
app.use(errorHandler);
|
||||
return app;
|
||||
}
|
||||
|
||||
/**
|
||||
* Clears the company and issue fixtures seeded by `seedCompanyWithBoardAccess`.
|
||||
* Suites that seed other tables need their own reset.
|
||||
*/
|
||||
export async function resetCompanyIssueFixtures(db: Db) {
|
||||
await db.delete(issues).where(isNotNull(issues.parentId));
|
||||
await db.delete(issues);
|
||||
await db.delete(principalPermissionGrants);
|
||||
await db.delete(companyMemberships);
|
||||
await db.delete(companies);
|
||||
}
|
||||
|
|
@ -1,74 +1,59 @@
|
|||
import express from "express";
|
||||
import { randomUUID } from "node:crypto";
|
||||
import request from "supertest";
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { parseStatusFilter } from "../services/issues.ts";
|
||||
import { expect, it } from "vitest";
|
||||
import { issues } from "@paperclipai/db";
|
||||
import { issueRoutes } from "../routes/issues.js";
|
||||
import {
|
||||
describeEmbeddedPostgres,
|
||||
resetCompanyIssueFixtures,
|
||||
routeApp,
|
||||
seedCompanyWithBoardAccess,
|
||||
useEmbeddedPostgres,
|
||||
} from "./helpers/route-test-harness.js";
|
||||
|
||||
/**
|
||||
* Regression test for https://github.com/paperclipai/paperclip/issues/4628
|
||||
*
|
||||
* Stands up a minimal Express app whose handler mirrors the parsing path
|
||||
* `server/src/routes/issues.ts:957-958` uses to forward `req.query.status`
|
||||
* into `issueService.list({ status })`. Verifies the four shapes Express's
|
||||
* default `qs` parser can produce all normalize correctly and none crash:
|
||||
*
|
||||
* 1. Single value — `?status=todo`
|
||||
* 2. Comma-separated — `?status=todo,in_progress` (legacy)
|
||||
* 3. Repeated key (array) — `?status=todo&status=in_progress` (the bug)
|
||||
* 4. Mixed array + CSV — `?status=todo,in_progress&status=done`
|
||||
*
|
||||
* Pre-fix, case 3 returned HTTP 500 with `TypeError: filters.status.split is
|
||||
* not a function`. We don't spin embedded-postgres here; the helper itself is
|
||||
* unit-tested in `parse-status-filter.test.ts`, and the route→helper contract
|
||||
* is what regresses if anyone reverts the fix.
|
||||
* Regression coverage for https://github.com/paperclipai/paperclip/issues/4628.
|
||||
* Express's `qs` parser hands the list route either a string or an array for
|
||||
* `?status=`, and the route normalizes both shapes.
|
||||
*/
|
||||
|
||||
function buildApp() {
|
||||
const app = express();
|
||||
app.use(express.json());
|
||||
app.get("/api/companies/:companyId/issues", (req, res) => {
|
||||
// Mirror the cast at routes/issues.ts:958 exactly. Pre-fix this said
|
||||
// `as string | undefined` and the cast was a lie when qs returned an array.
|
||||
const statusInput = req.query.status as string | string[] | undefined;
|
||||
const statuses = parseStatusFilter(statusInput);
|
||||
res.status(200).json({ statuses });
|
||||
});
|
||||
return app;
|
||||
}
|
||||
|
||||
describe("issue list status query parsing", () => {
|
||||
it("accepts a single ?status=todo and returns one normalized status", async () => {
|
||||
const res = await request(buildApp()).get("/api/companies/c1/issues?status=todo");
|
||||
expect(res.status).toBe(200);
|
||||
expect(res.body).toEqual({ statuses: ["todo"] });
|
||||
describeEmbeddedPostgres("issue list status query parsing", () => {
|
||||
const ctx = useEmbeddedPostgres("paperclip-issues-list-query-parsing-", {
|
||||
resetEach: resetCompanyIssueFixtures,
|
||||
});
|
||||
|
||||
it("accepts comma-separated ?status=todo,in_progress (preserves legacy CSV)", async () => {
|
||||
const res = await request(buildApp()).get(
|
||||
"/api/companies/c1/issues?status=todo,in_progress",
|
||||
);
|
||||
expect(res.status).toBe(200);
|
||||
expect(res.body).toEqual({ statuses: ["todo", "in_progress"] });
|
||||
async function listStatuses(query: string) {
|
||||
const company = await seedCompanyWithBoardAccess(ctx.db, "Status parsing");
|
||||
const companyId = company.companyId;
|
||||
await ctx.db.insert(issues).values([
|
||||
{ id: randomUUID(), companyId, title: "Todo", status: "todo", priority: "medium" },
|
||||
{ id: randomUUID(), companyId, title: "In progress", status: "in_progress", priority: "medium" },
|
||||
{ id: randomUUID(), companyId, title: "Done", status: "done", priority: "medium" },
|
||||
]);
|
||||
const res = await request(routeApp(ctx.db, company.actor, issueRoutes))
|
||||
.get(`/api/companies/${companyId}/issues${query}`)
|
||||
.expect(200);
|
||||
return (res.body as { status: string }[]).map((issue) => issue.status).sort();
|
||||
}
|
||||
|
||||
it("accepts a single ?status=todo", async () => {
|
||||
expect(await listStatuses("?status=todo")).toEqual(["todo"]);
|
||||
});
|
||||
|
||||
it("accepts repeated ?status=todo&status=in_progress without crashing (the bug fix)", async () => {
|
||||
const res = await request(buildApp()).get(
|
||||
"/api/companies/c1/issues?status=todo&status=in_progress",
|
||||
);
|
||||
expect(res.status).toBe(200);
|
||||
expect(res.body).toEqual({ statuses: ["todo", "in_progress"] });
|
||||
it("accepts comma-separated ?status=todo,in_progress", async () => {
|
||||
expect(await listStatuses("?status=todo,in_progress")).toEqual(["in_progress", "todo"]);
|
||||
});
|
||||
|
||||
it("accepts mixed array + CSV ?status=todo,in_progress&status=done", async () => {
|
||||
const res = await request(buildApp()).get(
|
||||
"/api/companies/c1/issues?status=todo,in_progress&status=done",
|
||||
);
|
||||
expect(res.status).toBe(200);
|
||||
expect(res.body).toEqual({ statuses: ["todo", "in_progress", "done"] });
|
||||
it("accepts repeated ?status=todo&status=in_progress", async () => {
|
||||
expect(await listStatuses("?status=todo&status=in_progress")).toEqual(["in_progress", "todo"]);
|
||||
});
|
||||
|
||||
it("accepts no ?status param and returns an empty status filter", async () => {
|
||||
const res = await request(buildApp()).get("/api/companies/c1/issues");
|
||||
expect(res.status).toBe(200);
|
||||
expect(res.body).toEqual({ statuses: [] });
|
||||
it("accepts mixed array and CSV ?status=todo,in_progress&status=done", async () => {
|
||||
expect(await listStatuses("?status=todo,in_progress&status=done"))
|
||||
.toEqual(["done", "in_progress", "todo"]);
|
||||
});
|
||||
|
||||
it("returns every status when ?status is absent", async () => {
|
||||
expect(await listStatuses("")).toEqual(["done", "in_progress", "todo"]);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -0,0 +1,116 @@
|
|||
import { randomUUID } from "node:crypto";
|
||||
import request from "supertest";
|
||||
import { expect, it } from "vitest";
|
||||
import { issues } from "@paperclipai/db";
|
||||
import { issueRoutes } from "../routes/issues.js";
|
||||
import {
|
||||
describeEmbeddedPostgres,
|
||||
resetCompanyIssueFixtures,
|
||||
routeApp,
|
||||
seedCompanyWithBoardAccess,
|
||||
useEmbeddedPostgres,
|
||||
} from "./helpers/route-test-harness.js";
|
||||
|
||||
describeEmbeddedPostgres("issue list parentIssueId query alias", () => {
|
||||
const ctx = useEmbeddedPostgres("paperclip-issues-parent-id-alias-", {
|
||||
resetEach: resetCompanyIssueFixtures,
|
||||
});
|
||||
|
||||
async function seed() {
|
||||
const company = await seedCompanyWithBoardAccess(ctx.db, "Parent alias");
|
||||
const companyId = company.companyId;
|
||||
const parentId = randomUUID();
|
||||
const otherParentId = randomUUID();
|
||||
const childId = randomUUID();
|
||||
const blockedChildId = randomUUID();
|
||||
|
||||
await ctx.db.insert(issues).values([
|
||||
{ id: parentId, companyId, title: "Parent", status: "todo", priority: "medium" },
|
||||
{ id: otherParentId, companyId, title: "Other parent", status: "todo", priority: "medium" },
|
||||
]);
|
||||
await ctx.db.insert(issues).values([
|
||||
{ id: childId, companyId, title: "Child", status: "todo", priority: "medium", parentId },
|
||||
{ id: blockedChildId, companyId, title: "Blocked child", status: "blocked", priority: "medium", parentId },
|
||||
{
|
||||
id: randomUUID(),
|
||||
companyId,
|
||||
title: "Other child",
|
||||
status: "todo",
|
||||
priority: "medium",
|
||||
parentId: otherParentId,
|
||||
},
|
||||
{
|
||||
id: randomUUID(),
|
||||
companyId,
|
||||
title: "Blocked other child",
|
||||
status: "blocked",
|
||||
priority: "medium",
|
||||
parentId: otherParentId,
|
||||
},
|
||||
]);
|
||||
|
||||
return { ...company, parentId, otherParentId, childId, blockedChildId };
|
||||
}
|
||||
|
||||
type Seeded = Awaited<ReturnType<typeof seed>>;
|
||||
|
||||
function appFor(seeded: Seeded) {
|
||||
return routeApp(ctx.db, seeded.actor, issueRoutes);
|
||||
}
|
||||
|
||||
async function listIds(seeded: Seeded, query: Record<string, string>) {
|
||||
const res = await request(appFor(seeded))
|
||||
.get(`/api/companies/${seeded.companyId}/issues`)
|
||||
.query(query)
|
||||
.expect(200);
|
||||
return (res.body as { id: string }[]).map((issue) => issue.id).sort();
|
||||
}
|
||||
|
||||
async function blockedCount(seeded: Seeded, query: Record<string, string>) {
|
||||
const res = await request(appFor(seeded))
|
||||
.get(`/api/companies/${seeded.companyId}/issues/count`)
|
||||
.query({ attention: "blocked", ...query })
|
||||
.expect(200);
|
||||
return res.body as { count: number };
|
||||
}
|
||||
|
||||
function childrenOfParent(seeded: Seeded) {
|
||||
return [seeded.childId, seeded.blockedChildId].sort();
|
||||
}
|
||||
|
||||
it("filters children by ?parentId=", async () => {
|
||||
const seeded = await seed();
|
||||
expect(await listIds(seeded, { parentId: seeded.parentId })).toEqual(childrenOfParent(seeded));
|
||||
});
|
||||
|
||||
it("filters children by ?parentIssueId=", async () => {
|
||||
const seeded = await seed();
|
||||
expect(await listIds(seeded, { parentIssueId: seeded.parentId })).toEqual(childrenOfParent(seeded));
|
||||
});
|
||||
|
||||
it("prefers ?parentId= when both spellings are present", async () => {
|
||||
const seeded = await seed();
|
||||
const ids = await listIds(seeded, {
|
||||
parentId: seeded.parentId,
|
||||
parentIssueId: seeded.otherParentId,
|
||||
});
|
||||
expect(ids).toEqual(childrenOfParent(seeded));
|
||||
});
|
||||
|
||||
it("returns the whole company list when neither spelling is present", async () => {
|
||||
const seeded = await seed();
|
||||
expect(await listIds(seeded, {})).toHaveLength(6);
|
||||
});
|
||||
|
||||
it("filters the blocked count by ?parentIssueId=", async () => {
|
||||
const seeded = await seed();
|
||||
expect(await blockedCount(seeded, { parentIssueId: seeded.parentId })).toEqual({ count: 1 });
|
||||
});
|
||||
|
||||
it("returns the same blocked count for both spellings", async () => {
|
||||
const seeded = await seed();
|
||||
const byShortForm = await blockedCount(seeded, { parentId: seeded.parentId });
|
||||
const byAlias = await blockedCount(seeded, { parentIssueId: seeded.parentId });
|
||||
expect(byAlias).toEqual(byShortForm);
|
||||
});
|
||||
});
|
||||
|
|
@ -5210,7 +5210,7 @@ export function issueRoutes(
|
|||
projectId: req.query.projectId as string | undefined,
|
||||
workspaceId: req.query.workspaceId as string | undefined,
|
||||
executionWorkspaceId: req.query.executionWorkspaceId as string | undefined,
|
||||
parentId: req.query.parentId as string | undefined,
|
||||
parentId: (req.query.parentId ?? req.query.parentIssueId) as string | undefined,
|
||||
descendantOf: req.query.descendantOf as string | undefined,
|
||||
labelId: req.query.labelId as string | undefined,
|
||||
originKind: req.query.originKind as string | undefined,
|
||||
|
|
@ -5402,7 +5402,7 @@ export function issueRoutes(
|
|||
projectId: req.query.projectId as string | undefined,
|
||||
workspaceId: req.query.workspaceId as string | undefined,
|
||||
executionWorkspaceId: req.query.executionWorkspaceId as string | undefined,
|
||||
parentId: req.query.parentId as string | undefined,
|
||||
parentId: (req.query.parentId ?? req.query.parentIssueId) as string | undefined,
|
||||
descendantOf: req.query.descendantOf as string | undefined,
|
||||
labelId: req.query.labelId as string | undefined,
|
||||
originKind: req.query.originKind as string | undefined,
|
||||
|
|
|
|||
Loading…
Reference in New Issue