Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add multipartfile model in Azure.Core #1458

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
changeKind: feature
packages:
- "@azure-tools/typespec-azure-core"
---

Add model MultiPartFile with required `filename` and `contentType`
15 changes: 15 additions & 0 deletions docs/libraries/azure-core/reference/data-types.md
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,21 @@ model Azure.Core.MaxPageSizeQueryParameter
| ------------ | ------- | -------------------------------------------- |
| maxpagesize? | `int32` | The maximum number of result items per page. |

### `MultiPartFile` {#Azure.Core.MultiPartFile}

Used in file part of multipart request body

```typespec
model Azure.Core.MultiPartFile
```

#### Properties

| Name | Type | Description |
| ----------- | -------- | ------------------------------------------------------- |
| filename | `string` | The file name in file part of multipart request body |
| contentType | `string` | The content type in file part of multipart request body |

### `OrderByQueryParameter` {#Azure.Core.OrderByQueryParameter}

Provides the standard 'orderby' query parameter for list operations.
Expand Down
1 change: 1 addition & 0 deletions docs/libraries/azure-core/reference/index.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ npm install --save-peer @azure-tools/typespec-azure-core
- [`FilterParameter`](./data-types.md#Azure.Core.FilterParameter)
- [`FilterQueryParameter`](./data-types.md#Azure.Core.FilterQueryParameter)
- [`MaxPageSizeQueryParameter`](./data-types.md#Azure.Core.MaxPageSizeQueryParameter)
- [`MultiPartFile`](./data-types.md#Azure.Core.MultiPartFile)
- [`OrderByQueryParameter`](./data-types.md#Azure.Core.OrderByQueryParameter)
- [`Page`](./data-types.md#Azure.Core.Page)
- [`PollingOptions`](./data-types.md#Azure.Core.PollingOptions)
Expand Down
9 changes: 9 additions & 0 deletions packages/typespec-azure-core/lib/models.tsp
Original file line number Diff line number Diff line change
Expand Up @@ -400,3 +400,12 @@ model ArmResourceIdentifierAllowedResource {
*/
scopes?: ArmResourceDeploymentScope[];
}

@doc("Used in file part of multipart request body")
model MultiPartFile extends File {
msyyc marked this conversation as resolved.
Show resolved Hide resolved
@doc("The file name in file part of multipart request body")
filename: string;

@doc("The content type in file part of multipart request body")
msyyc marked this conversation as resolved.
Show resolved Hide resolved
contentType: string;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

context: #1380 (comment)

Copy link
Member

@weidongxu-microsoft weidongxu-microsoft Sep 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why contentType is required? Are user required to fill-in the contentType; or it just mean client required to send the contentType, but value may not be filled by user?

Should we have e.g. contentType: "application/octet-stream", if this must be a required property?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have e.g. contentType: "application/octet-stream", if this must be a required property?
-> It is OK for Python since the default content-type in RFC is also application/octect-stream. @chunyu3 / @MaryGao / @qiaozha what's your opinion?

This comment was marked as outdated.

Copy link
Member

@weidongxu-microsoft weidongxu-microsoft Sep 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Java would typically want

this contentType must to be sent on wire, but if user does not set anything, SDK would use "application/octet-stream" for File

I assume this cannot be expressed in TypeSpec without client default value.

But the point here, is that I didn't see why the contentType property be different from the definition in TypeSpec.Http.File. If contentType be required here, it should be required in TypeSpec.Http.File too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So Java hopes the core model only change filename to required.

Copy link
Contributor

@MaryGao MaryGao Sep 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Share my two cents here.

Does the required/optional stand for the client code's optionality or payload's absence?
I would expect typespec definition here stands for the client code's optionality, which means if optional user don't need to fill-in that value. That is aligned with TypeSpec principle to represent some logic stuff of API. And we could mitigate the gaps according to RFC if contentType is absent. We do similar things in normal request if content type is missing in core(at least JS).

Do we prefer the contentType here is optional or required?
I prefer optional, we have seen serveral mfd RPs and I didn't realize one which require contentType as required parameter yet, so I would expect optional contentType would be more common cases. Also contentType as optional would be more aligned with browser behavior in JS(see blob api).

One thing I am not sure is if service team wants this to be required, what is their best practice to define tsp? can they leverage our FileWithRequiredMetadata? and is this wired we name this as WithRequiredMetadata but actually only WithRequiredFilename?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this model is explicitly for a file that has more restrictions on it than a normal File. It extends from File but overrides the properties to be required instead of optional

Copy link
Member Author

@msyyc msyyc Sep 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With more flexibility for customers, We could add 2 models for customer:

  • FileWithRequiredMetadata: both filename and contentType are required (already in this PR)
  • FileWithRequiredFileName: only filename is required

Then azure service team could choose any one which meets their requirements.

Copy link
Member

@weidongxu-microsoft weidongxu-microsoft Sep 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd assume Java azure-core would only add one class.

loop @srnagar for opinion as well.

So far, I think I've seen 1 service require user input the contentType.
https://github.com/Azure/azure-rest-api-specs/blob/main/specification/translation/Azure.AI.DocumentTranslation/models.tsp#L11-L18
(this is before TypeSpec.Http.File, service asks SDK to allow set contentType)

}
30 changes: 30 additions & 0 deletions packages/typespec-azure-core/test/models.test.ts
msyyc marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
import { expectDiagnosticEmpty } from "@typespec/compiler/testing";
import { getHttpPart, isOrExtendsHttpFile } from "@typespec/http";
import { ok, strictEqual } from "assert";
import { describe, it } from "vitest";
import { getOperations } from "./test-host.js";

describe("typespec-azure-core: models", () => {
msyyc marked this conversation as resolved.
Show resolved Hide resolved
it("MultiPartFile could be recognized by @typespec/http", async () => {
const [operations, diagnostics, runner] = await getOperations(
`
model TestModel {
file: HttpPart<MultiPartFile>;
};

@post op TestOperation(@header contentType: "multipart/form-date", @multipartBody body: TestModel): void;
`
);

ok(operations.length === 1);
ok(operations[0].parameters.body);
strictEqual(operations[0].parameters.body.bodyKind, "multipart");
strictEqual(operations[0].parameters.body.type.kind, "Model");
const fileHttpPart = operations[0].parameters.body.type.properties.get("file");
ok(fileHttpPart);
const file = getHttpPart(runner.program, fileHttpPart.type);
ok(file !== undefined);
ok(isOrExtendsHttpFile(runner.program, file.type));
expectDiagnosticEmpty(diagnostics);
msyyc marked this conversation as resolved.
Show resolved Hide resolved
});
});
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { AzureCoreTestLibrary } from "@azure-tools/typespec-azure-core/testing";
import { expectDiagnostics } from "@typespec/compiler/testing";
import { deepEqual, ok, strictEqual } from "assert";
import { beforeEach, describe, it } from "vitest";
Expand Down Expand Up @@ -454,6 +455,52 @@ describe("typespec-client-generator-core: multipart types", () => {
strictEqual(fileRequiredFileName.multipartOptions.contentType.optional, false);
});

it("with MultiPartFile of Azure.Core", async function () {
msyyc marked this conversation as resolved.
Show resolved Hide resolved
const runnerCore = await createSdkTestRunner({
librariesToAdd: [AzureCoreTestLibrary],
emitterName: "@azure-tools/typespec-java",
autoUsings: ["Azure.Core"],
});
await runnerCore.compileWithBuiltInService(`
model MultiPartRequest{
fileOptionalFileName: HttpPart<File>;
fileRequiredFileName: HttpPart<MultiPartFile>;
}
@post
op upload(@header contentType: "multipart/form-data", @multipartBody body: MultiPartRequest): void;
`);
const models = runnerCore.context.sdkPackage.models;
strictEqual(models.length, 2);
const MultiPartRequest = models.find((x) => x.name === "MultiPartRequest");
ok(MultiPartRequest);
ok(MultiPartRequest.usage & UsageFlags.MultipartFormData);
const fileOptionalFileName = MultiPartRequest.properties.find(
(x) => x.name === "fileOptionalFileName"
) as SdkBodyModelPropertyType;
ok(fileOptionalFileName);
strictEqual(fileOptionalFileName.optional, false);
ok(fileOptionalFileName.multipartOptions);
strictEqual(fileOptionalFileName.name, "fileOptionalFileName");
strictEqual(fileOptionalFileName.multipartOptions.isFilePart, true);
ok(fileOptionalFileName.multipartOptions.filename);
strictEqual(fileOptionalFileName.multipartOptions.filename.optional, true);
ok(fileOptionalFileName.multipartOptions.contentType);
strictEqual(fileOptionalFileName.multipartOptions.contentType.optional, true);

const fileRequiredFileName = MultiPartRequest.properties.find(
(x) => x.name === "fileRequiredFileName"
) as SdkBodyModelPropertyType;
ok(fileRequiredFileName);
strictEqual(fileRequiredFileName.optional, false);
ok(fileRequiredFileName.multipartOptions);
strictEqual(fileRequiredFileName.name, "fileRequiredFileName");
strictEqual(fileRequiredFileName.multipartOptions.isFilePart, true);
ok(fileRequiredFileName.multipartOptions.filename);
strictEqual(fileRequiredFileName.multipartOptions.filename.optional, false);
ok(fileRequiredFileName.multipartOptions.contentType);
strictEqual(fileRequiredFileName.multipartOptions.contentType.optional, false);
});

it("check 'multi' of multipart with @multipartBody for model", async function () {
await runner.compileWithBuiltInService(`
model Address {
Expand Down
Loading