Close the malformed-trait pass in the two generators that still had it

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"

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

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 };

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<string, unknown> = {
+    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/);
+  });
+});