From a6365a1770d07b763f5ae9805374cda11085e180 Mon Sep 17 00:00:00 2001 From: Yuchao Yan Date: Wed, 11 Sep 2024 13:40:26 +0800 Subject: [PATCH 1/8] case to reproduce --- .../test/types/multipart-types.test.ts | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/packages/typespec-client-generator-core/test/types/multipart-types.test.ts b/packages/typespec-client-generator-core/test/types/multipart-types.test.ts index e85ab84033..ff7686aaac 100644 --- a/packages/typespec-client-generator-core/test/types/multipart-types.test.ts +++ b/packages/typespec-client-generator-core/test/types/multipart-types.test.ts @@ -61,6 +61,22 @@ describe("typespec-client-generator-core: multipart types", () => { code: "@azure-tools/typespec-client-generator-core/conflicting-multipart-model-usage", }); }); + it("multipart conflicting model usage for HttpPart", async function () { + await runner.compile( + ` + @service({title: "Test Service"}) namespace TestService; + + model MultiPartRequest { + id: string; + profileImage: bytes; + } + + @post op basic1(@header contentType: "multipart/form-data", @body body: MultiPartRequest): NoContentResponse; + @put op basic2(@header contentType: "multipart/form-data", @body body: MultiPartRequest): NoContentResponse; + ` + ); + deepEqual(runner.context.diagnostics.length, 0); + }); it("multipart resolving conflicting model usage with spread", async function () { await runner.compileWithBuiltInService( ` From d3b434f37515bcefde90f6d67f44569735261a45 Mon Sep 17 00:00:00 2001 From: Yuchao Yan Date: Fri, 13 Sep 2024 13:26:42 +0800 Subject: [PATCH 2/8] fix logic to check conflicting usage between multipart request and regular request --- .../src/types.ts | 41 ++++++++++--------- 1 file changed, 21 insertions(+), 20 deletions(-) diff --git a/packages/typespec-client-generator-core/src/types.ts b/packages/typespec-client-generator-core/src/types.ts index 5d4c997561..092e87121c 100644 --- a/packages/typespec-client-generator-core/src/types.ts +++ b/packages/typespec-client-generator-core/src/types.ts @@ -1630,26 +1630,6 @@ function updateTypesFromOperation( } const multipartOperation = isMultipartOperation(context, operation); - // this part should be put before setting current body's usage because it is based on the previous usage - if ( - sdkType.kind === "model" && - ((!multipartOperation && (sdkType.usage & UsageFlags.MultipartFormData) > 0) || - (multipartOperation && - (sdkType.usage & UsageFlags.Input) > 0 && - (sdkType.usage & UsageFlags.Input & UsageFlags.MultipartFormData) === 0)) - ) { - // This means we have a model that is used both for formdata input and for regular body input - diagnostics.add( - createDiagnostic({ - code: "conflicting-multipart-model-usage", - target: httpBody.type, - format: { - modelName: sdkType.name, - }, - }) - ); - } - if (generateConvenient) { if (spread) { updateUsageOrAccessOfModel(context, UsageFlags.Spread, sdkType, { propagation: false }); @@ -1677,6 +1657,27 @@ function updateTypesFromOperation( } const access = getAccessOverride(context, operation) ?? "public"; diagnostics.pipe(updateUsageOrAccessOfModel(context, access, sdkType)); + + // after complete of usage calculation for httpBody, check whether it has + // conflicting usage between multipart and regular body + if ( + sdkType.kind === "model" && + ((!multipartOperation && (sdkType.usage & UsageFlags.MultipartFormData) > 0) || + (multipartOperation && + (sdkType.usage & UsageFlags.MultipartFormData) > 0 && + ((sdkType.usage & UsageFlags.Json | sdkType.usage & UsageFlags.Xml) > 0))) + ) { + // This means we have a model that is used both for formdata input and for regular body input + diagnostics.add( + createDiagnostic({ + code: "conflicting-multipart-model-usage", + target: httpBody.type, + format: { + modelName: sdkType.name, + }, + }) + ); + } } for (const response of httpOperation.responses) { for (const innerResponse of response.responses) { From 1c723d4424e53a844773f46ca59389dc8686887f Mon Sep 17 00:00:00 2001 From: Yuchao Yan Date: Fri, 13 Sep 2024 13:30:16 +0800 Subject: [PATCH 3/8] format --- packages/typespec-client-generator-core/src/types.ts | 4 ++-- .../test/types/multipart-types.test.ts | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/typespec-client-generator-core/src/types.ts b/packages/typespec-client-generator-core/src/types.ts index 092e87121c..1c7f166180 100644 --- a/packages/typespec-client-generator-core/src/types.ts +++ b/packages/typespec-client-generator-core/src/types.ts @@ -1658,14 +1658,14 @@ function updateTypesFromOperation( const access = getAccessOverride(context, operation) ?? "public"; diagnostics.pipe(updateUsageOrAccessOfModel(context, access, sdkType)); - // after complete of usage calculation for httpBody, check whether it has + // after complete of usage calculation for httpBody, check whether it has // conflicting usage between multipart and regular body if ( sdkType.kind === "model" && ((!multipartOperation && (sdkType.usage & UsageFlags.MultipartFormData) > 0) || (multipartOperation && (sdkType.usage & UsageFlags.MultipartFormData) > 0 && - ((sdkType.usage & UsageFlags.Json | sdkType.usage & UsageFlags.Xml) > 0))) + ((sdkType.usage & UsageFlags.Json) | (sdkType.usage & UsageFlags.Xml)) > 0)) ) { // This means we have a model that is used both for formdata input and for regular body input diagnostics.add( diff --git a/packages/typespec-client-generator-core/test/types/multipart-types.test.ts b/packages/typespec-client-generator-core/test/types/multipart-types.test.ts index ff7686aaac..5e739abc37 100644 --- a/packages/typespec-client-generator-core/test/types/multipart-types.test.ts +++ b/packages/typespec-client-generator-core/test/types/multipart-types.test.ts @@ -61,7 +61,7 @@ describe("typespec-client-generator-core: multipart types", () => { code: "@azure-tools/typespec-client-generator-core/conflicting-multipart-model-usage", }); }); - it("multipart conflicting model usage for HttpPart", async function () { + it("multipart conflicting model usage for only multipart operations", async function () { await runner.compile( ` @service({title: "Test Service"}) namespace TestService; From 44cf17b493b2ee435579ea32e9fb2a82ed3644ad Mon Sep 17 00:00:00 2001 From: Yuchao Yan Date: Fri, 13 Sep 2024 13:31:36 +0800 Subject: [PATCH 4/8] changelog --- .../fix-multipart-diagnostics-2024-8-13-13-31-25.md | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 .chronus/changes/fix-multipart-diagnostics-2024-8-13-13-31-25.md diff --git a/.chronus/changes/fix-multipart-diagnostics-2024-8-13-13-31-25.md b/.chronus/changes/fix-multipart-diagnostics-2024-8-13-13-31-25.md new file mode 100644 index 0000000000..555966ea96 --- /dev/null +++ b/.chronus/changes/fix-multipart-diagnostics-2024-8-13-13-31-25.md @@ -0,0 +1,7 @@ +--- +changeKind: fix +packages: + - "@azure-tools/typespec-client-generator-core" +--- + +Fix logic to check conflicting usage for model of multipart body and regular body \ No newline at end of file From 2af62d53e927e404805b47d86d3165726940c056 Mon Sep 17 00:00:00 2001 From: Yuchao Yan Date: Fri, 13 Sep 2024 13:37:32 +0800 Subject: [PATCH 5/8] update comment --- packages/typespec-client-generator-core/src/types.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/typespec-client-generator-core/src/types.ts b/packages/typespec-client-generator-core/src/types.ts index 1c7f166180..93cc0a049a 100644 --- a/packages/typespec-client-generator-core/src/types.ts +++ b/packages/typespec-client-generator-core/src/types.ts @@ -1658,7 +1658,7 @@ function updateTypesFromOperation( const access = getAccessOverride(context, operation) ?? "public"; diagnostics.pipe(updateUsageOrAccessOfModel(context, access, sdkType)); - // after complete of usage calculation for httpBody, check whether it has + // after completion of usage calculation for httpBody, check whether it has // conflicting usage between multipart and regular body if ( sdkType.kind === "model" && From 5ac2907c063774304f5261986f80d6f9d6b8447a Mon Sep 17 00:00:00 2001 From: Yuchao Yan Date: Fri, 13 Sep 2024 14:29:30 +0800 Subject: [PATCH 6/8] update test --- .../test/types/multipart-types.test.ts | 52 +++++++++++++++++-- 1 file changed, 49 insertions(+), 3 deletions(-) diff --git a/packages/typespec-client-generator-core/test/types/multipart-types.test.ts b/packages/typespec-client-generator-core/test/types/multipart-types.test.ts index 5e739abc37..8bb36ab392 100644 --- a/packages/typespec-client-generator-core/test/types/multipart-types.test.ts +++ b/packages/typespec-client-generator-core/test/types/multipart-types.test.ts @@ -65,18 +65,64 @@ describe("typespec-client-generator-core: multipart types", () => { await runner.compile( ` @service({title: "Test Service"}) namespace TestService; - + model Address {city: string;} + model MultiPartRequest { + address: Address; + id: string; + profileImage: bytes; + } + + @post + @route("/basic1") + op basic1(@header contentType: "multipart/form-data", @body body: MultiPartRequest): NoContentResponse; + @post + @route("/basic2") + op basic2(@header contentType: "multipart/form-data", @body body: MultiPartRequest): NoContentResponse; + ` + ); + deepEqual(runner.context.diagnostics.length, 0); + const address = runner.context.sdkPackage.models.find((x) => x.name === "Address"); + ok(address); + deepEqual(address.usage, UsageFlags.Input); + const multiPartRequest = runner.context.sdkPackage.models.find( + (x) => x.name === "MultiPartRequest" + ); + ok(multiPartRequest); + deepEqual(multiPartRequest.usage, UsageFlags.MultipartFormData | UsageFlags.Input); + }); + it("multipart conflicting model usage for mixed operations", async function () { + await runner.compile( + ` + @service({title: "Test Service"}) namespace TestService; + model Address {city: string;} + model RegularRequest { + address: Address; + } model MultiPartRequest { + address: Address; id: string; profileImage: bytes; } - @post op basic1(@header contentType: "multipart/form-data", @body body: MultiPartRequest): NoContentResponse; - @put op basic2(@header contentType: "multipart/form-data", @body body: MultiPartRequest): NoContentResponse; + @post + @route("/basic1") + op basic1(@body body: RegularRequest): NoContentResponse; + @post + @route("/basic2") + op basic2(@header contentType: "multipart/form-data", @body body: MultiPartRequest): NoContentResponse; ` ); deepEqual(runner.context.diagnostics.length, 0); + const address = runner.context.sdkPackage.models.find((x) => x.name === "Address"); + ok(address); + deepEqual(address.usage, UsageFlags.Input | UsageFlags.Json); + const multiPartRequest = runner.context.sdkPackage.models.find( + (x) => x.name === "MultiPartRequest" + ); + ok(multiPartRequest); + deepEqual(multiPartRequest.usage, UsageFlags.MultipartFormData | UsageFlags.Input); }); + it("multipart resolving conflicting model usage with spread", async function () { await runner.compileWithBuiltInService( ` From dbb5c292465c010384fe15772298d0b98c099276 Mon Sep 17 00:00:00 2001 From: tadelesh Date: Thu, 19 Sep 2024 16:02:34 +0800 Subject: [PATCH 7/8] refine logic --- .../src/types.ts | 37 +++++++++---------- .../test/types/multipart-types.test.ts | 3 ++ 2 files changed, 21 insertions(+), 19 deletions(-) diff --git a/packages/typespec-client-generator-core/src/types.ts b/packages/typespec-client-generator-core/src/types.ts index 0b2b7b1499..2253551e26 100644 --- a/packages/typespec-client-generator-core/src/types.ts +++ b/packages/typespec-client-generator-core/src/types.ts @@ -1331,8 +1331,8 @@ function updateMultiPartInfo( : undefined, contentType: httpOperationPart.body.contentTypeProperty ? diagnostics.pipe( - getSdkModelPropertyType(context, httpOperationPart.body.contentTypeProperty, operation), - ) + getSdkModelPropertyType(context, httpOperationPart.body.contentTypeProperty, operation), + ) : undefined, defaultContentTypes: httpOperationPart.body.contentTypes, }; @@ -1660,25 +1660,24 @@ function updateTypesFromOperation( // after completion of usage calculation for httpBody, check whether it has // conflicting usage between multipart and regular body - if ( - sdkType.kind === "model" && - ((!multipartOperation && (sdkType.usage & UsageFlags.MultipartFormData) > 0) || - (multipartOperation && - (sdkType.usage & UsageFlags.MultipartFormData) > 0 && - ((sdkType.usage & UsageFlags.Json) | (sdkType.usage & UsageFlags.Xml)) > 0)) - ) { - // This means we have a model that is used both for formdata input and for regular body input - diagnostics.add( - createDiagnostic({ - code: "conflicting-multipart-model-usage", - target: httpBody.type, - format: { - modelName: sdkType.name, - }, - }) - ); + if (sdkType.kind === "model") { + const isUsedInMultipart = (sdkType.usage & UsageFlags.MultipartFormData) > 0; + const isUsedInOthers = ((sdkType.usage & UsageFlags.Json) | (sdkType.usage & UsageFlags.Xml)) > 0; + if (!multipartOperation && isUsedInMultipart || multipartOperation && isUsedInOthers) { + // This means we have a model that is used both for formdata input and for regular body input + diagnostics.add( + createDiagnostic({ + code: "conflicting-multipart-model-usage", + target: httpBody.type, + format: { + modelName: sdkType.name, + }, + }) + ); + } } } + for (const response of httpOperation.responses) { for (const innerResponse of response.responses) { if (innerResponse.body?.type && !isNeverOrVoidType(innerResponse.body.type)) { diff --git a/packages/typespec-client-generator-core/test/types/multipart-types.test.ts b/packages/typespec-client-generator-core/test/types/multipart-types.test.ts index 0add033134..a224bd05e9 100644 --- a/packages/typespec-client-generator-core/test/types/multipart-types.test.ts +++ b/packages/typespec-client-generator-core/test/types/multipart-types.test.ts @@ -44,6 +44,7 @@ describe("typespec-client-generator-core: multipart types", () => { ok(profileImage.multipartOptions); strictEqual(profileImage.multipartOptions.isFilePart, true); }); + it("multipart conflicting model usage", async function () { await runner.compile( ` @@ -61,6 +62,7 @@ describe("typespec-client-generator-core: multipart types", () => { code: "@azure-tools/typespec-client-generator-core/conflicting-multipart-model-usage", }); }); + it("multipart conflicting model usage for only multipart operations", async function () { await runner.compile( ` @@ -90,6 +92,7 @@ describe("typespec-client-generator-core: multipart types", () => { ok(multiPartRequest); deepEqual(multiPartRequest.usage, UsageFlags.MultipartFormData | UsageFlags.Input); }); + it("multipart conflicting model usage for mixed operations", async function () { await runner.compile( ` From 8ed87e3190cd3331524e3e77dca0947112532cbf Mon Sep 17 00:00:00 2001 From: tadelesh Date: Thu, 19 Sep 2024 16:02:41 +0800 Subject: [PATCH 8/8] format --- packages/typespec-client-generator-core/src/types.ts | 11 ++++++----- .../test/types/multipart-types.test.ts | 10 +++++----- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/packages/typespec-client-generator-core/src/types.ts b/packages/typespec-client-generator-core/src/types.ts index 2253551e26..7626bede4c 100644 --- a/packages/typespec-client-generator-core/src/types.ts +++ b/packages/typespec-client-generator-core/src/types.ts @@ -1331,8 +1331,8 @@ function updateMultiPartInfo( : undefined, contentType: httpOperationPart.body.contentTypeProperty ? diagnostics.pipe( - getSdkModelPropertyType(context, httpOperationPart.body.contentTypeProperty, operation), - ) + getSdkModelPropertyType(context, httpOperationPart.body.contentTypeProperty, operation), + ) : undefined, defaultContentTypes: httpOperationPart.body.contentTypes, }; @@ -1662,8 +1662,9 @@ function updateTypesFromOperation( // conflicting usage between multipart and regular body if (sdkType.kind === "model") { const isUsedInMultipart = (sdkType.usage & UsageFlags.MultipartFormData) > 0; - const isUsedInOthers = ((sdkType.usage & UsageFlags.Json) | (sdkType.usage & UsageFlags.Xml)) > 0; - if (!multipartOperation && isUsedInMultipart || multipartOperation && isUsedInOthers) { + const isUsedInOthers = + ((sdkType.usage & UsageFlags.Json) | (sdkType.usage & UsageFlags.Xml)) > 0; + if ((!multipartOperation && isUsedInMultipart) || (multipartOperation && isUsedInOthers)) { // This means we have a model that is used both for formdata input and for regular body input diagnostics.add( createDiagnostic({ @@ -1672,7 +1673,7 @@ function updateTypesFromOperation( format: { modelName: sdkType.name, }, - }) + }), ); } } diff --git a/packages/typespec-client-generator-core/test/types/multipart-types.test.ts b/packages/typespec-client-generator-core/test/types/multipart-types.test.ts index a224bd05e9..8211de4956 100644 --- a/packages/typespec-client-generator-core/test/types/multipart-types.test.ts +++ b/packages/typespec-client-generator-core/test/types/multipart-types.test.ts @@ -44,7 +44,7 @@ describe("typespec-client-generator-core: multipart types", () => { ok(profileImage.multipartOptions); strictEqual(profileImage.multipartOptions.isFilePart, true); }); - + it("multipart conflicting model usage", async function () { await runner.compile( ` @@ -80,14 +80,14 @@ describe("typespec-client-generator-core: multipart types", () => { @post @route("/basic2") op basic2(@header contentType: "multipart/form-data", @body body: MultiPartRequest): NoContentResponse; - ` + `, ); deepEqual(runner.context.diagnostics.length, 0); const address = runner.context.sdkPackage.models.find((x) => x.name === "Address"); ok(address); deepEqual(address.usage, UsageFlags.Input); const multiPartRequest = runner.context.sdkPackage.models.find( - (x) => x.name === "MultiPartRequest" + (x) => x.name === "MultiPartRequest", ); ok(multiPartRequest); deepEqual(multiPartRequest.usage, UsageFlags.MultipartFormData | UsageFlags.Input); @@ -113,14 +113,14 @@ describe("typespec-client-generator-core: multipart types", () => { @post @route("/basic2") op basic2(@header contentType: "multipart/form-data", @body body: MultiPartRequest): NoContentResponse; - ` + `, ); deepEqual(runner.context.diagnostics.length, 0); const address = runner.context.sdkPackage.models.find((x) => x.name === "Address"); ok(address); deepEqual(address.usage, UsageFlags.Input | UsageFlags.Json); const multiPartRequest = runner.context.sdkPackage.models.find( - (x) => x.name === "MultiPartRequest" + (x) => x.name === "MultiPartRequest", ); ok(multiPartRequest); deepEqual(multiPartRequest.usage, UsageFlags.MultipartFormData | UsageFlags.Input);