From a063ba7d4fb568a74284894494dfbe2fdd1beeab Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Wed, 16 Sep 2026 13:21:42 +0200 Subject: [PATCH 1/2] Close the malformed-trait pass in the two generators that still had it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "All six generators refuse an unrecognised style by name" was true for a well-formed trait and false for a malformed one, in two of the six. Both holes are the same shape as the one this branch already closed in Swift, which is the reason to distrust the claim rather than the code: the fix was applied where a reviewer pointed, not everywhere it belonged. Kotlin gated the refusal on `as? JsonObject`, which answers null for a present-but-non-object trait. A bare `"x-basecamp-pagination": "page"` skipped the refusal entirely and generated an unpaginated method for a spec that plainly said "page" — the exact silent pass the refusal exists to close. Presence is now tested before the cast, as Swift does, and a non-primitive `style` is refused by name instead of throwing the cast error `jsonPrimitive` would. TypeScript gated it on truthiness, so `false`, `0` and `""` were exempt and read as unpaginated. It now narrows through `unknown`: absent and a literal null are unpaginated, anything else is declared, and a declared trait that is not an object has no style to accept. The key is read off the same narrowed value, so it cannot outlive the style check. Neither generator changes its answer for any operation in the spec: all 61 paginated operations carry a well-formed `{"style": "link"}`, and both drift gates report no drift. Tests in both languages, and they fail when the guard is reverted — I checked, because the last test on this branch that re-implemented the predicate it was guarding stayed green when the guard was deleted. `parseOperation` is exported from the TypeScript generator to reach it; the Kotlin test drives OperationParser against an in-memory spec. Co-Authored-By: Claude Opus 5 (1M context) --- .../basecamp/sdk/generator/OperationParser.kt | 17 +- .../sdk/generator/PaginationStyleTest.kt | 152 ++++++++++++++++++ typescript/scripts/generate-services.ts | 24 ++- .../tests/generator/pagination-style.test.ts | 87 ++++++++++ 4 files changed, 272 insertions(+), 8 deletions(-) create mode 100644 kotlin/generator/src/test/kotlin/com/basecamp/sdk/generator/PaginationStyleTest.kt create mode 100644 typescript/tests/generator/pagination-style.test.ts diff --git a/kotlin/generator/src/main/kotlin/com/basecamp/sdk/generator/OperationParser.kt b/kotlin/generator/src/main/kotlin/com/basecamp/sdk/generator/OperationParser.kt index b148105c83..a5bfc302dd 100644 --- a/kotlin/generator/src/main/kotlin/com/basecamp/sdk/generator/OperationParser.kt +++ b/kotlin/generator/src/main/kotlin/com/basecamp/sdk/generator/OperationParser.kt @@ -204,12 +204,23 @@ class OperationParser(private val api: OpenApiParser) { // `?.jsonObject` throws on JsonNull, which is not Kotlin null -- a literal // "x-basecamp-pagination": null would crash here where the other five // generators read it as unpaginated. Narrow to a real object first. - val paginationExtension = operation["x-basecamp-pagination"] as? JsonObject - val paginationStyle = paginationExtension?.get("style")?.jsonPrimitive?.content + // + // Presence is tested BEFORE the cast, exactly as Swift does. Gating the + // refusal on `as? JsonObject` answers null for a present-but-non-object + // trait -- a bare "x-basecamp-pagination": "page" -- and skips the refusal + // entirely, which is the silent pass this whole branch exists to close. + // Absent and a literal null are unpaginated; anything else is declared, + // and a declared trait that is not an object has no style to accept. + val paginationValue = operation["x-basecamp-pagination"] + val paginationDeclared = paginationValue != null && paginationValue !is JsonNull + val paginationExtension = paginationValue as? JsonObject + // A non-primitive `style` becomes null here and is refused by name below, + // rather than throwing the confusing cast error `jsonPrimitive` would. + val paginationStyle = (paginationExtension?.get("style") as? JsonPrimitive)?.contentOrNull // Only "link" and "cursor" are implemented. Anything else -- a typo, or // the "page" style the trait used to advertise -- must fail loudly: read // as "not paginated" it would silently ship a method that never walks. - require(paginationExtension == null || paginationStyle in setOf("link", "cursor")) { + require(!paginationDeclared || paginationStyle in setOf("link", "cursor")) { "$operationId: unsupported pagination style $paginationStyle (expected \"link\" or \"cursor\")" } val hasPagination = paginationStyle == "link" diff --git a/kotlin/generator/src/test/kotlin/com/basecamp/sdk/generator/PaginationStyleTest.kt b/kotlin/generator/src/test/kotlin/com/basecamp/sdk/generator/PaginationStyleTest.kt new file mode 100644 index 0000000000..39dace8be2 --- /dev/null +++ b/kotlin/generator/src/test/kotlin/com/basecamp/sdk/generator/PaginationStyleTest.kt @@ -0,0 +1,152 @@ +package com.basecamp.sdk.generator + +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertFailsWith +import kotlin.test.assertNull +import kotlinx.serialization.json.* + +/** + * Pins Kotlin's refusal of a pagination style it does not implement. + * + * `hasPagination` keys off `style == "link"`, which makes an unrecognised value + * MORE dangerous than the old presence check was: read as "not paginated" it + * ships a method that never walks, silently, with nothing downstream to catch + * it. So every unimplemented spelling has to fail generation by name. + * + * The malformed-trait cases below are the ones that actually bite. Gating the + * refusal on `as? JsonObject` answers Kotlin null for a present-but-non-object + * trait — a bare `"x-basecamp-pagination": "page"` — and skips the refusal + * entirely, which is the exact silent pass the refusal exists to close. Swift + * had the same hole through `as? [String: Any]` and closed it by testing + * presence before the cast; these pin that Kotlin does the same. + */ +class PaginationStyleTest { + + private val api = OpenApiParser( + buildJsonObject { + putJsonObject("components") { putJsonObject("schemas") {} } + putJsonObject("paths") {} + } + ) + + private fun parse(pagination: JsonElement?): ParsedOperation { + val operation = buildJsonObject { + put("operationId", "ListWidgets") + if (pagination != null) put("x-basecamp-pagination", pagination) + } + return OperationParser(api).parseOperation("/widgets", "get", operation) + } + + private fun styleFailure(pagination: JsonElement?): String = + assertFailsWith { parse(pagination) }.message ?: "" + + @Test + fun `the link style paginates and keeps its key`() { + val parsed = parse( + buildJsonObject { + put("style", "link") + put("key", "widgets") + } + ) + + assertEquals(true, parsed.hasPagination) + assertEquals("widgets", parsed.paginationKey) + } + + @Test + fun `the cursor style does not paginate and withholds its key`() { + // The key is withheld at the source rather than gated at each consumer: + // findUnderlyingEntitySchema unwraps an envelope whenever the key is set + // and never consults hasPagination, so a cursor operation carrying a key + // would be typed as the item under it instead of the envelope the wire + // actually sends — wrong public return type, and it would not decode. + val parsed = parse( + buildJsonObject { + put("style", "cursor") + put("key", "events") + } + ) + + assertEquals(false, parsed.hasPagination) + assertNull(parsed.paginationKey) + } + + @Test + fun `an absent trait is unpaginated`() { + val parsed = parse(null) + + assertEquals(false, parsed.hasPagination) + assertNull(parsed.paginationKey) + } + + @Test + fun `a literal null trait is unpaginated, as it is in the other five generators`() { + // `?.jsonObject` throws on JsonNull, which is not Kotlin null. Kotlin was + // once the only generator that crashed here where the rest read absent. + val parsed = parse(JsonNull) + + assertEquals(false, parsed.hasPagination) + assertNull(parsed.paginationKey) + } + + @Test + fun `the retired page style is refused by name`() { + assertEquals( + "ListWidgets: unsupported pagination style page (expected \"link\" or \"cursor\")", + styleFailure(buildJsonObject { put("style", "page") }) + ) + } + + @Test + fun `a typo is refused rather than read as unpaginated`() { + assertEquals( + "ListWidgets: unsupported pagination style linkk (expected \"link\" or \"cursor\")", + styleFailure(buildJsonObject { put("style", "linkk") }) + ) + } + + @Test + fun `the style match is case sensitive`() { + assertEquals( + "ListWidgets: unsupported pagination style Link (expected \"link\" or \"cursor\")", + styleFailure(buildJsonObject { put("style", "Link") }) + ) + } + + @Test + fun `a declared trait with no style at all is refused`() { + assertEquals( + "ListWidgets: unsupported pagination style null (expected \"link\" or \"cursor\")", + styleFailure(buildJsonObject { put("maxPageSize", 50) }) + ) + } + + @Test + fun `a bare string trait is refused, not read as unpaginated`() { + // The hole: `as? JsonObject` answers null here, so a presence check + // written against the CAST would exempt this and ship an unpaginated + // method for a spec that plainly said "page". + styleFailure(JsonPrimitive("page")) + } + + @Test + fun `an array trait is refused`() { + styleFailure(buildJsonArray { add("link") }) + } + + @Test + fun `a boolean trait is refused`() { + styleFailure(JsonPrimitive(false)) + } + + @Test + fun `a numeric trait is refused`() { + styleFailure(JsonPrimitive(0)) + } + + @Test + fun `a non-primitive style is refused by name rather than throwing a cast error`() { + styleFailure(buildJsonObject { putJsonObject("style") { put("name", "link") } }) + } +} diff --git a/typescript/scripts/generate-services.ts b/typescript/scripts/generate-services.ts index 5fc88b17d0..897f119ba9 100644 --- a/typescript/scripts/generate-services.ts +++ b/typescript/scripts/generate-services.ts @@ -834,11 +834,24 @@ function parseOperation( // consumer persists after accepting that page. Flattening the walk would // swallow every intermediate position and leave a crashed consumer with // nothing to resume from. - const paginationStyle = operation["x-basecamp-pagination"]?.style; + // Presence is tested explicitly rather than by truthiness, exactly as Swift + // does. A truthy guard exempts a present-but-falsy trait — `false`, `0`, `""` — + // from the refusal and reads it as unpaginated, which is the silent pass this + // branch exists to close. The declared type says this is always an object or + // absent, but the value comes from parsed JSON, so read it as unknown and + // narrow: absent and a literal null are unpaginated; anything else is + // declared, and a declared trait that is not an object has no style to accept. + const paginationValue: unknown = operation["x-basecamp-pagination"]; + const paginationDeclared = paginationValue !== undefined && paginationValue !== null; + const paginationTrait = + typeof paginationValue === "object" && paginationValue !== null + ? (paginationValue as { style?: unknown; key?: unknown }) + : undefined; + const paginationStyle = typeof paginationTrait?.style === "string" ? paginationTrait.style : undefined; // Only "link" and "cursor" are implemented. Anything else — a typo, or the // "page" style the trait used to advertise — must fail loudly: read as "not // paginated" it would silently ship a method that never walks. - if (operation["x-basecamp-pagination"] && paginationStyle !== "link" && paginationStyle !== "cursor") { + if (paginationDeclared && paginationStyle !== "link" && paginationStyle !== "cursor") { throw new Error( `${operationId}: unsupported pagination style ${JSON.stringify(paginationStyle)} (expected "link" or "cursor")` ); @@ -849,7 +862,8 @@ function parseOperation( // position}` into the item type — and several of those consumers never check // `hasPagination`. A cursor operation must be typed as its envelope, so the // key never reaches them rather than every consumer having to remember. - const paginationKey = paginationStyle === "link" ? operation["x-basecamp-pagination"]?.key : undefined; + const paginationKey = + paginationStyle === "link" && typeof paginationTrait?.key === "string" ? paginationTrait.key : undefined; const multipartField = operation["x-basecamp-multipart"]?.field; return { @@ -1828,5 +1842,5 @@ if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) // the declared return type is only half of a wrapped-paginated signature — // nothing but the emitted method shows the `requestPaginatedWrapped` type // argument, and the two have to name the same element. -export { generateExampleValue, setSchemas, buildReturnType, generateMethod }; -export type { Schema, ParsedOperation }; +export { generateExampleValue, setSchemas, buildReturnType, generateMethod, parseOperation }; +export type { Schema, ParsedOperation, Operation }; diff --git a/typescript/tests/generator/pagination-style.test.ts b/typescript/tests/generator/pagination-style.test.ts new file mode 100644 index 0000000000..a375b6de4d --- /dev/null +++ b/typescript/tests/generator/pagination-style.test.ts @@ -0,0 +1,87 @@ +import { describe, it, expect } from "vitest"; +import { parseOperation, type Operation } from "../../scripts/generate-services.js"; + +// Pins TypeScript's refusal of a pagination style it does not implement. +// +// `hasPagination` keys off `style === "link"`, which makes an unrecognised value +// MORE dangerous than the old presence check was: read as "not paginated" it +// ships a method that never walks, silently, with nothing downstream to catch +// it. So every unimplemented spelling has to fail generation by name. +// +// The malformed-trait cases are the ones that actually bite. A truthy guard — +// `if (operation["x-basecamp-pagination"] && ...)` — exempts a present-but-falsy +// trait from the refusal and reads it as unpaginated, which is the exact silent +// pass the refusal exists to close. Swift had the same hole through +// `as? [String: Any]` and closed it by testing presence before the cast; Kotlin +// had it through `as? JsonObject`. These pin that TypeScript does the same. +// +// The declared `Operation` type says the trait is always an object or absent. +// The value comes from parsed JSON, so it can be anything; the casts below are +// deliberate, and removing them would hide the very cases under test. + +function operationWith(pagination: unknown): Operation { + const operation: Record = { + operationId: "ListWidgets", + responses: {}, + }; + if (pagination !== undefined) { + operation["x-basecamp-pagination"] = pagination; + } + return operation as unknown as Operation; +} + +const parse = (pagination: unknown) => parseOperation("/widgets", "get", operationWith(pagination)); + +describe("pagination style refusal", () => { + it("paginates on the link style and keeps its key", () => { + const parsed = parse({ style: "link", key: "widgets" }); + + expect(parsed.hasPagination).toBe(true); + expect(parsed.paginationKey).toBe("widgets"); + }); + + it("does not paginate on the cursor style, and withholds its key", () => { + // The key is withheld at the source rather than gated at each consumer: + // findUnderlyingEntitySchema unwraps an envelope whenever the key is set and + // never consults hasPagination, so a cursor operation carrying a key would be + // typed as the item under it instead of the envelope the wire actually sends. + const parsed = parse({ style: "cursor", key: "events" }); + + expect(parsed.hasPagination).toBe(false); + expect(parsed.paginationKey).toBeUndefined(); + }); + + it.each([ + ["absent", undefined], + ["a literal null", null], + ])("treats %s trait as unpaginated", (_label, pagination) => { + const parsed = parse(pagination); + + expect(parsed.hasPagination).toBe(false); + expect(parsed.paginationKey).toBeUndefined(); + }); + + it.each([ + ["the retired page style", { style: "page" }, '"page"'], + ["a typo", { style: "linkk" }, '"linkk"'], + ["a capitalised link", { style: "Link" }, '"Link"'], + ["an empty style", { style: "" }, '""'], + ["a declared trait with no style", { maxPageSize: 50 }, "undefined"], + ["a non-string style", { style: { name: "link" } }, "undefined"], + ])("refuses %s by name", (_label, pagination, spelling) => { + expect(() => parse(pagination)).toThrow( + `ListWidgets: unsupported pagination style ${spelling} (expected "link" or "cursor")`, + ); + }); + + it.each([ + ["a bare string", "page"], + ["an array", ["link"]], + ["false", false], + ["zero", 0], + ["an empty string", ""], + ])("refuses %s rather than reading it as unpaginated", (_label, pagination) => { + // `false`, `0` and `""` are the cases a truthiness guard lets through. + expect(() => parse(pagination)).toThrow(/unsupported pagination style/); + }); +}); From 8f073ffa9be45e70773924ae55ec5835745e42a7 Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Wed, 16 Sep 2026 13:29:35 +0200 Subject: [PATCH 2/2] Assert the refusal message, not just the exception type Copilot, and it is the same defect this PR exists to close, one level up. `styleFailure` answers the exception's message, and the named-style tests assert against it -- but the five malformed-trait tests threw the answer away and checked only that an IllegalArgumentException came out. `jsonPrimitive` throws that same type, so `a non-primitive style is refused by name rather than throwing a cast error` could not tell a refusal from the cast error it is named after: revert the parser and it stayed green. All five now assert the message. Proven by reverting the parser to its pre-fix shape and re-running: five failures, and the fifth lands on the message assertion rather than on the exception type, which is the distinction that was missing. --- .../sdk/generator/PaginationStyleTest.kt | 24 +++++++++++++++---- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/kotlin/generator/src/test/kotlin/com/basecamp/sdk/generator/PaginationStyleTest.kt b/kotlin/generator/src/test/kotlin/com/basecamp/sdk/generator/PaginationStyleTest.kt index 39dace8be2..bbcafc5ea3 100644 --- a/kotlin/generator/src/test/kotlin/com/basecamp/sdk/generator/PaginationStyleTest.kt +++ b/kotlin/generator/src/test/kotlin/com/basecamp/sdk/generator/PaginationStyleTest.kt @@ -41,6 +41,20 @@ class PaginationStyleTest { private fun styleFailure(pagination: JsonElement?): String = assertFailsWith { parse(pagination) }.message ?: "" + // A trait with no readable style refuses by naming `null` as the style. + // + // Asserting the MESSAGE, not just the exception type, is the whole point of + // this helper. `jsonPrimitive` throws IllegalArgumentException too, so a + // test that checked only the type would stay green if the parser went back + // to the cast that this file exists to rule out -- it would be catching the + // cast error and calling it a refusal. + private fun assertRefusedWithNoReadableStyle(pagination: JsonElement?) { + assertEquals( + "ListWidgets: unsupported pagination style null (expected \"link\" or \"cursor\")", + styleFailure(pagination) + ) + } + @Test fun `the link style paginates and keeps its key`() { val parsed = parse( @@ -127,26 +141,26 @@ class PaginationStyleTest { // The hole: `as? JsonObject` answers null here, so a presence check // written against the CAST would exempt this and ship an unpaginated // method for a spec that plainly said "page". - styleFailure(JsonPrimitive("page")) + assertRefusedWithNoReadableStyle(JsonPrimitive("page")) } @Test fun `an array trait is refused`() { - styleFailure(buildJsonArray { add("link") }) + assertRefusedWithNoReadableStyle(buildJsonArray { add("link") }) } @Test fun `a boolean trait is refused`() { - styleFailure(JsonPrimitive(false)) + assertRefusedWithNoReadableStyle(JsonPrimitive(false)) } @Test fun `a numeric trait is refused`() { - styleFailure(JsonPrimitive(0)) + assertRefusedWithNoReadableStyle(JsonPrimitive(0)) } @Test fun `a non-primitive style is refused by name rather than throwing a cast error`() { - styleFailure(buildJsonObject { putJsonObject("style") { put("name", "link") } }) + assertRefusedWithNoReadableStyle(buildJsonObject { putJsonObject("style") { put("name", "link") } }) } }