Skip to content

Commit b30537d

Browse files
the-ultThe UltclaudeCopilot
authored
fix(angular): place body before accept in multi-content overload signatures (#3354)
* fix(angular): place body before accept in multi-content overload signatures For operations with a request body, path parameters, and multiple response content types, the generated overloads incorrectly placed the `accept` literal parameter before the body. This broke TypeScript callers (API break) and could throw at runtime when a body object was passed into the `accept` position and reached `accept.includes(...)`. Split props into three ordered buckets — required non-body, body, optional non-body — so all generated signatures follow the natural call shape: `method(pathParams, body, accept, optionalParams, options)` Optional body params in per-content-type overloads are rendered as positionally required (`name: Type | undefined`) to satisfy TS1016, which forbids required params after optional ones. Closes #3349 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test(angular): add regression tests and samples for body param order fix - Extend `angular-multi-content-query-params` spec with a `confirmReservation` POST endpoint (optional body + multi-content) and update its snapshots to assert body-before-accept order in all generated overloads - Add `updatePetById` (PUT, required body) and `patchPetById` (PATCH, optional body) to the Angular sample petstore spec, both with multi-content responses, to cover both required and optional body variants end-to-end - Regenerate all affected Angular sample outputs (http-client, http-both, http-resource, endpoints-zod, http-resource-zod) from the updated spec Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test(angular): cover required body before accept in multi-content overloads Complements the optional-body regression test for #3349: verifies that a required body param also appears before `accept` in all generated overload signatures and is not widened to `Type | undefined` (the TS1016 transform only fires for optional props). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * refactor(angular): anchor optional-marker stripping to body param identifier Replace brittle p.definition.replace('?:', ':') with an identifier-anchored slice using `${p.name}?:` as the prefix. This guarantees we only strip the parameter's own optional marker, never a '?:' that may appear elsewhere in the type (e.g. mapped or conditional types). Also refresh angular-app snapshot fixtures for the new updatePetById / patchPetById endpoints introduced in this PR. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * refactor(angular): extract partitionPropsForMultiContent helper Replace three duplicated filter-by-(required/body) blocks in generateHttpClientImplementation with a single partitionPropsForMultiContent helper. Both the type-level overload and the implementation site now share the same partitioning logic, eliminating drift risk between the two and addressing PR #3354 review comments 2, 3, and 4. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test(angular): assert TS1016-safe optional-body form in overloads The existing regression test only verified ordering of body vs accept via indexOf. Add a positive assertion that the per-content-type overloads render `confirmReservationBody: ... | undefined` (positionally required, type widened) immediately followed by a required `accept` literal, plus a negative assertion that no `confirmReservationBody?:` is followed by a required `accept` literal anywhere — which would re-introduce TS1016 (required parameter cannot follow optional). Addresses PR #3354 review comment 5. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(samples): add missing UpdatePetByIdAccept and PatchPetByIdAccept exports The http-resource and http-resource-zod sample variants referenced UpdatePetByIdAccept and PatchPetByIdAccept in their updatePetById and patchPetById signatures but were missing the corresponding type/const definitions, causing TS compilation errors. Added the type unions and const objects after ShowPetByIdAccept in: - src/api/http-resource/pets/pets.service.ts - src/api/http-resource-zod/pets/pets.service.ts - __snapshots__/api/http-resource/pets/pets.service.ts - __snapshots__/api/http-resource-zod/pets/pets.service.ts * test(angular): add /confirm suffix to confirmReservation route fixtures Route strings in the regression test now match the OpenAPI fixture path '/reservations/{token}/confirm', making the test self-consistent. * fix(angular): treat falsy defaults as present in prop partitioning Use a nullish check (== null) instead of a truthiness check when deciding whether a required non-body prop has no default value. * fix(angular): tighten undefined detection in overload body typing Replace required.includes('undefined') with a word-boundary regex check to avoid brittle substring matches while preserving behavior. * fix(angular): unblock multi-content ci Restore the original required-parameter partition check in the Angular HttpClient generator so the multi-content refactor no longer trips unicorn/no-null. Emit Accept helper types for httpResource mutation methods as well. This keeps mixed resource/service outputs consistent with the generated mutation overloads and fixes the Angular sample snapshot regression that appears once lint passes. Refs #3354 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: The Ult <ult_dev@pm.me> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent ee5933c commit b30537d

45 files changed

Lines changed: 4361 additions & 17 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎packages/angular/src/http-client.test.ts‎

Lines changed: 153 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -817,6 +817,159 @@ describe('angular HttpClient generator', () => {
817817
);
818818
});
819819

