Copilot review mirror for orval-labs/orval#3716 (issue #3712) — do not merge#8
Open
the-ult wants to merge 11 commits into
Open
Copilot review mirror for orval-labs/orval#3716 (issue #3712) — do not merge#8the-ult wants to merge 11 commits into
the-ult wants to merge 11 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates Orval’s Angular client generation to ensure required + nullable query parameters are not silently omitted when their runtime value is null (issue orval-labs#3712). It adjusts the generated Angular filterParams logic to emit an empty string when no paramsSerializer is configured, while preserving literal null when a serializer is present, and adds tests/docs/snapshots to lock in the behavior.
Changes:
- Update Angular query-parameter filtering generation to handle required+nullable
nullvalues via''fallback (ornullpassthrough whenparamsSerializeris configured). - Add a dedicated OpenAPI spec + angular test configs and API-generation assertions for issue orval-labs#3712.
- Update docs, samples, and snapshots to reflect and validate the new serialization behavior.
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 | Adds a minimal spec covering required+nullable vs optional+nullable query params. |
| tests/configs/angular.config.ts | Adds generation targets for issue-3712 (default, httpResource, and serializer variants). |
| tests/api-generation.spec.ts | Asserts generated Angular output preserves required-nullables correctly (empty string vs literal null depending on serializer). |
| tests/snapshots/default/issue-2998/requests/requests.service.ts | Updates snapshot filterParams required-nullable handling to empty-string fallback when no serializer. |
| tests/snapshots/angular/zod-schema-response/endpoints.ts | Snapshot update for required-nullable handling in filterParams. |
| tests/snapshots/angular/url-encode-parameters/endpoints.ts | Snapshot update for required-nullable handling in filterParams. |
| tests/snapshots/angular/url-encode-parameters-http-resource/endpoints.ts | Snapshot update for required-nullable handling in filterParams. |
| tests/snapshots/angular/tags/pets.ts | Snapshot update for required-nullable handling in filterParams. |
| tests/snapshots/angular/tags-split/pets/pets.service.ts | Snapshot update for required-nullable handling in filterParams. |
| tests/snapshots/angular/split/endpoints.service.ts | Snapshot update for required-nullable handling in filterParams. |
| tests/snapshots/angular/petstore/endpoints.ts | Snapshot update for required-nullable handling in filterParams. |
| tests/snapshots/angular/named-parameters/endpoints.ts | Snapshot update for required-nullable handling in filterParams. |
| tests/snapshots/angular/multi-content-query-params/endpoints.ts | Snapshot update for required-nullable handling in filterParams. |
| tests/snapshots/angular/issue-3712/model/listThingsParams.ts | New snapshot model for issue-3712 params typing (`string |
| tests/snapshots/angular/issue-3712/model/index.ts | New snapshot barrel export for issue-3712 model. |
| tests/snapshots/angular/issue-3712/endpoints.ts | New snapshot service showing required-nullable empty-string fallback without serializer. |
| tests/snapshots/angular/issue-3712-serializer/model/listThingsParams.ts | New snapshot model for serializer variant. |
| tests/snapshots/angular/issue-3712-serializer/model/index.ts | New snapshot barrel export for serializer variant. |
| tests/snapshots/angular/issue-3712-serializer/endpoints.ts | New snapshot showing literal null passthrough when paramsSerializer is configured. |
| tests/snapshots/angular/issue-3712-http-resource/model/listThingsParams.ts | New snapshot model for httpResource variant. |
| tests/snapshots/angular/issue-3712-http-resource/model/index.ts | New snapshot barrel export for httpResource variant. |
| tests/snapshots/angular/issue-3712-http-resource/endpoints.ts | New snapshot showing required-nullable empty-string fallback in httpResource generator. |
| tests/snapshots/angular/issue-3634/widget/widget.service.ts | Snapshot update for required-nullable handling in filterParams. |
| tests/snapshots/angular/issue-3634/ab-widget/ab-widget.service.ts | Snapshot update for required-nullable handling in filterParams. |
| tests/snapshots/angular/issue-3624/petstore.client.service.ts | Snapshot update for required-nullable handling in filterParams. |
| tests/snapshots/angular/issue-3326/endpoints.ts | Snapshot update for required-nullable handling in filterParams. |
| tests/snapshots/angular/issue-3326-serializer/endpoints.ts | Snapshot update for required-nullable handling in filterParams. |
| tests/snapshots/angular/issue-3103/default/default.service.ts | Snapshot update for required-nullable handling in filterParams. |
| tests/snapshots/angular/http-resource-zod/endpoints.ts | Snapshot update for required-nullable handling in filterParams. |
| tests/snapshots/angular/http-resource-zod-disabled/endpoints.ts | Snapshot update for required-nullable handling in filterParams. |
| tests/snapshots/angular/http-resource-tags/pets.ts | Snapshot update for required-nullable handling in filterParams. |
| tests/snapshots/angular-query/use-prefetch/endpoints.ts | Snapshot update for required-nullable handling in filterParams. |
| tests/snapshots/angular-query/url-encode-parameters/endpoints.ts | Snapshot update for required-nullable handling in filterParams. |
| tests/snapshots/angular-query/tags-split/pets/pets.ts | Snapshot update for required-nullable handling in filterParams. |
| tests/snapshots/angular-query/split/endpoints.ts | Snapshot update for required-nullable handling in filterParams. |
| tests/snapshots/angular-query/basic/endpoints.ts | Snapshot update for required-nullable handling in filterParams. |
| samples/angular-query/src/api/endpoints/pets/pets.ts | Updates sample generated output to match required-nullable empty-string fallback behavior. |
| samples/angular-query/src/api/endpoints-zod/pets/pets.ts | Updates sample generated output to match required-nullable empty-string fallback behavior. |
| samples/angular-query/src/api/endpoints-no-transformer/pets/pets.ts | Updates sample generated output to match required-nullable empty-string fallback behavior. |
| samples/angular-query/src/api/endpoints-custom-instance/pets/pets.ts | Updates custom-instance sample generation to preserve required-nullable keys (empty-string fallback). |
| samples/angular-query/snapshots/api/endpoints/pets/pets.ts | Snapshot update for sample output. |
| samples/angular-query/snapshots/api/endpoints-zod/pets/pets.ts | Snapshot update for sample output. |
| samples/angular-query/snapshots/api/endpoints-no-transformer/pets/pets.ts | Snapshot update for sample output. |
| samples/angular-query/snapshots/api/endpoints-custom-instance/pets/pets.ts | Snapshot update for sample output. |
| samples/angular-app/src/app/required-nullable-params.spec.ts | Adds an Angular app-level test ensuring null required params reach the wire as ?key=. |
| samples/angular-app/src/api/http-resource/pets/pets.service.ts | Updates generated app client snapshot to required-nullable empty-string fallback. |
| samples/angular-app/src/api/http-resource-zod/pets/pets.service.ts | Updates generated app client snapshot to required-nullable empty-string fallback. |
| samples/angular-app/src/api/http-client/pets/pets.service.ts | Updates generated app client snapshot to required-nullable empty-string fallback. |
| samples/angular-app/src/api/http-client-custom-params/pets/pets.service.ts | Updates generated app client snapshot to required-nullable empty-string fallback. |
| samples/angular-app/src/api/http-both/pets/pets.service.ts | Updates generated app client snapshot to required-nullable empty-string fallback. |
| samples/angular-app/src/api/http-both/pets/pets.resource.ts | Updates generated app client snapshot to required-nullable empty-string fallback. |
| samples/angular-app/src/api/endpoints-zod/pets/pets.service.ts | Updates generated app client snapshot to required-nullable empty-string fallback. |
| samples/angular-app/snapshots/api/http-resource/pets/pets.service.ts | Snapshot update for app output. |
| samples/angular-app/snapshots/api/http-resource-zod/pets/pets.service.ts | Snapshot update for app output. |
| samples/angular-app/snapshots/api/http-client/pets/pets.service.ts | Snapshot update for app output. |
| samples/angular-app/snapshots/api/http-client-custom-params/pets/pets.service.ts | Snapshot update for app output. |
| samples/angular-app/snapshots/api/http-both/pets/pets.service.ts | Snapshot update for app output. |
| samples/angular-app/snapshots/api/http-both/pets/pets.resource.ts | Snapshot update for app output. |
| samples/angular-app/snapshots/api/endpoints-zod/pets/pets.service.ts | Snapshot update for app output. |
| packages/core/src/generators/options.ts | Implements required-nullable handling (empty-string fallback vs null passthrough) in Angular param filtering generation. |
| packages/core/src/generators/options.test.ts | Adds unit tests covering the new required-nullable behavior for both IIFE and shared-helper paths. |
| docs/content/docs/guides/angular.mdx | Documents the new required-nullable behavior and how paramsFilter interacts with it. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…d object keys (orval-labs#3692) * fix(core,zod): escape spec-controlled strings in generated template literals Unescaped spec values (servers[].url, path segments, schema defaults) were baked into generated template literals, allowing code injection via backtick or ${ in an attacker-controlled OpenAPI spec. Uses jsesc with quotes: 'backtick' to escape at three boundaries: - getRoute(): raw OpenAPI path before param processing - getFullRoute(): resolved server URL after variable substitution - formatDefaultValue(): schema default values in zod generation Addresses: GHSA-88f2-fpv8-89q2, GHSA-w727-8j6c-2rj4, GHSA-3575-w9fc-c2j6, GHSA-2h9g-j24r-h63g, GHSA-8j6p-r8jg-mxqh, GHSA-p4cg-3328-rvfg * fix(zod): escape object keys via JSON.stringify in zod.object generation Unescaped schema property and parameter names were emitted as double-quoted keys in zod.object({...}), allowing computed property key injection via " in the name. Replaces "${key}" with JSON.stringify(key) at all 5 render sites. Addresses: GHSA-6437-gxhq-pqv8, GHSA-653q-5476-x79g, GHSA-6mr6-jvcr-2f25 * fix(core): escape single-quoted object keys in getKey via jsStringLiteralEscape Unescaped schema property names were wrapped in single quotes by getKey(), allowing computed property key injection via ' in the name in MSW mock output. Wraps the key body with jsStringLiteralEscape before quoting. Addresses: GHSA-2w86-xfrc-g85r * fix(query,zod): escape raw route prefix and prove backtick-in-key safety mutation-generator.ts passed a raw spec path prefix to getFullRoute, bypassing getRoute's escaping. Wrap with getRoute to close the gap. Add test proving JSON.stringify-wrapped object keys are safe against backtick injection (backtick is harmless inside double-quoted strings). * fix: escape object-default keys and add ${} path test - Escape object-default keys with JSON.stringify in zod schema defaults (same fix as zod.object keys) - Add test verifying ${globalThis.X} in path segments is not re-interpreted as interpolation - Update existing test expectations for quoted object keys * fix(core): prevent ${} re-interpretation in getRoutePath and escape single quotes in getRouteAsArray getRoutePath: after jsesc escapes ${ to \${, the remaining {evil} was mistaken for an OpenAPI path param and re-converted to ${evil}. Add early-return when { is preceded by $. getRouteAsArray: segments wrapped in single-quoted strings without escaping '. A spec path containing ' would break out. Now escapes single quotes at both wrap sites. * fix(core): handle ${} in getRoutePath and escaped tags in getRouteAsArray getRoutePath: skip past ${...} block and continue processing remaining suffix so later {param} segments are still converted. getRouteAsArray: add (?<!\) to split/match regexes so jsesc-escaped ${...} is treated as literal text, preventing standalone backslash from breaking single-quote wrapping. * fix(core): escape per-segment in getRoute to preserve params after ${...}
…#3685) (orval-labs#3696) The axios generator used a module-level Map (returnTypesToWrite) to collect *Result type declarations during implementation generation, then read them during footer generation. In tags-split mode, all implementations are generated first (populating the map), then footers are generated per-tag. When two operations from different tags shared the same operationName (via override), the second overwrote the first's entry, causing the wrong *Result type to be emitted in the first tag's footer. Fix: eliminate the module-level map entirely. The return type generator is now returned from generateAxiosImplementation as part of GeneratorClient.returnType, stored on GeneratorOperation.types.result, and passed to the footer via the new optional operations param on ClientFooterBuilder/GeneratorClientFooter. The footer reads from operations[].types.result instead of the side-effect map, ensuring each tag's footer only emits types for its own operations.
…orval-labs#3700) (orval-labs#3701) An optional OpenAPI request body maps to an optional `Signal` parameter on the generated `httpResource` factory. Previously the factory accessed it unconditionally (`body: bodySignal?.()`), so omitting the signal still produced a request object with an undefined body and eagerly fired the resource instead of leaving it idle. Emit an idle guard at the top of the request factory (`if (!bodySignal) return undefined;`) for optional direct request bodies, matching Angular's `undefined`-request contract. Past the guard the signal is known to be defined, so it is invoked directly (`body: bodySignal()`). The guard is only emitted on the single-content path, where the request is built lazily inside the factory. The multi-content path assembles the request eagerly at the function-body level (where returning `undefined` would violate the `HttpResourceRef` return type) and keeps the runtime-safe `?.()` form. Co-authored-by: The Ult <the-ult@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…content-type resources (orval-labs#3715) * fix(angular): read signals inside the httpResource factory for multi-content-type resources The multi-content-type httpResource template built the request descriptor (path/query/version/body signal reads, normalizeRequest, Accept-header merge) eagerly, before httpResource() was called, and the factory only spread the pre-resolved object. Angular's httpResource tracks only signal reads that happen during factory execution, so these resources fetched once at creation and never refetched when their input signals changed. The request construction now lives in a buildRequest closure passed as the factory, matching the single-content-type output; accept-based branch selection stays eager. Also guards withSignal's default-detection against core's `default: false` boolean sentinel for query/body/header props, which wrongly rendered spec-required Signal parameters as optional (and produced invalid TS1016 overloads when combined with a trailing required accept parameter). Required query-param signals are now correctly non-optional — a compile-time tightening for consumers whose specs mark query params required. Fixes orval-labs#3713 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(angular): dispatch blob accept values to httpResource.blob in multi-content resources The multi-content accept dispatch covered json/text/arrayBuffer but never selected the blob factory, even though blob overloads are generated and blobType participates in the fallback chain — accept values like image/png fell through to the default branch and created the wrong resource kind. Emit a conditional blob branch (accept startsWith image/ or includes blob), mirroring the arrayBuffer branch and getHttpResourceFactory priority. Addresses Copilot review on orval-labs#3715. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: The Ult <the-ult@users.noreply.github.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
… in arrayItems factories (orval-labs#3709) * fix(mock): use exported element aliases for top-level array responses in arrayItems factories (orval-labs#3706) getArrayItemFactoryNames fabricated `${pascal(operationId)}${pascal(propertyName)}Item` for top-level array responses (no parentName), producing type names the schema generator never emits — unexported/undeclared identifiers that fail tsc. Now the mock generator derives the real emitted alias instead: for inline top-level arrays, reuse the alias before the trailing `[]` (bailing on non-identifier bases like unions or `readonly`); for `$ref`'d array schemas, reuse `<RefName><itemSuffix>` (bailing when the items are a multi-ref `allOf` with no direct properties, since core emits no alias there). All previously-working branches (itemsRef, parentName) are byte-identical. Fixes orval-labs#3706 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * style(mock): apply formatter to array-item-factory tests Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(mock): derive element aliases for nullable top-level array responses Copilot review: nullable top-level arrays reach the no-parentName branch with a ' | null'-suffixed definition ('CatalogItems | null'), which fell into the bare-ref-name path and fabricated phantom names. The suffix is now stripped before branch selection, and the bare-name branch gained an identifier guard that bails to the always-correct inline body for any non-identifier shape. Fixture gains a nullable top-level array operation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: The Ult <the-ult@users.noreply.github.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…method names (orval-labs#3684) (orval-labs#3693) The override.operationName callback can now return [methodName, typeNameBase] to independently control generated function/hook names from TypeScript type identifier names (*Params, *Body, *Error, *Result, *Accept, zod/hono/effect schema names). Returning a string (existing behavior) keeps both identical. This enables gateway-aggregated specs with tags-split + splitByTags + indexFiles where bare method names are safe per-tag but type names need global uniqueness to avoid barrel-level TS2300 collisions. Fully backward compatible — existing callbacks returning string work unchanged. The tuple form is opt-in.
… instead of dropping them (orval-labs#3708) * fix(angular): serialize object query params per OpenAPI style/explode instead of dropping them (orval-labs#3705) Plain-object query params were silently dropped by the generated filterParams helper unless a paramsSerializer was configured. The core getters now derive a per-param strategy from the spec (form+explode:true -> flatten, form+explode:false -> comma-joined, deepObject -> bracketed keys) and both Angular outputs (HttpClient services and httpResource functions) serialize accordingly through the shared helper. Parameter types intentionally stay nested/keyed by the declared param name. paramsFilter/paramsSerializer precedence is unchanged, and override.angular.queryObjectSerialization: 'legacy' restores the old dropping behavior. Fixes orval-labs#3705 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(angular): include primitive arrays in comma object-param serialization; apply formatter Addresses Copilot review: the comma strategy (style: form, explode: false) dropped array-valued object properties at both emission sites. Arrays are now filtered to primitives and appended after the property name, matching the documented 'itemReferences,a,b,pageNumber,1' shape; runtime-eval tests cover the array and null/object-filtering cases. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: The Ult <the-ult@users.noreply.github.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…tpResource helpers (orval-labs#3717) * feat(angular): expose request headers and HttpContext on generated httpResource helpers Generated httpResource helpers only accepted HttpResourceOptions, while the generated HttpClient service methods already accept headers/context via HttpClientOptions. The trailing options bag now also accepts an OrvalHttpResourceRequestExtension: - headers: extra request headers (static or reactive function form), merged over generated ones via an emitted mergeOrvalResourceHeaders helper - context: Angular HttpContext for interceptor policy (static or function) - request: escape-hatch callback over the final HttpResourceRequest All extension reads happen inside the httpResource reactive factory, so signal-driven headers/context retrigger requests. The generated Accept header is always re-applied last in multi-content-type helpers. OpenAPI in:header parameters were already supported via output.headers; this adds test coverage and documentation for that path. Also fixes two latent generator bugs surfaced by the new coverage: required body/query/header props rendered as optional Signals (core's `default: false` sentinel), and the both-mode extra-file writer misrouting external imports through the schema-import bucket. Fixes orval-labs#3710 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(angular): clarify request-extension options-bag position in helper calls The extension rides on the trailing options parameter; multi-content helpers and output.headers operations generate accept/header arguments before it. Show both call shapes with the real petstore-sample signature. Addresses Copilot review on orval-labs#3717. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * chore(angular): regenerate httpResource fixtures after rebase onto master Folds request-extension application into the reactive buildRequest closure (from orval-labs#3715's signal-inside-factory fix) for multi-content-type resources, and updates the sample/snapshot fixtures accordingly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(angular): fix multi-content Accept-merge assertion after buildRequest merge Rebasing folded the request-extension application into orval-labs#3715's shared buildRequest closure, so the multi-content-type template no longer emits a separate acceptHeaders local — the Accept-header merge is inlined in buildRequest's return statement instead. Update the assertion to match. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: The Ult <the-ult@users.noreply.github.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Co-authored-by: melloware <4399574+melloware@users.noreply.github.com>
…ies (orval-labs#3721) * fix(core): skip Required<Pick> for required keys missing from properties Orval 8.21 can emit invalid TypeScript when required omits properties. This breaks compilation with TS2344. Only apply Required<Pick> for keys on subschema properties. Add a petstore repro plus regression tests. * test(core): assert context.spec.components in combine test Use a non-null assertion when spreading schemas in contextWithTagMetadata. * test: add snapshots and fix petstore repro assertions Avoid Required<Pick> in the OpenAPI description so generated headers do not trip assertions. Commit default snapshots for petstore-nullable-allof-required and assert on the property type only. * fix(core): use Extract guard for unresolved required keys Keep plain Required<Pick> for top-level properties and use Extract for nested or ghost keys. Rename the regression fixture to issue-3722.yaml and refresh snapshots. * fix(core): add Extract guard and issue-3722 regression fixture Apply unresolved required keys via Extract while keeping plain Pick for top-level keys. Add issue-3722.yaml snapshots and tests for ghost and nested required keys.
…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>
the-ult
force-pushed
the
fix/3712-angular-required-nullable-params
branch
from
July 13, 2026 21:12
aef12c0 to
e3164ee
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Mirror of orval-labs#3716 for requesting a Copilot review. Do not merge.