Generate types for new MFD design #2455

timovv · 2024-04-18T18:20:45Z

See #2447 for context. This version generates without a type helper. Hopefully this will improve understanding.

The scope of this first round of work is to create the API surface correctly. In follow up PRs, we should look at

Generating good docs which highlight how to construct the body and show which parts are optional, required, etc
Generating samples

These follow up items will improve the user experience.

MaryGao · 2024-04-23T06:55:01Z

packages/typespec-ts/test/integration/generated/payload/multipart/src/index.ts

-  createFileFromStream,
-  type CreateFileOptions,
-  type CreateFileFromStreamOptions,
-} from "@azure/core-rest-pipeline";



is it because of filename and content type attributes so we could remove these helpers?

Yeah, that, and the fact that we now accept streams as part bodies, so no need for createFileFromStream.

MaryGao · 2024-04-23T07:02:02Z

Thanks for making this! The approach looks good to me :)~

MaryGao · 2024-04-23T07:04:38Z

packages/typespec-ts/test/integration/multipart.spec.ts

-      const file = createFileFromStream(() => content, "hello.jpg", {
-        type: "image/jpg"
-      });
+    it("allows specifying MIME type and filename", async () => {


just curious without the helper createFileFromStream how to feed the big file in a streaming way?

You can just pass the stream directly:

const result = await client .path("/multipart/form-data/check-filename-and-content-type") .post({ contentType: "multipart/form-data", body: [ { name: "profileImage", body: fs.createReadStream("/path/to/file"), filename, contentType } ] });

…l-ranch

This reverts commit adaa2bc.

MaryGao · 2024-04-26T02:51:34Z

packages/rlc-common/src/metadata/packageJson/azurePackageCommon.ts

-    "@azure-rest/core-client": specSource === "Swagger" ? "^1.4.0" : "^2.0.0",
+    // Swagger and modular libraries currently only support the old multipart/form-data implementation.
+    "@azure-rest/core-client":
+      specSource === "Swagger" || isModularLibrary ? "^1.4.0" : "^2.0.0",


why modular is depend on v1?

I was wondering what is our plan for v2 adoption?

HLC -> v2?

Modular -> v2

RLC from swagger -> v1 or v2? // my preference is v2 if the upgrading effort is not too much

RLC from typespec -> v2

For modular to depend on v2, we need to update the modular API models as well, and probably also figure out how we want to handle serialization, which is a whole other piece of work which is I think out of scope for this PR. So I kept modular on the old version for now. Obviously we aren't going to keep it like that.

For modular to depend on v2, we need to update the modular API models as well, and probably also figure out how we want to handle serialization

Personally I prefer to always use the latest version in modular. We didn't support mfd feature in core rest v1 either, it's fine considering there is no real need to modular mfd yet except openai(but i heard that we plan to depreate azure openai ), another reason is modular is still in early stage. The version mismatch may introduce un-necessary maintainance effort.

We could either generate as regular models or throw errors for modular mfd.

@joheredi How do you think?

I think it depends, if making modular take a dependency on v2 means there is additional effort in this PR I would favor keeping it in v1. If it is just a matter fo switching the string, go for it. However I think it is most likely the former, if so I'm fine keeping the 2 versions in the meantime.

ah, I think I see what you mean. I was under the impression that MFD was somewhat working in modular v1 given OpenAI is using it. But that is because they are doing customization, right? In that case I will change this to use 2.0, since there won't be a regression (it was already broken).

At the moment I have modular just generating normal models for MFD so as not to break the compilation.

yes, openai has customization here.

joheredi

Looks good! Great improvement :)

This reverts commit f0d9cc1.

qiaozha

LGTM, just curious what would happen if a customer passes inconsistent content type and body in multipart form data operation ? how would the core serialize the body and send the request?

MaryGao · 2024-04-26T08:53:57Z

packages/typespec-test/test/faceai/generated/typespec-ts/src/models.ts

-   */
-  VerifyImage:
+export interface LivenessSessionWithVerifyImageCreationContentParametersPartDescriptor {
+  name: "Parameters";


Glad to see this happened.

MaryGao

LGTM

MaryGao · 2024-04-26T09:08:20Z

packages/rlc-common/src/buildObjectTypes.ts

@@ -40,6 +48,19 @@ export function buildObjectInterfaces(
    ) {
      continue;
    }
+
+    // FIXME: disabling new multipart generation for modular while we figure out the story
+    if (objectSchema.isMultipartBody && !model.options?.isModularLibrary) {


nit: what RLC version will be generated in modular SDK? I image that new design would align with v2, old design would align with v1.

We just generate a normal RLC model in the modular SDK (as if it was a plain old JSON operation) because of this check. This shape is more in line with v1 of the Core package. We can't generate the v2 models here yet as that results in a compile error, since the serializers don't serialize into the array shape.

MaryGao · 2024-04-26T09:18:36Z

packages/rlc-common/src/buildObjectTypes.ts

+    }
+  ];
+
+function buildMultipartPartDefinitions(


Do we consider the case for A extends B?

Not currently but we can revisit if this becomes a requirement

timovv · 2024-04-26T17:29:23Z

LGTM, just curious what would happen if a customer passes inconsistent content type and body in multipart form data operation ? how would the core serialize the body and send the request?

If you set the content type to multipart/form-data and pass something unexpected it currently just JSON.stringifies it, which is what we were also doing in the old MFD implementation. I opened an issue for Core tracking this earlier: Azure/azure-sdk-for-js#29424

timovv added 3 commits April 18, 2024 11:15

Multipart - generate type directly

9c5be60

Rename type to contentType to match Core implementation

51d1ed0

e2e tests

fe180d6

timovv mentioned this pull request Apr 19, 2024

[WIP] New design for multipart (type helper implementation) #2447

Closed

2 tasks

Generate interfaces for parts in the array

d7bbb59

MaryGao reviewed Apr 23, 2024

View reviewed changes

fixes

804de8b

timovv changed the title ~~[WIP] New design for multipart - no type helper~~ Generate types for new MFD design Apr 24, 2024

timovv added 4 commits April 25, 2024 13:42

Generate JSON array instead of multiple parts for JSON to satisfy cad…

6bba312

…l-ranch

Merge branch 'main' into multipart-direct-generation

42f1389

@azure-rest/core-client@2.0.0

f6f9930

Integration tests to 2.0.0

9c1a89d

timovv marked this pull request as ready for review April 26, 2024 00:17

timovv requested review from sarangan12, joheredi and qiaozha as code owners April 26, 2024 00:17

timovv added 8 commits April 25, 2024 17:30

Fix rlc-common unit tests

e58f5d3

Use older core-clietn for swagger gen

f7990c7

rush update

23dda1d

Update autorest.typescript core client version

adaa2bc

Revert "Update autorest.typescript core client version"

6b5d840

This reverts commit adaa2bc.

Use old core-client for Swagger

0f7ccb5

Disable new MFD for modular

7673232

Make modular depend on old core-client

f0d9cc1

MaryGao reviewed Apr 26, 2024

View reviewed changes

Fix bug

514ffbe

joheredi approved these changes Apr 26, 2024

View reviewed changes

Revert "Make modular depend on old core-client"

7ee574b

This reverts commit f0d9cc1.

Update snapshots

8f9fefb

qiaozha approved these changes Apr 26, 2024

View reviewed changes

kazrael2119 added 2 commits April 26, 2024 14:32

update faceai smoke test

3484daf

rerun smoke-test

efcae26

MaryGao reviewed Apr 26, 2024

View reviewed changes

MaryGao approved these changes Apr 26, 2024

View reviewed changes

MaryGao reviewed Apr 26, 2024

View reviewed changes

timovv merged commit 8bcf247 into Azure:main Apr 26, 2024
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate types for new MFD design #2455

Generate types for new MFD design #2455

timovv commented Apr 18, 2024

MaryGao Apr 23, 2024

timovv Apr 23, 2024

MaryGao commented Apr 23, 2024 •

edited

MaryGao Apr 23, 2024

timovv Apr 23, 2024

MaryGao Apr 26, 2024 •

edited

timovv Apr 26, 2024

MaryGao Apr 26, 2024 •

edited

joheredi Apr 26, 2024

timovv Apr 26, 2024

MaryGao Apr 26, 2024

joheredi left a comment

qiaozha left a comment

MaryGao Apr 26, 2024

MaryGao left a comment

MaryGao Apr 26, 2024

timovv Apr 26, 2024 •

edited

MaryGao Apr 26, 2024

timovv Apr 26, 2024

timovv commented Apr 26, 2024

Generate types for new MFD design #2455

Generate types for new MFD design #2455

Conversation

timovv commented Apr 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaryGao commented Apr 23, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaryGao Apr 26, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaryGao Apr 26, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joheredi left a comment

Choose a reason for hiding this comment

qiaozha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaryGao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timovv Apr 26, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timovv commented Apr 26, 2024

MaryGao commented Apr 23, 2024 •

edited

MaryGao Apr 26, 2024 •

edited

MaryGao Apr 26, 2024 •

edited

timovv Apr 26, 2024 •

edited