fix(codegen/gcp): route query-location params in action methods and CRUD update (#1990) #263

Merged

stack72 merged 4 commits from fix/1990-query-location-params into main 2026-09-08 05:04:37 +00:00

Conversation 4 Commits 4 Files changed 694 +19243 -2071

stack72 commented 2026-09-08 04:28:50 +00:00

Owner

Copy link

Summary

• Pipeline: Added a collection pass for update/patch query parameters into domainProperties and updatePropertyNames, mirroring the existing insert query-param pass. This makes method-level query params like valueInputOption available in GlobalArgsSchema so the CRUD update codegen routes them to the URL automatically.
• Action methods: Modified the action method generator to extract query-location parameters from action.config.parameters, add them to the action's arguments schema, and route them to params (not body). This fixes methods like Sheets values.append where valueInputOption is required by the API but was previously unreachable.
• Regenerated model/gcp/sheets/ to pick up the fix. Other GCP models will pick up the fix in the next scheduled full regeneration.

Sibling of #1274 (path params missing from GlobalArgsSchema) and #1562 (query routing in CRUD update codegen).

Closes #1990

Test plan

• New codegen unit test: action method with query + body params verifies correct routing
• Existing GET action method snapshot updated (batch_get ranges now in arguments)
• Filtered regen: deno task generate:gcp sheets produces intended diff
• Idempotency: second regen run = 0 changed models
• Type-check + lint + fmt pass for both codegen/ and model/gcp/sheets/
• Lockfile frozen in both directories

🤖 Generated with Claude Code

## Summary - **Pipeline**: Added a collection pass for update/patch query parameters into `domainProperties` and `updatePropertyNames`, mirroring the existing insert query-param pass. This makes method-level query params like `valueInputOption` available in `GlobalArgsSchema` so the CRUD update codegen routes them to the URL automatically. - **Action methods**: Modified the action method generator to extract query-location parameters from `action.config.parameters`, add them to the action's arguments schema, and route them to `params` (not `body`). This fixes methods like Sheets `values.append` where `valueInputOption` is required by the API but was previously unreachable. - **Regenerated** `model/gcp/sheets/` to pick up the fix. Other GCP models will pick up the fix in the next scheduled full regeneration. Sibling of #1274 (path params missing from GlobalArgsSchema) and #1562 (query routing in CRUD update codegen). Closes #1990 ## Test plan - [x] New codegen unit test: action method with query + body params verifies correct routing - [x] Existing GET action method snapshot updated (batch_get `ranges` now in arguments) - [x] Filtered regen: `deno task generate:gcp sheets` produces intended diff - [x] Idempotency: second regen run = 0 changed models - [x] Type-check + lint + fmt pass for both `codegen/` and `model/gcp/sheets/` - [x] Lockfile frozen in both directories 🤖 Generated with [Claude Code](https://claude.com/claude-code)

stack72 added 1 commit 2026-09-08 04:28:50 +00:00

fix(codegen/gcp): route query-location params in action methods and CRUD update (#1990) ... 2c131824e8

The GCP codegen had two gaps for query-location parameters:

1. The pipeline only collected insert method query params into
   domainProperties — update/patch method query params (like
   valueInputOption on Sheets values.update) were missing from
   GlobalArgsSchema, making them unreachable. Added a collection
   pass for update/patch query params mirroring the insert pass.

2. The action method generator only processed requestProperties
   (body schema) when building argument schemas. Query-location
   parameters from action.config.parameters were ignored entirely,
   making action methods like Sheets values.append unable to send
   required query params. Added query param extraction, argument
   schema inclusion, and params routing for action methods.

The runtime buildUrl() already supported query params in the params
dict — only the codegen needed fixing.

Closes #1990

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

stack72 added 1 commit 2026-09-08 04:31:17 +00:00

fix(codegen/gcp): skip updateMask in update/patch query-param collection ... 768dc73a51

updateMask is auto-computed from body keys by the CRUD update codegen —
adding it to domainProperties would put a useless field in GlobalArgsSchema
for ~205 GCP services whose value gets silently overwritten.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

stack72 added 2 commits 2026-09-08 04:38:13 +00:00

fix(codegen/gcp): sanitize dotted query param names with safeIdent ... a7068dc42e

GCP discovery docs can declare query parameters with dots in the name
(e.g. options.requestedPolicyVersion). Using these as bare JS identifiers
in the arguments schema produces a syntax error. Apply safeIdent() to
convert dots to underscores for the schema key and args access, while
keeping the raw name for the params key passed to buildUrl.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

chore: regenerate all GCP models with query-param routing fix ... 6fe33b8c39

Full regeneration picks up the query-location parameter fix for both
CRUD update methods (pipeline pass) and action methods (codegen).
158 models changed across 266 services. Second run confirms idempotency.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

stack72 commented 2026-09-08 04:40:32 +00:00

Author

Owner

Copy link

Code Review

Blocking Issues

None.

Suggestions

1. String() coercion for array-valued query params (extensionModelGenerator.ts, pipeline.ts): The ranges parameter on batch_get (and similar array-typed query params in Google APIs) is meant to be repeated — ?ranges=A1&ranges=B2. The generated code emits params["ranges"] = String(args["ranges"]), which for an array produces "A1,B2" instead. This is a pre-existing framework limitation (the same pattern is used in existing insert query-param routing), but the Sheets batchGet endpoint is a likely real-world pain point since ranges is almost always an array. No change needed to block merge, but worth a follow-up.

2. pathParamNames variable name is slightly misleading (extensionModelGenerator.ts, line ~1575): It's built from action.config.parameterOrder, which is the list of required ordered params — in practice always path params for Google APIs, but not strictly enforced by the type. The name implies it contains only path params. Since the filter paramDef.location !== "query" already handles real path params, the pathParamNames guard is defensive and harmless, just cosmetically misnamed.

3. New unit test uses assert(output.includes(...)) but the existing pattern is snapshots: The new test in extensionModelGenerator_test.ts asserts on specific substrings rather than adding a snapshot entry. The existing test suite uses @std/testing/snapshot for full-output verification. The substring approach is readable for targeted assertions but won't catch unexpected regressions in surrounding generated code. Not blocking — the snapshot for batch_get in __snapshots__/extensionModelGenerator_test.ts.snap does cover the equivalent generated output from the test fixture.

Overall the change is clean: the update/patch query-param pass correctly mirrors the insert pass and excludes updateMask and validateOnly, the action-method query-param routing correctly separates path params (via parameterOrder) from query params, and the model changes are consistent with the codegen output.

## Code Review ### Blocking Issues None. ### Suggestions 1. **`String()` coercion for array-valued query params** (`extensionModelGenerator.ts`, `pipeline.ts`): The `ranges` parameter on `batch_get` (and similar array-typed query params in Google APIs) is meant to be repeated — `?ranges=A1&ranges=B2`. The generated code emits `params["ranges"] = String(args["ranges"])`, which for an array produces `"A1,B2"` instead. This is a pre-existing framework limitation (the same pattern is used in existing insert query-param routing), but the Sheets `batchGet` endpoint is a likely real-world pain point since `ranges` is almost always an array. No change needed to block merge, but worth a follow-up. 2. **`pathParamNames` variable name is slightly misleading** (`extensionModelGenerator.ts`, line ~1575): It's built from `action.config.parameterOrder`, which is the list of *required ordered* params — in practice always path params for Google APIs, but not strictly enforced by the type. The name implies it contains only path params. Since the filter `paramDef.location !== "query"` already handles real path params, the `pathParamNames` guard is defensive and harmless, just cosmetically misnamed. 3. **New unit test uses `assert(output.includes(...))` but the existing pattern is snapshots**: The new test in `extensionModelGenerator_test.ts` asserts on specific substrings rather than adding a snapshot entry. The existing test suite uses `@std/testing/snapshot` for full-output verification. The substring approach is readable for targeted assertions but won't catch unexpected regressions in surrounding generated code. Not blocking — the snapshot for `batch_get` in `__snapshots__/extensionModelGenerator_test.ts.snap` does cover the equivalent generated output from the test fixture. Overall the change is clean: the update/patch query-param pass correctly mirrors the insert pass and excludes `updateMask` and `validateOnly`, the action-method query-param routing correctly separates path params (via `parameterOrder`) from query params, and the model changes are consistent with the codegen output.

stack72 commented 2026-09-08 04:40:39 +00:00

Author

Owner

Copy link

Adversarial Review

Critical / High

None found.

Medium

1. Missing deprecated filter on action method query params — codegen/gcp/extensionModelGenerator.ts:1577-1584

   The action method query-param collection loop does not check paramDef.deprecated === true, while the parallel code in pipeline.ts:1466-1470 (for CRUD update/patch) and pipeline.ts:1447 (for insert) both filter deprecated params. If a GCP API marks an action query parameter as deprecated, it will still be emitted into the generated arguments schema and routing code.

   Breaking example: A GCP service deprecates a query parameter (e.g., responseValueRenderOption) on an action method. The generated model exposes it as an argument and routes it to the URL, potentially sending an unsupported parameter that the API rejects or ignores with a warning.

   Suggested fix: Add if (paramDef.deprecated === true) continue; after the location check at line 1581, consistent with both pipeline.ts passes.

2. String() coercion flattens array-valued query params — codegen/gcp/extensionModelGenerator.ts:1735-1736

   Query params like ranges (Sheets batchGet) accept repeated values. The generated code does params["ranges"] = String(args["ranges"]). If the caller passes ["Sheet1!A1:B2", "Sheet2!C3"], JavaScript's String() on an array produces "Sheet1!A1:B2,Sheet2!C3" — a single comma-joined string. GCP APIs that expect repeated query params (?ranges=X&ranges=Y) would receive the wrong encoding.

   This is a pre-existing pattern (the CRUD insert query-param routing at line 529 does the same String() coercion via globalArgs), so it's not new to this PR. But the action method change widens the surface area — ranges on batchGet is the first generated action param where this matters.

   Suggested fix (non-blocking): Consider handling array values by iterating and adding multiple entries, or document that callers must pass pre-formatted strings. This could be addressed in a follow-up.

Low

1. No validateOnly filter on action method query params — codegen/gcp/extensionModelGenerator.ts:1577-1584

   The CRUD passes in pipeline.ts skip validateOnly (a dry-run flag), but the action method query-param loop doesn't. If an action method declares a validateOnly query parameter, it would be exposed as an argument. This may be intentional — validateOnly could be useful for action methods — but it's an inconsistency worth noting.

Verdict

PASS — The core logic is correct: query-location parameters on action methods are properly split into the arguments schema and routed to URL params instead of the request body, and the pipeline correctly mirrors the insert pass for update/patch methods. The updateMask exclusion in the second commit is necessary and correct. Test coverage validates the golden path. The medium findings are edge cases around schema inconsistencies (deprecated params) and data type handling (arrays), neither of which block the merge.

## Adversarial Review ### Critical / High None found. ### Medium 1. **Missing `deprecated` filter on action method query params** — `codegen/gcp/extensionModelGenerator.ts:1577-1584` The action method query-param collection loop does not check `paramDef.deprecated === true`, while the parallel code in `pipeline.ts:1466-1470` (for CRUD update/patch) and `pipeline.ts:1447` (for insert) both filter deprecated params. If a GCP API marks an action query parameter as deprecated, it will still be emitted into the generated arguments schema and routing code. **Breaking example:** A GCP service deprecates a query parameter (e.g., `responseValueRenderOption`) on an action method. The generated model exposes it as an argument and routes it to the URL, potentially sending an unsupported parameter that the API rejects or ignores with a warning. **Suggested fix:** Add `if (paramDef.deprecated === true) continue;` after the `location` check at line 1581, consistent with both pipeline.ts passes. 2. **`String()` coercion flattens array-valued query params** — `codegen/gcp/extensionModelGenerator.ts:1735-1736` Query params like `ranges` (Sheets `batchGet`) accept repeated values. The generated code does `params["ranges"] = String(args["ranges"])`. If the caller passes `["Sheet1!A1:B2", "Sheet2!C3"]`, JavaScript's `String()` on an array produces `"Sheet1!A1:B2,Sheet2!C3"` — a single comma-joined string. GCP APIs that expect repeated query params (`?ranges=X&ranges=Y`) would receive the wrong encoding. This is a **pre-existing pattern** (the CRUD insert query-param routing at line 529 does the same `String()` coercion via globalArgs), so it's not new to this PR. But the action method change widens the surface area — `ranges` on `batchGet` is the first generated action param where this matters. **Suggested fix (non-blocking):** Consider handling array values by iterating and adding multiple entries, or document that callers must pass pre-formatted strings. This could be addressed in a follow-up. ### Low 1. **No `validateOnly` filter on action method query params** — `codegen/gcp/extensionModelGenerator.ts:1577-1584` The CRUD passes in `pipeline.ts` skip `validateOnly` (a dry-run flag), but the action method query-param loop doesn't. If an action method declares a `validateOnly` query parameter, it would be exposed as an argument. This may be intentional — `validateOnly` could be useful for action methods — but it's an inconsistency worth noting. ### Verdict **PASS** — The core logic is correct: query-location parameters on action methods are properly split into the arguments schema and routed to URL params instead of the request body, and the pipeline correctly mirrors the insert pass for update/patch methods. The `updateMask` exclusion in the second commit is necessary and correct. Test coverage validates the golden path. The medium findings are edge cases around schema inconsistencies (deprecated params) and data type handling (arrays), neither of which block the merge.

stack72 commented 2026-09-08 04:51:39 +00:00

Author

Owner

Copy link

Code Review

Blocking Issues

None.

Suggestions

1. No explicit test for CRUD update query-param routing (pipeline side)

   pipeline.ts lines 1463–1479 add a new pass that collects query-location parameters from update/patch method configs into domainProperties and updatePropertyNames. The routing of those collected params in the CRUD update body (already present in extensionModelGenerator.ts) is exercised indirectly by the model diffs, but there is no unit test in extensionModelGenerator_test.ts that provides a resource with an update method query param and asserts it lands in params rather than body. The analogous action-method path now has a dedicated assertion test (generateGcpExtensionModel - action method query params routed to params); a similar test for the CRUD update path would make the coverage symmetric and prevent regression.

2. Sanitization mismatch in updateParameters[propName]?.location lookup (pre-existing pattern, carried into new code)

   In the CRUD update body loop (extensionModelGenerator.ts ~line 1052), propName is a sanitized key from resource.domainProperties, but updateParameters (from extractMethodConfig) stores raw parameter names. For any GCP update/patch query param whose name contains a dot, hyphen, or slash (e.g., a hypothetical "foo.bar") the sanitized lookup updateParameters["foo_bar"] returns undefined, so the property silently falls into the request body instead of the URL query string. The same mismatch exists in the pre-existing insert code path. In practice, GCP query-parameter names appear to be plain camelCase identifiers, so this is unlikely to cause observable failures, but it is worth noting since the new pipeline.ts collection pass follows the same pattern. Consider sanitizing the keys in extractMethodConfig (or adding a safeIdent wrapper at the lookup site) to make the system robust by construction.

3. Minor: domainProperties[paramName] duplicate-guard uses raw name against mixed-case-key map

   In the new update/patch query-param collection loop (pipeline.ts line 1468: if (domainProperties[paramName]) continue;), paramName is the raw parameter name, but domainProperties may already contain an entry under the sanitized version of that name (populated by the earlier gcpSchemaToCfProperties calls). For standard alphanumeric param names this is a non-issue, but for param names with dots/hyphens the guard could fail to detect an existing sanitized entry and add a second raw entry. This is another instance of the same pre-existing pattern; same note as above applies.

## Code Review ### Blocking Issues None. ### Suggestions 1. **No explicit test for CRUD update query-param routing (pipeline side)** `pipeline.ts` lines 1463–1479 add a new pass that collects query-location parameters from `update`/`patch` method configs into `domainProperties` and `updatePropertyNames`. The routing of those collected params in the CRUD update body (already present in `extensionModelGenerator.ts`) is exercised indirectly by the model diffs, but there is no unit test in `extensionModelGenerator_test.ts` that provides a resource with an update method query param and asserts it lands in `params` rather than `body`. The analogous action-method path now has a dedicated assertion test (`generateGcpExtensionModel - action method query params routed to params`); a similar test for the CRUD update path would make the coverage symmetric and prevent regression. 2. **Sanitization mismatch in `updateParameters[propName]?.location` lookup (pre-existing pattern, carried into new code)** In the CRUD update body loop (`extensionModelGenerator.ts` ~line 1052), `propName` is a *sanitized* key from `resource.domainProperties`, but `updateParameters` (from `extractMethodConfig`) stores *raw* parameter names. For any GCP update/patch query param whose name contains a dot, hyphen, or slash (e.g., a hypothetical `"foo.bar"`) the sanitized lookup `updateParameters["foo_bar"]` returns `undefined`, so the property silently falls into the request body instead of the URL query string. The same mismatch exists in the pre-existing insert code path. In practice, GCP query-parameter names appear to be plain camelCase identifiers, so this is unlikely to cause observable failures, but it is worth noting since the new `pipeline.ts` collection pass follows the same pattern. Consider sanitizing the keys in `extractMethodConfig` (or adding a `safeIdent` wrapper at the lookup site) to make the system robust by construction. 3. **Minor: `domainProperties[paramName]` duplicate-guard uses raw name against mixed-case-key map** In the new update/patch query-param collection loop (`pipeline.ts` line 1468: `if (domainProperties[paramName]) continue;`), `paramName` is the raw parameter name, but `domainProperties` may already contain an entry under the *sanitized* version of that name (populated by the earlier `gcpSchemaToCfProperties` calls). For standard alphanumeric param names this is a non-issue, but for param names with dots/hyphens the guard could fail to detect an existing sanitized entry and add a second raw entry. This is another instance of the same pre-existing pattern; same note as above applies.

stack72 commented 2026-09-08 05:02:52 +00:00

Author

Owner

Copy link

Adversarial Review

Critical / High

No critical or high severity findings.

Medium

1. codegen/gcp/extensionModelGenerator.ts:1741 — String() coercion silently flattens array-valued query params.

   GCP Discovery Documents sometimes declare query parameters that accept repeated values (e.g., ranges on sheets.spreadsheets.values.batchGet). The generated code coerces them with String(args["ranges"]), which turns ["Sheet1!A1:B2", "Sheet2!A1"] into "Sheet1!A1:B2,Sheet2!A1". GCP APIs expect repeated query params as ranges=Sheet1!A1:B2&ranges=Sheet2!A1.

   Breaking example: A user calls the batch_get action method with ranges: ["Sheet1!A1:B2", "Sheet2!C3"]. The generated code sends ?ranges=Sheet1!A1:B2,Sheet2!C3 (one param, comma-joined) instead of ?ranges=Sheet1!A1:B2&ranges=Sheet2!C3 (two params). The API may reject or misinterpret this.

   Note: This is a pre-existing pattern — the list method query param routing (extensionModelGenerator.ts:1514) and CRUD query param routing have the same String() coercion. This PR didn't introduce it, just propagated the same pattern to action methods. Flagging because it's now exercised on a concrete new path (Sheets ranges). Not blocking since it's consistent with the existing codebase.

Low

1. codegen/gcp/extensionModelGenerator.ts:1598-1603 — Query params on action methods are always .optional() even when potentially required by the API.

   All query params added to the action method's arguments schema are generated as z.any().optional(). If a GCP API requires a query param (e.g., valueInputOption is effectively required for Sheets values.append), the client won't validate this — the user gets a runtime 400 from GCP instead of a local validation error.

   Mitigation: This matches the existing behavior for body properties on actions (which use z.any() without propagating type info from the Discovery schema). The GCP error message is usually clear. Low impact.

2. codegen/gcp/pipeline.ts:1460-1479 — Update/patch query param collection doesn't filter by parameterOrder membership, unlike insert.

   The insert query param collection (lines 1444-1456) adds ALL query params from the insert method. The new update/patch collection mirrors this. Both skip validateOnly and (for update/patch) updateMask. However, neither checks whether the param is actually in parameterOrder — they collect all method-level query params. This is correct behavior (query params typically aren't in parameterOrder since that's for path params), just noting the design intent is clear and consistent.

Verdict

PASS — The changes correctly route query-location parameters in action methods to URL params instead of the request body, with proper dedup against request body properties. The pipeline change consistently collects update/patch query params into domainProperties. Test coverage is adequate (direct assertion test + snapshot). The design doc is updated. No blocking issues found.

## Adversarial Review ### Critical / High No critical or high severity findings. ### Medium 1. **`codegen/gcp/extensionModelGenerator.ts:1741` — `String()` coercion silently flattens array-valued query params.** GCP Discovery Documents sometimes declare query parameters that accept repeated values (e.g., `ranges` on `sheets.spreadsheets.values.batchGet`). The generated code coerces them with `String(args["ranges"])`, which turns `["Sheet1!A1:B2", "Sheet2!A1"]` into `"Sheet1!A1:B2,Sheet2!A1"`. GCP APIs expect repeated query params as `ranges=Sheet1!A1:B2&ranges=Sheet2!A1`. **Breaking example**: A user calls the `batch_get` action method with `ranges: ["Sheet1!A1:B2", "Sheet2!C3"]`. The generated code sends `?ranges=Sheet1!A1:B2,Sheet2!C3` (one param, comma-joined) instead of `?ranges=Sheet1!A1:B2&ranges=Sheet2!C3` (two params). The API may reject or misinterpret this. **Note**: This is a **pre-existing** pattern — the list method query param routing (`extensionModelGenerator.ts:1514`) and CRUD query param routing have the same `String()` coercion. This PR didn't introduce it, just propagated the same pattern to action methods. Flagging because it's now exercised on a concrete new path (Sheets `ranges`). Not blocking since it's consistent with the existing codebase. ### Low 1. **`codegen/gcp/extensionModelGenerator.ts:1598-1603` — Query params on action methods are always `.optional()` even when potentially required by the API.** All query params added to the action method's arguments schema are generated as `z.any().optional()`. If a GCP API requires a query param (e.g., `valueInputOption` is effectively required for Sheets `values.append`), the client won't validate this — the user gets a runtime 400 from GCP instead of a local validation error. **Mitigation**: This matches the existing behavior for body properties on actions (which use `z.any()` without propagating type info from the Discovery schema). The GCP error message is usually clear. Low impact. 2. **`codegen/gcp/pipeline.ts:1460-1479` — Update/patch query param collection doesn't filter by `parameterOrder` membership, unlike insert.** The insert query param collection (lines 1444-1456) adds ALL query params from the insert method. The new update/patch collection mirrors this. Both skip `validateOnly` and (for update/patch) `updateMask`. However, neither checks whether the param is actually in `parameterOrder` — they collect all method-level query params. This is correct behavior (query params typically aren't in `parameterOrder` since that's for path params), just noting the design intent is clear and consistent. ### Verdict **PASS** — The changes correctly route query-location parameters in action methods to URL params instead of the request body, with proper dedup against request body properties. The pipeline change consistently collects update/patch query params into `domainProperties`. Test coverage is adequate (direct assertion test + snapshot). The design doc is updated. No blocking issues found.

stack72 merged commit 7b1f1de314 into main 2026-09-08 05:04:37 +00:00

stack72 referenced this pull request from a commit 2026-09-08 05:04:39 +00:00

fix(codegen/gcp): route query-location params in action methods and CRUD update (#1990) (#263)

stack72 deleted branch fix/1990-query-location-params 2026-09-08 05:04:39 +00:00

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

swamp-club/swamp-extensions!263

No description provided.