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..bbcafc5ea3 --- /dev/null +++ b/kotlin/generator/src/test/kotlin/com/basecamp/sdk/generator/PaginationStyleTest.kt @@ -0,0 +1,166 @@ +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 ?: "" + + // 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( + 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". + assertRefusedWithNoReadableStyle(JsonPrimitive("page")) + } + + @Test + fun `an array trait is refused`() { + assertRefusedWithNoReadableStyle(buildJsonArray { add("link") }) + } + + @Test + fun `a boolean trait is refused`() { + assertRefusedWithNoReadableStyle(JsonPrimitive(false)) + } + + @Test + fun `a numeric trait is refused`() { + assertRefusedWithNoReadableStyle(JsonPrimitive(0)) + } + + @Test + fun `a non-primitive style is refused by name rather than throwing a cast error`() { + assertRefusedWithNoReadableStyle(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/); + }); +});