Skip to content

fix(sdk-client): omit undefined optional params from page.get() graphql.variables#36290

Merged
rjvelazco merged 4 commits into
mainfrom
issue-36108-page-get-undefined-graphql-variables
Jun 25, 2026
Merged

fix(sdk-client): omit undefined optional params from page.get() graphql.variables#36290
rjvelazco merged 4 commits into
mainfrom
issue-36108-page-get-undefined-graphql-variables

Conversation

@rjvelazco

@rjvelazco rjvelazco commented Jun 23, 2026

Copy link
Copy Markdown
Member

Proposed Changes

Fixes #36108

client.page.get() returned undefined values inside graphql.variables, breaking Next.js Pages Router apps that return the page response from getServerSideProps/getStaticProps (JSON has no representation for undefined).

Root cause

In core-web/libs/sdk/client/src/lib/client/page/page-api.ts, the optional request params personaId, publishDate, and variantName were destructured without defaults and spread verbatim into requestVariables. That object is returned on the response, so the undefined values surfaced to consumers.

Fix

Only include the optional params (personaId, publishDate, variantName) in requestVariables when they are actually defined. The GraphQL request is unaffected — these variables are nullable and JSON.stringify already drops undefined keys — so the only observable change is that the returned graphql.variables object is now JSON-serializable.

Tests

  • Updated existing assertions that expected personaId/publishDate/variantName: undefined.
  • Added a regression test asserting graphql.variables contains no undefined values and round-trips through JSON.parse(JSON.stringify(...)).
  • Added a request-contract test asserting undefined optional params are omitted from the request variables.

All 280 sdk-client tests pass; lint clean.

🤖 Generated with Claude Code

This PR fixes: #36108

