fix(angular): send required nullable query params as empty string without a paramsSerializer - #3716
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (11)
📒 Files selected for processing (15)
🚧 Files skipped from review as they are similar to previous changes (12)
📝 WalkthroughWalkthroughAngular query filtering now keeps required-nullable parameters on the wire as empty strings without a serializer and preserves literal ChangesAngular required-nullable query parameters
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant OpenAPI
participant AngularGenerator
participant filterParams
participant HttpClient
OpenAPI->>AngularGenerator: Define required nullable query parameter
AngularGenerator->>filterParams: Generate requiredNullableKeys logic
filterParams->>HttpClient: Send empty string without serializer
filterParams->>HttpClient: Preserve null with serializer
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Fixes Angular client query parameter filtering so required + nullable query params are no longer silently omitted when their runtime value is null and no paramsSerializer is configured. Instead, the generated filter logic emits '' (wire form ?key=) to keep the required key present, while preserving existing behavior (literal null) when a serializer is configured.
Changes:
- Updated generated Angular
filterParamslogic to emitpreserveRequiredNullables ? null : ''for required-nullablenullvalues. - Adjusted the inline IIFE param-filter generation to emit
requiredNullableParamKeys/ branch whenever required-nullable keys exist (not only with serializers), while avoiding TS6133 unused-variable cases. - Added unit, generation, and Angular runtime tests + fixtures; regenerated affected snapshots/samples; updated Angular docs to document the new behavior and the
paramsFilterescape hatch.
Reviewed changes
Copilot reviewed 62 out of 62 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/specifications/issue-3712.yaml | New minimal spec fixture for required+nullable query params. |
| tests/configs/angular.config.ts | Adds generation configs for issue-3712 variants (plain, httpResource, serializer). |
| tests/api-generation.spec.ts | Adds assertions verifying empty-string fallback and serializer preservation. |
| packages/core/src/generators/options.ts | Implements empty-string fallback + required-nullable branch emission logic. |
| packages/core/src/generators/options.test.ts | Adds unit coverage for IIFE/helper behavior and branch omission. |
| docs/content/docs/guides/angular.mdx | Documents required-nullable empty-string fallback + paramsFilter implications. |
| samples/angular-app/src/app/required-nullable-params.spec.ts | Adds wire-level Angular test asserting ?key= behavior via HttpTestingController. |
| tests/snapshots/default/issue-2998/requests/requests.service.ts | Snapshot update for new filterParams required-nullable behavior. |
| tests/snapshots/angular/zod-schema-response/endpoints.ts | Snapshot update for new filterParams required-nullable behavior. |
| tests/snapshots/angular/url-encode-parameters/endpoints.ts | Snapshot update for new filterParams required-nullable behavior. |
| tests/snapshots/angular/url-encode-parameters-http-resource/endpoints.ts | Snapshot update for new filterParams required-nullable behavior. |
| tests/snapshots/angular/tags/pets.ts | Snapshot update for new filterParams required-nullable behavior. |
| tests/snapshots/angular/tags-split/pets/pets.service.ts | Snapshot update for new filterParams required-nullable behavior. |
| tests/snapshots/angular/split/endpoints.service.ts | Snapshot update for new filterParams required-nullable behavior. |
| tests/snapshots/angular/petstore/endpoints.ts | Snapshot update for new filterParams required-nullable behavior. |
| tests/snapshots/angular/named-parameters/endpoints.ts | Snapshot update for new filterParams required-nullable behavior. |
| tests/snapshots/angular/multi-content-query-params/endpoints.ts | Snapshot update for new filterParams required-nullable behavior. |
| tests/snapshots/angular/issue-3712/model/listThingsParams.ts | New snapshot models for issue-3712 fixture. |
| tests/snapshots/angular/issue-3712/model/index.ts | New snapshot barrel for issue-3712 fixture models. |
| tests/snapshots/angular/issue-3712/endpoints.ts | New snapshot output verifying empty-string fallback without serializer. |
| tests/snapshots/angular/issue-3712-serializer/model/listThingsParams.ts | New snapshot models for issue-3712 serializer variant. |
| tests/snapshots/angular/issue-3712-serializer/model/index.ts | New snapshot barrel for issue-3712 serializer variant models. |
| tests/snapshots/angular/issue-3712-serializer/endpoints.ts | New snapshot output verifying literal null preserved when serializer configured. |
| tests/snapshots/angular/issue-3712-http-resource/model/listThingsParams.ts | New snapshot models for issue-3712 httpResource variant. |
| tests/snapshots/angular/issue-3712-http-resource/model/index.ts | New snapshot barrel for issue-3712 httpResource variant models. |
| tests/snapshots/angular/issue-3712-http-resource/endpoints.ts | New snapshot output verifying empty-string fallback in httpResource path. |
| tests/snapshots/angular/issue-3634/widget/widget.service.ts | Snapshot update for new filterParams required-nullable behavior. |
| tests/snapshots/angular/issue-3634/ab-widget/ab-widget.service.ts | Snapshot update for new filterParams required-nullable behavior. |
| tests/snapshots/angular/issue-3624/petstore.client.service.ts | Snapshot update for new filterParams required-nullable behavior. |
| tests/snapshots/angular/issue-3326/endpoints.ts | Snapshot update for new filterParams required-nullable behavior. |
| tests/snapshots/angular/issue-3326-serializer/endpoints.ts | Snapshot update for new filterParams required-nullable behavior. |
| tests/snapshots/angular/issue-3103/default/default.service.ts | Snapshot update for new filterParams required-nullable behavior. |
| tests/snapshots/angular/http-resource-zod/endpoints.ts | Snapshot update for new filterParams required-nullable behavior. |
| tests/snapshots/angular/http-resource-zod-disabled/endpoints.ts | Snapshot update for new filterParams required-nullable behavior. |
| tests/snapshots/angular/http-resource-tags/pets.ts | Snapshot update for new filterParams required-nullable behavior. |
| tests/snapshots/angular-query/use-prefetch/endpoints.ts | Snapshot update for new filterParams required-nullable behavior. |
| tests/snapshots/angular-query/url-encode-parameters/endpoints.ts | Snapshot update for new filterParams required-nullable behavior. |
| tests/snapshots/angular-query/tags-split/pets/pets.ts | Snapshot update for new filterParams required-nullable behavior. |
| tests/snapshots/angular-query/split/endpoints.ts | Snapshot update for new filterParams required-nullable behavior. |
| tests/snapshots/angular-query/basic/endpoints.ts | Snapshot update for new filterParams required-nullable behavior. |
| samples/angular-query/src/api/endpoints/pets/pets.ts | Regenerated sample output with updated filterParams behavior. |
| samples/angular-query/src/api/endpoints-zod/pets/pets.ts | Regenerated sample output with updated filterParams behavior. |
| samples/angular-query/src/api/endpoints-no-transformer/pets/pets.ts | Regenerated sample output with updated filterParams behavior. |
| samples/angular-query/src/api/endpoints-custom-instance/pets/pets.ts | Regenerated sample output with updated filterParams behavior + IIFE branch emission. |
| samples/angular-query/snapshots/api/endpoints/pets/pets.ts | Snapshot update for regenerated sample output. |
| samples/angular-query/snapshots/api/endpoints-zod/pets/pets.ts | Snapshot update for regenerated sample output. |
| samples/angular-query/snapshots/api/endpoints-no-transformer/pets/pets.ts | Snapshot update for regenerated sample output. |
| samples/angular-query/snapshots/api/endpoints-custom-instance/pets/pets.ts | Snapshot update for regenerated sample output + IIFE branch emission. |
| samples/angular-app/src/api/http-resource/pets/pets.service.ts | Regenerated sample output with updated filterParams behavior. |
| samples/angular-app/src/api/http-resource-zod/pets/pets.service.ts | Regenerated sample output with updated filterParams behavior. |
| samples/angular-app/src/api/http-client/pets/pets.service.ts | Regenerated sample output with updated filterParams behavior. |
| samples/angular-app/src/api/http-client-custom-params/pets/pets.service.ts | Regenerated sample output with updated filterParams behavior. |
| samples/angular-app/src/api/http-both/pets/pets.service.ts | Regenerated sample output with updated filterParams behavior. |
| samples/angular-app/src/api/http-both/pets/pets.resource.ts | Regenerated sample output with updated filterParams behavior. |
| samples/angular-app/src/api/endpoints-zod/pets/pets.service.ts | Regenerated sample output with updated filterParams behavior. |
| samples/angular-app/snapshots/api/http-resource/pets/pets.service.ts | Snapshot update for regenerated Angular app sample. |
| samples/angular-app/snapshots/api/http-resource-zod/pets/pets.service.ts | Snapshot update for regenerated Angular app sample. |
| samples/angular-app/snapshots/api/http-client/pets/pets.service.ts | Snapshot update for regenerated Angular app sample. |
| samples/angular-app/snapshots/api/http-client-custom-params/pets/pets.service.ts | Snapshot update for regenerated Angular app sample. |
| samples/angular-app/snapshots/api/http-both/pets/pets.service.ts | Snapshot update for regenerated Angular app sample. |
| samples/angular-app/snapshots/api/http-both/pets/pets.resource.ts | Snapshot update for regenerated Angular app sample. |
| samples/angular-app/snapshots/api/endpoints-zod/pets/pets.service.ts | Snapshot update for regenerated Angular app sample. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/core/src/generators/options.ts (1)
59-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider simplifying
emitRequiredNullableBranchto avoid dead code.When
preserveRequiredNullablesistrue(serializer configured) butrequiredNullableParamKeysis empty, the current condition still emits theSetdeclaration and theelse ifbranch. The branch never matches (empty Set), so this is dead code in the generated output. SincepreserveRequiredNullablesonly controls what value is assigned (nullvs''), not whether the branch should exist, the condition can be simplified to justrequiredNullableParamKeys.length > 0without changing behavior.♻️ Proposed simplification
const emitRequiredNullableBranch = - preserveRequiredNullables || requiredNullableParamKeys.length > 0; + requiredNullableParamKeys.length > 0;This eliminates dead code in the common case where a
paramsSerializeris configured but the spec has no required+nullable query params.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/generators/options.ts` around lines 59 - 60, Update emitRequiredNullableBranch in the generator options logic to depend only on requiredNullableParamKeys.length > 0. Preserve preserveRequiredNullables for selecting the assigned value within the emitted branch, but avoid generating the Set declaration and else-if branch when the key collection is empty.samples/angular-app/src/app/required-nullable-params.spec.ts (1)
31-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a serializer-path test for completeness.
The test correctly validates the no-serializer empty-string behavior. Adding a parallel test using the
http-client-custom-paramsservice (wherepreserveRequiredNullables=true) would verify thatnullis preserved on the wire when a serializer is configured, completing the coverage for both code paths.Based on learnings, files under
samples/are treated as sample/demo data, so this is an optional suggestion only.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@samples/angular-app/src/app/required-nullable-params.spec.ts` around lines 31 - 47, Add a parallel spec covering the serializer path through the http-client-custom-params service with preserveRequiredNullables=true. Pass a required nullable parameter as null, issue the request, and assert that the serialized request preserves null on the wire while the existing non-null parameter remains unchanged.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/core/src/generators/options.ts`:
- Around line 59-60: Update emitRequiredNullableBranch in the generator options
logic to depend only on requiredNullableParamKeys.length > 0. Preserve
preserveRequiredNullables for selecting the assigned value within the emitted
branch, but avoid generating the Set declaration and else-if branch when the key
collection is empty.
In `@samples/angular-app/src/app/required-nullable-params.spec.ts`:
- Around line 31-47: Add a parallel spec covering the serializer path through
the http-client-custom-params service with preserveRequiredNullables=true. Pass
a required nullable parameter as null, issue the request, and assert that the
serialized request preserves null on the wire while the existing non-null
parameter remains unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2b8ef410-ddd2-4c37-9ae7-db71a6da36aa
⛔ Files ignored due to path filters (44)
samples/angular-app/__snapshots__/api/endpoints-zod/pets/pets.service.tsis excluded by!**/__snapshots__/**samples/angular-app/__snapshots__/api/http-both/pets/pets.resource.tsis excluded by!**/__snapshots__/**samples/angular-app/__snapshots__/api/http-both/pets/pets.service.tsis excluded by!**/__snapshots__/**samples/angular-app/__snapshots__/api/http-client-custom-params/pets/pets.service.tsis excluded by!**/__snapshots__/**samples/angular-app/__snapshots__/api/http-client/pets/pets.service.tsis excluded by!**/__snapshots__/**samples/angular-app/__snapshots__/api/http-resource-zod/pets/pets.service.tsis excluded by!**/__snapshots__/**samples/angular-app/__snapshots__/api/http-resource/pets/pets.service.tsis excluded by!**/__snapshots__/**samples/angular-query/__snapshots__/api/endpoints-custom-instance/pets/pets.tsis excluded by!**/__snapshots__/**samples/angular-query/__snapshots__/api/endpoints-no-transformer/pets/pets.tsis excluded by!**/__snapshots__/**samples/angular-query/__snapshots__/api/endpoints-zod/pets/pets.tsis excluded by!**/__snapshots__/**samples/angular-query/__snapshots__/api/endpoints/pets/pets.tsis excluded by!**/__snapshots__/**tests/__snapshots__/angular-query/basic/endpoints.tsis excluded by!**/__snapshots__/**tests/__snapshots__/angular-query/split/endpoints.tsis excluded by!**/__snapshots__/**tests/__snapshots__/angular-query/tags-split/pets/pets.tsis excluded by!**/__snapshots__/**tests/__snapshots__/angular-query/url-encode-parameters/endpoints.tsis excluded by!**/__snapshots__/**tests/__snapshots__/angular-query/use-prefetch/endpoints.tsis excluded by!**/__snapshots__/**tests/__snapshots__/angular/http-resource-tags/pets.tsis excluded by!**/__snapshots__/**tests/__snapshots__/angular/http-resource-zod-disabled/endpoints.tsis excluded by!**/__snapshots__/**tests/__snapshots__/angular/http-resource-zod/endpoints.tsis excluded by!**/__snapshots__/**tests/__snapshots__/angular/issue-3103/default/default.service.tsis excluded by!**/__snapshots__/**tests/__snapshots__/angular/issue-3326-serializer/endpoints.tsis excluded by!**/__snapshots__/**tests/__snapshots__/angular/issue-3326/endpoints.tsis excluded by!**/__snapshots__/**tests/__snapshots__/angular/issue-3624/petstore.client.service.tsis excluded by!**/__snapshots__/**tests/__snapshots__/angular/issue-3634/ab-widget/ab-widget.service.tsis excluded by!**/__snapshots__/**tests/__snapshots__/angular/issue-3634/widget/widget.service.tsis excluded by!**/__snapshots__/**tests/__snapshots__/angular/issue-3712-http-resource/endpoints.tsis excluded by!**/__snapshots__/**tests/__snapshots__/angular/issue-3712-http-resource/model/index.tsis excluded by!**/__snapshots__/**tests/__snapshots__/angular/issue-3712-http-resource/model/listThingsParams.tsis excluded by!**/__snapshots__/**tests/__snapshots__/angular/issue-3712-serializer/endpoints.tsis excluded by!**/__snapshots__/**tests/__snapshots__/angular/issue-3712-serializer/model/index.tsis excluded by!**/__snapshots__/**tests/__snapshots__/angular/issue-3712-serializer/model/listThingsParams.tsis excluded by!**/__snapshots__/**tests/__snapshots__/angular/issue-3712/endpoints.tsis excluded by!**/__snapshots__/**tests/__snapshots__/angular/issue-3712/model/index.tsis excluded by!**/__snapshots__/**tests/__snapshots__/angular/issue-3712/model/listThingsParams.tsis excluded by!**/__snapshots__/**tests/__snapshots__/angular/multi-content-query-params/endpoints.tsis excluded by!**/__snapshots__/**tests/__snapshots__/angular/named-parameters/endpoints.tsis excluded by!**/__snapshots__/**tests/__snapshots__/angular/petstore/endpoints.tsis excluded by!**/__snapshots__/**tests/__snapshots__/angular/split/endpoints.service.tsis excluded by!**/__snapshots__/**tests/__snapshots__/angular/tags-split/pets/pets.service.tsis excluded by!**/__snapshots__/**tests/__snapshots__/angular/tags/pets.tsis excluded by!**/__snapshots__/**tests/__snapshots__/angular/url-encode-parameters-http-resource/endpoints.tsis excluded by!**/__snapshots__/**tests/__snapshots__/angular/url-encode-parameters/endpoints.tsis excluded by!**/__snapshots__/**tests/__snapshots__/angular/zod-schema-response/endpoints.tsis excluded by!**/__snapshots__/**tests/__snapshots__/default/issue-2998/requests/requests.service.tsis excluded by!**/__snapshots__/**
📒 Files selected for processing (18)
docs/content/docs/guides/angular.mdxpackages/core/src/generators/options.test.tspackages/core/src/generators/options.tssamples/angular-app/src/api/endpoints-zod/pets/pets.service.tssamples/angular-app/src/api/http-both/pets/pets.resource.tssamples/angular-app/src/api/http-both/pets/pets.service.tssamples/angular-app/src/api/http-client-custom-params/pets/pets.service.tssamples/angular-app/src/api/http-client/pets/pets.service.tssamples/angular-app/src/api/http-resource-zod/pets/pets.service.tssamples/angular-app/src/api/http-resource/pets/pets.service.tssamples/angular-app/src/app/required-nullable-params.spec.tssamples/angular-query/src/api/endpoints-custom-instance/pets/pets.tssamples/angular-query/src/api/endpoints-no-transformer/pets/pets.tssamples/angular-query/src/api/endpoints-zod/pets/pets.tssamples/angular-query/src/api/endpoints/pets/pets.tstests/api-generation.spec.tstests/configs/angular.config.tstests/specifications/issue-3712.yaml
@orval/angular
@orval/axios
@orval/core
@orval/effect
@orval/fetch
@orval/hono
@orval/mcp
@orval/mock
orval
@orval/query
@orval/solid-start
@orval/swr
@orval/zod
commit: |
|
Merge Conflicts here too |
…hout a paramsSerializer
A query parameter that is required and nullable per the OpenAPI schema was
silently dropped from the request when its runtime value was null, unless a
paramsSerializer mutator was configured. Angular's HttpParams type structurally
excludes null, so the no-serializer path now serializes null as an empty
string ('' -> "key=") instead of dropping the key, keeping the request
contract-complete. The serializer path still preserves literal null for the
serializer to encode (unchanged).
Also fixes the inline IIFE generator to emit the requiredNullableParamKeys Set
and its reading branch whenever required-nullable keys exist, not only when a
serializer is configured, while preserving the orval-labs#3593 TS6133 guarantee.
Fixes orval-labs#3712
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
aef12c0 to
e3164ee
Compare
Summary
Fixes #3712.
For Angular
HttpClientservices andhttpResourcefunctions, a query parameter that is bothrequired: trueand nullable per the OpenAPI schema was silently dropped from the request whenever its runtime value wasnull— unless a customparamsSerializermutator was configured. The request then went out without a parameter the spec documents as required.Root cause
preserveRequiredNullableswas only ever passed astruein theparamsSerializerbranches (packages/core/src/generators/options.tsand thehttpResourcebuilder). That gating exists for a real reason: Angular'sparamstype (HttpParams | Record<string, string | number | boolean | ReadonlyArray<...>>) structurally excludesnull, so a literalnullis only type-safe when a serializer consumes it before it reaches Angular. The naive fix of passingtrueunconditionally does not compile (see the analysis in #3712).Fix
filterParamshelper now emitsfilteredParams[key] = preserveRequiredNullables ? null : '';for required-nullablenullvalues: without a serializer,nullis sent as an empty string (?key=) so the key still reaches the wire; with a serializer, the literalnullis preserved for the serializer to encode (unchanged behavior).requiredNullableParamKeysSet+ its reading branch whenever required-nullable keys exist (previously only when a serializer was configured), while preserving the TS6133: 'requiredNullableParamKeys' is declared but its value is never read. #3593 guarantee that theSetis only declared when the branch reading it is emitted (no TS6133).''assignment fits the existing non-nullable overload offilterParams, so the four overloads andfilteredParamValueTypeare unchanged — no type widening.Tests
packages/core/src/generators/options.test.ts(empty-string fallback on the IIFE path, branch omission when no required-nullable keys, shared-helper body).tests/specifications/issue-3712.yaml+ threetests/configs/angular.config.tsentries (issue3712,issue3712HttpResource,issue3712Serializer, modeled on the issue-3326 pattern) withtests/api-generation.spec.tsassertions for all three variants.samples/angular-app/src/app/required-nullable-params.spec.tsusesHttpTestingControlleragainst the generated no-serializerPetsServiceand assertsreq.params.get('requirednullableString') === ''— i.e. the key actually reaches the wire.tests/__snapshots__/angular/**,angular-query/**,default/issue-2998,samples/angular-app,samples/angular-query) — every diff is the same expected helper-body hunk.Docs
docs/content/docs/guides/angular.mdx"Query parameter filtering": documents the empty-string fallback, notes thatoverride.paramsFilterreplaces this fallback (escape hatch back to drop-silently semantics), and the referencedefault-filter-behaviour.tssnippet now mirrors the emitted ternary exactly.Backwards compatibility
nullstill preserved).override.paramsFilter.Validation
bun run build:release✅ ·bun run lint✅ · core 2105/2105, angular 222/222, query 140/140 ✅ · snapshots 5313 tests, update + verify both green ✅ · angular-appng test17/17 incl. the new wire-level spec ✅ (one pre-existing macOS-only/tmprealpath flake inpackages/orvaloptions.test.ts, unrelated — package untouched).Related issues
preserveRequiredNullablesis serializer-gated (literalnulldoesn't compile otherwise)Setguarantee this change preservesnullas literal"null"and it was treated as a bug — evidence against the literal-"null"alternative🤖 Generated with Claude Code
Summary by CodeRabbit
null.paramsSerializeris used,nullis sent as an empty value (?key=); when a serializer is configured, the literalnullis preserved.required+nullablenullhandling and how filtering overrides affect it.