820+
// Regression test for https://github.com/orval-labs/orval/issues/3349
821+
it('places optional body before accept in overloads for multi-content responses', () => {
822+
const verbOption = createVerbOption({
823+
operationId: 'confirmReservation',
824+
operationName: 'confirmReservation',
825+
verb: 'post',
826+
route: '/reservations/${token}/confirm',
827+
pathRoute: '/reservations/{token}/confirm',
828+
body: {
829+
implementation: 'confirmReservationBody',
830+
definition: 'ConfirmReservationBody',
831+
imports: [],
832+
schemas: [],
833+
originalSchema: {} as never,
834+
contentType: 'application/json',
835+
formData: '',
836+
formUrlEncoded: '',
837+
isOptional: true,
838+
},
839+
props: [
840+
{
841+
name: 'token',
842+
definition: 'token: string',
843+
implementation: 'token: string',
844+
default: false,
845+
required: true,
846+
type: GetterPropType.PARAM,
847+
},
848+
{
849+
name: 'confirmReservationBody',
850+
definition:
851+
'confirmReservationBody?: ConfirmReservationBody | null',
852+
implementation:
853+
'confirmReservationBody?: ConfirmReservationBody | null',
854+
default: false,
855+
required: false,
856+
type: GetterPropType.BODY,
857+
},
858+
],
859+
response: baseResponse({
860+
definition: { success: 'Pet | string', errors: 'Error' },
861+
types: {
862+
success: [
863+
createSuccessType('Pet', 'application/json'),
864+
createSuccessType('string', 'text/plain'),
865+
],
866+
errors: [],
867+
},
868+
contentTypes: ['application/json', 'text/plain'],
869+
}),
870+
});
871+
const options = createGeneratorOptions({
872+
route: '/api/reservations/${token}/confirm',
873+
});
874+
875+
const impl = generateHttpClientImplementation(verbOption, options);
876+
877+
// Body must come before accept in all overload and implementation signatures.
878+
// Check the first occurrence of each — body must appear first.
879+
const bodyIdx = impl.indexOf('confirmReservationBody');
880+
const acceptIdx = impl.indexOf("accept: 'application/json'");
881+
expect(bodyIdx).toBeGreaterThanOrEqual(0);
882+
expect(acceptIdx).toBeGreaterThanOrEqual(0);
883+
expect(bodyIdx).toBeLessThan(acceptIdx);
884+
885+
// TS1016 regression guard: per-content-type overloads must render the
886+
// optional body as a positionally required parameter — the `?` is
887+
// dropped and the type is widened with `| undefined` so a required
888+
// `accept` literal can follow it. The catch-all fallback overload
889+
// legitimately keeps `confirmReservationBody?:` because no required
890+
// parameter follows it there, so we only forbid the optional body
891+
// form when it's directly followed by a required `accept` literal.
892+
expect(impl).toMatch(
893+
/confirmReservationBody:\s*ConfirmReservationBody\s*\|\s*null\s*\|\s*undefined,\s*\n\s*accept:\s*'/,
894+
);
895+
expect(impl).not.toMatch(
896+
/confirmReservationBody\?:[^\n]*\n\s*accept:\s*'/,
897+
);
898+
899+
// The HTTP call itself must still pass the body as the positional argument
900+
expect(impl).toContain(
901+
'this.http.post<Pet>(`/api/reservations/${token}/confirm`, confirmReservationBody, {',
902+
);
903+
});
904+
905+
it('places required body before accept in overloads for multi-content responses', () => {
906+
const verbOption = createVerbOption({
907+
operationId: 'updatePet',
908+
operationName: 'updatePet',
909+
verb: 'put',
910+
route: '/pets/${petId}',
911+
pathRoute: '/pets/{petId}',
912+
body: {
913+
implementation: 'pet',
914+
definition: 'Pet',
915+
imports: [],
916+
schemas: [],
917+
originalSchema: {} as never,
918+
contentType: 'application/json',
919+
formData: '',
920+
formUrlEncoded: '',
921+
isOptional: false,
922+
},
923+
props: [
924+
{
925+
name: 'petId',
926+
definition: 'petId: string',
927+
implementation: 'petId: string',
928+
default: false,
929+
required: true,
930+
type: GetterPropType.PARAM,
931+
},
932+
{
933+
name: 'pet',
934+
definition: 'pet: Pet',
935+
implementation: 'pet: Pet',
936+
default: false,
937+
required: true,
938+
type: GetterPropType.BODY,
939+
},
940+
],
941+
response: baseResponse({
942+
definition: { success: 'Pet | string', errors: 'Error' },
943+
types: {
944+
success: [
945+
createSuccessType('Pet', 'application/json'),
946+
createSuccessType('string', 'text/plain'),
947+
],
948+
errors: [],
949+
},
950+
contentTypes: ['application/json', 'text/plain'],
951+
}),
952+
});
953+
const options = createGeneratorOptions({
954+
route: '/api/pets/${petId}',
955+
});
956+
957+
const impl = generateHttpClientImplementation(verbOption, options);
958+
959+
// Body must come before accept in all overload and implementation signatures.
960+
const bodyIdx = impl.indexOf('pet: Pet');
961+
const acceptIdx = impl.indexOf("accept: 'application/json'");
962+
expect(bodyIdx).toBeGreaterThanOrEqual(0);
963+
expect(acceptIdx).toBeGreaterThanOrEqual(0);
964+
expect(bodyIdx).toBeLessThan(acceptIdx);
965+
966+
// Required body must NOT be widened to `Pet | undefined`.
967+
expect(impl).not.toContain('Pet | undefined');
968+
969+
// The HTTP call must pass the body as the positional argument.
970+
expect(impl).toContain('this.http.put<Pet>(`/api/pets/${petId}`, pet, {');
971+
});
972+
820973
it('preserves query params for multi-content responses', () => {
821974
const verbOption = createVerbOption({
822975
operationId: 'listPets',

‎packages/angular/src/http-client.ts‎

Lines changed: 77 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,8 @@ import {
1818
getDefaultContentType,
1919
getEnumImplementation,
2020
getIsBodyVerb,
21+
type GetterProp,
22+
GetterPropType,
2123
isBoolean,
2224
pascal,
2325
toObjectString,
@@ -66,6 +68,36 @@ const hasSchemaImport = (
6668
const getSchemaValueRef = (typeName: string): string =>
6769
typeName === 'Error' ? 'ErrorSchema' : typeName;
6870

71+
/**
72+
* Partition props into the three buckets used by per-content-type overload
73+
* rendering: required non-body params, body params, and optional non-body
74+
* params. The body always sits between the required and optional non-body
75+
* params so that the per-content-type overloads can insert a required
76+
* `accept` literal immediately after the body without violating TS1016
77+
* (required parameter cannot follow an optional one).
78+
*/
79+
const partitionPropsForMultiContent = (
80+
props: readonly GetterProp[],
81+
): {
82+
requiredNonBody: GetterProp[];
83+
body: GetterProp[];
84+
optionalNonBody: GetterProp[];
85+
} => {
86+
const requiredNonBody: GetterProp[] = [];
87+
const body: GetterProp[] = [];
88+
const optionalNonBody: GetterProp[] = [];
89+
for (const p of props) {
90+
if (p.type === GetterPropType.BODY) {
91+
body.push(p);
92+
} else if (p.required && !p.default) {
93+
requiredNonBody.push(p);
94+
} else {
95+
optionalNonBody.push(p);
96+
}
97+
}
98+
return { requiredNonBody, body, optionalNonBody };
99+
};
100+
69101
const getContentTypeReturnType = (
70102
contentType: string | undefined,
71103
value: string,
@@ -565,22 +597,45 @@ export const generateHttpClientImplementation = (
565597

566598
let contentTypeOverloads = '';
567599
if (hasMultipleContentTypes && isRequestOptions) {
568-
const requiredPart = props
569-
.filter((p) => p.required && !p.default)
600+
const {
601+
requiredNonBody: requiredNonBodyProps,
602+
body: bodyProps,
603+
optionalNonBody: optionalNonBodyProps,
604+
} = partitionPropsForMultiContent(props);
605+
const requiredNonBodyPart = requiredNonBodyProps
570606
.map((p) => p.definition)
571607
.join(',\n ');
572-
const optionalPart = props
573-
.filter((p) => !p.required || p.default)
608+
const bodyPart = bodyProps.map((p) => p.definition).join(',\n ');
609+
// Per-content-type overloads have a required `accept` literal after the body.
610+
// TS1016 forbids required params after optional ones, so optional body params
611+
// are rendered as positionally required (`name: Type | undefined`) here.
612+
// The `?` is removed via an identifier-anchored replacement so we only
613+
// affect the parameter's own optional marker, never a `?:` that may appear
614+
// elsewhere in the type (e.g. mapped or conditional types).
615+
const bodyOverloadPart = bodyProps
616+
.map((p) => {
617+
const optionalMarker = `${p.name}?:`;
618+
if (!p.required && p.definition.startsWith(optionalMarker)) {
619+
const required = `${p.name}:${p.definition.slice(optionalMarker.length)}`;
620+
return /\bundefined\b/.test(required)
621+
? required
622+
: `${required} | undefined`;
623+
}
624+
return p.definition;
625+
})
626+
.join(',\n ');
627+
const optionalNonBodyPart = optionalNonBodyProps
574628
.map((p) => p.definition)
575629
.join(',\n ');
576630
const branchOverloads = successTypes
577631
.filter(({ contentType }) => !!contentType)
578632
.map(({ contentType, value }) => {
579633
const returnType = getGeneratedResponseType(value, contentType);
580634
const overloadParams = [
581-
requiredPart,
635+
requiredNonBodyPart,
636+
bodyOverloadPart,
582637
`accept: '${contentType}'`,
583-
optionalPart,
638+
optionalNonBodyPart,
584639
]
585640
.filter(Boolean)
586641
.join(',\n ');
@@ -589,9 +644,10 @@ export const generateHttpClientImplementation = (
589644
})
590645
.join('\n ');
591646
const allParams = [
592-
requiredPart,
647+
requiredNonBodyPart,
648+
bodyPart,
593649
`accept?: ${acceptTypeName ?? 'string'}`,
594-
optionalPart,
650+
optionalNonBodyPart,
595651
]
596652
.filter(Boolean)
597653
.join(',\n ');
@@ -630,18 +686,25 @@ export const generateHttpClientImplementation = (
630686
? `this.http.${verb}${typeArg}(\`${route}\`, ${bodyIdentifier ?? 'undefined'}, ${optionsObject})`
631687
: `this.http.${verb}${typeArg}(\`${route}\`, ${optionsObject})`;
632688

633-
const requiredPart = props
634-
.filter((p) => p.required && !p.default)
689+
const {
690+
requiredNonBody: requiredNonBodyImplProps,
691+
body: bodyImplProps,
692+
optionalNonBody: optionalNonBodyImplProps,
693+
} = partitionPropsForMultiContent(props);
694+
const requiredNonBodyImplPart = requiredNonBodyImplProps
695+
.map((p) => p.implementation)
696+
.join(',\n ');
697+
const bodyImplPart = bodyImplProps
635698
.map((p) => p.implementation)
636699
.join(',\n ');
637-
const optionalPart = props
638-
.filter((p) => !p.required || p.default)
700+
const optionalNonBodyImplPart = optionalNonBodyImplProps
639701
.map((p) => p.implementation)
640702
.join(',\n ');
641703
const allParams = [
642-
requiredPart,
704+
requiredNonBodyImplPart,
705+
bodyImplPart,
643706
`accept: ${acceptTypeName ?? 'string'} = '${defaultContentType}'`,
644-
optionalPart,
707+
optionalNonBodyImplPart,
645708
]
646709
.filter(Boolean)
647710
.join(',\n ');

‎packages/angular/src/http-resource.test.ts‎

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -646,6 +646,86 @@ describe('angular httpResource generator', () => {
646646
expect(header).toContain('this.http.post');
647647
});
648648

649+
it('emits Accept helpers for multi-content mutations', () => {
650+
const verbOption = createVerbOption({
651+
operationId: 'updatePetById',
652+
operationName: 'updatePetById',
653+
verb: 'put',
654+
route: '/pets/${petId}',
655+
pathRoute: '/pets/{petId}',
656+
body: {
657+
implementation: 'updatePetByIdBody',
658+
definition: 'CreatePetBody',
659+
imports: [],
660+
schemas: [],
661+
originalSchema: {} as never,
662+
contentType: 'application/json',
663+
formData: '',
664+
formUrlEncoded: '',
665+
isOptional: false,
666+
},
667+
props: [
668+
{
669+
name: 'petId',
670+
definition: 'petId: string',
671+
implementation: 'petId: string',
672+
default: false,
673+
required: true,
674+
type: GetterPropType.PARAM,
675+
},
676+
{
677+
name: 'updatePetByIdBody',
678+
definition: 'updatePetByIdBody: CreatePetBody',
679+
implementation: 'updatePetByIdBody: CreatePetBody',
680+
default: false,
681+
required: true,
682+
type: GetterPropType.BODY,
683+
},
684+
],
685+
params: [
686+
{
687+
name: 'petId',
688+
definition: 'petId: string',
689+
implementation: 'petId: string',
690+
default: false,
691+
required: true,
692+
imports: [],
693+
},
694+
],
695+
response: baseResponse({
696+
definition: { success: 'Pet | string', errors: 'Error' },
697+
types: {
698+
success: [
699+
createSuccessType('Pet', 'application/json'),
700+
createSuccessType('string', 'text/plain'),
701+
],
702+
errors: [],
703+
},
704+
contentTypes: ['application/json', 'text/plain'],
705+
}),
706+
});
707+
routeRegistry.set('updatePetById', '/api/pets/${petId}');
708+
709+
const header = generateHttpResourceHeader({
710+
title: 'PetService',
711+
isRequestOptions: true,
712+
isMutator: false,
713+
isGlobalMutator: false,
714+
provideIn: 'root',
715+
hasAwaitedType: false,
716+
output: createOutput(),
717+
verbOptions: { updatePetById: verbOption },
718+
clientImplementation: '',
719+
} as never);
720+
721+
expect(header).toContain('export type UpdatePetByIdAccept');
722+
expect(header).toContain("accept: 'application/json'");
723+
expect(header).toContain("accept: 'text/plain'");
724+
expect(header).toContain(
725+
"accept: UpdatePetByIdAccept = 'application/json'",
726+
);
727+
});
728+
649729
it('generates both resources and service class for mixed GET + mutation', () => {
650730
const getVerb = createVerbOption();
651731
const postVerb = createVerbOption({

0 commit comments

Comments
 (0)