…ql.variables (#36108)

page.get() spread the optional request params (personaId, publishDate,
variantName) into requestVariables without defaults, leaving `undefined`
values in the returned graphql.variables object. That object is returned
verbatim on the response and breaks JSON serialization for Next.js Pages
Router consumers (getServerSideProps/getStaticProps throw on undefined).

Only include these optional params when they are defined. The GraphQL
request itself is unaffected — the variables are nullable and
JSON.stringify already drops undefined keys.

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

claude Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Claude finished @rjvelazco's task in 56s —— View job


Rollback Safety Analysis

  • Read rollback-unsafe categories reference
  • Get the full PR diff
  • Analyze diff against unsafe categories
  • Apply appropriate label

Result: ✅ Safe To Rollback

The label "AI: Safe To Rollback" has been applied.

Diff scope: 4 files changed, all within core-web/libs/sdk/client/ (TypeScript SDK client + tests). No Java, no database, no Elasticsearch, no OSGi.

Category-by-category analysis:

Category Verdict
C-1 Structural Data Model Change ✅ No DB schema touched
C-2 Elasticsearch Mapping Change ✅ No ES mapping touched
C-3 Content JSON Model Version Bump ✅ No contentlet_as_json / CURRENT_MODEL_VERSION touched
C-4 DROP TABLE / DROP COLUMN ✅ No DDL
H-1 One-Way Data Migration ✅ No data migration
H-2 RENAME TABLE / RENAME COLUMN ✅ No DDL
H-3 PK Restructuring ✅ No DDL
H-4 New Content Type Field Type ✅ No field type registered
H-5 Binary Storage Provider Change ✅ No storage config touched
H-6 DROP PROCEDURE / FUNCTION ✅ No DDL
H-7 NOT NULL Column Without Default ✅ No DDL
H-8 VTL Viewtool Contract Change ✅ No viewtool modified
M-1 Non-Broadening Column Type Change ✅ No DDL
M-2 Push Publishing Bundle Format Change ✅ No bundle XML touched
M-3 REST / GraphQL API Contract Change ✅ The returned graphql.variables object loses undefined-valued keys — this is a fix, not a breaking change. Consumers relying on undefined keys would have been broken before (they're invalid JSON). No GraphQL schema types or REST endpoint signatures changed.
M-4 OSGi Plugin API Breakage ✅ No OSGi interfaces touched

Summary: This PR only modifies a TypeScript SDK utility function (removeUndefinedValues) and its call site in page-api.ts. The change makes the returned response object JSON-serializable by omitting undefined-valued optional keys. It has zero impact on the dotCMS backend, database, Elasticsearch, or any server-side API contract.

@github-actions

github-actions Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

🤖 Bedrock Review — deepseek.v3.2

[🟡 Medium] core-web/libs/sdk/client/src/lib/client/page/page-api.ts:152removeUndefinedValues is imported but not defined in this file; ensure the utility function exists and is exported from ./utils.
[🟡 Medium] core-web/libs/sdk/client/src/lib/client/page/page-api.ts:152-165 — The variable newURL is used in error messages but normalizedUrl is still referenced in the logVerboseError call on line 179; ensure consistency.


Run: #28053795134 · tokens: in: 3396 · out: 117 · total: 3513

@rjvelazco rjvelazco changed the title fix(sdk-client): omit undefined optional params from page.get() graphql.variables (#36108) fix(sdk-client): omit undefined optional params from page.get() graphql.variables Jun 23, 2026
#36108)

Replace the per-param undefined guards with a generic removeUndefinedValues
helper applied to the whole requestVariables object. This protects any
variable from leaking `undefined` (e.g. mode/languageId/fireRules if a
caller passes them explicitly as undefined), not just the optional ones.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…#36108)

CI nx format:check flagged utils.ts. Collapse the single-statement helper
signature/body to match the workspace prettier config.

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

mergify Bot commented Jun 23, 2026

Copy link
Copy Markdown

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

  • Queue this pull request

@rjvelazco
rjvelazco added this pull request to the merge queue Jun 25, 2026
Merged via the queue into main with commit 5b24475 Jun 25, 2026
39 checks passed
@rjvelazco
rjvelazco deleted the issue-36108-page-get-undefined-graphql-variables branch June 25, 2026 13:37
dcolina pushed a commit to dotCMS/core-workflow-test that referenced this pull request Jul 16, 2026
…page.get() (dotCMS#36373)

## Proposed Changes

Follow-up to dotCMS#36290 (already merged) addressing review feedback on
dotCMS#36108.

The first PR fixed the `undefined` values inside `graphql.variables`,
but `page.get()` still leaked `undefined` in two other places that break
Next.js Pages Router serialization
(`getServerSideProps`/`getStaticProps` throw on `undefined`).

### Fixes

1. **`errors`** — returned `undefined` when there were no GraphQL
errors. Now always returns an array (`[]` when none) so the response
stays JSON-serializable.
2. **`errors` type** — `DotCMSPageResponse.errors` is now **required**
(`errors: DotCMSGraphQLError[]`) to match the runtime contract, instead
of `errors?`. The raw backend response type (`DotGraphQLApiResponse`)
keeps `errors?` optional since the backend genuinely omits it on
success.
3. **`pageAsset.page.styleEditorSchemas`** — `graphqlToPageEntity` set
this to `undefined` when GraphQL returns `null` (outside EDIT_MODE) — a
guaranteed `undefined` in every LIVE page. Now the key is omitted
instead. Behavior is unchanged for populated/empty-array cases.
4. **Strengthened the regression test** to validate the **full response
object** serializes (not just `graphql.variables`), so a top-level
`undefined` field is caught next time.

### ⚠️ Behavioral change (release notes)

`page.get()` now always returns `errors` as an **array** (empty when
none) instead of `undefined`. Consumers should check
**`errors.length`**, not truthiness (`if (response.errors)` is now
always truthy). This was explicitly requested in the issue feedback
(`null` or `[]`).

The required-type change is safe for anyone consuming `page.get()`'s
return value (a stronger guarantee; `?.errors` still compiles). It only
affects code that manually *constructs* a `DotCMSPageResponse` literal —
verified `sdk-react`, `sdk-angular`, and `sdk-uve` still typecheck.

### Verification

- All 287 `sdk-client` tests pass.
- `sdk-client`, `sdk-react`, `sdk-angular`, `sdk-uve` build/typecheck
clean.
- Lint clean, format clean.
- Verified the `core/examples` are unaffected — they branch on a
thrown/caught singular `error` key, never the `errors` array.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI: Safe To Rollback Area : Frontend PR changes Angular/TypeScript frontend code Area : SDK PR changes SDK libraries

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

page.get() returns undefined in graphql.variables, breaking Next.js Pages Router serialization

2 participants