Multipart form-data #4613

chunyu3 · 2024-04-22T06:04:38Z

Description

Fix #4339
Fix #4340
Fix #4338

implement multipart/form-data
Codegen will generate core library MultipartFormDataRequestConent for azure, and MultipartFormDataBinaryContent for non-azure
each model will generate ToMultipartRequestContent to serialize model to multipart/form-data format
Add contentType parameter in protocol method
Convenience method will generate MultipartFormDataRequestContent/MultipartFormDataBinaryContent from model and call protocol method.

Checklist

To ensure a quick review and merge, please ensure:

The PR has a understandable title and description explaining the why and what.
The PR is opened in draft if not ready for review yet.
- If opened in draft, please allocate sufficient time (24 hours) after moving out of draft for review
The branch is recent enough to not have merge conflicts upon creation.

Ready to Land?

Build is completely green
- Submissions with test failures require tracking issue and approval of a CODEOWNER
At least one +1 review by a CODEOWNER
All -1 reviews are confirmed resolved by the reviewer
- Override/Marking reviews stale must be discussed with CODEOWNERS first

…into multipart-adoption

…ntProvider

…into multipart-adoption

src/AutoRest.CSharp/LowLevel/Output/ConvenienceMethod.cs

src/AutoRest.CSharp/LowLevel/Output/OperationMethodChainBuilder.cs

src/AutoRest.CSharp/Common/Output/Builders/MultipartSerializationMethodsBuilder.cs

...rp/Common/Output/Models/Types/HelperTypeProviders/MultipartFormDataRequestContentProvider.cs

src/AutoRest.CSharp/LowLevel/Generation/DpgClientWriter.cs

src/TypeSpec.Extension/Emitter.Csharp/src/type/usage.ts

src/AutoRest.CSharp/Common/Output/Builders/MultipartSerializationMethodsBuilder.cs

…ionMethodsBuilder.cs Co-authored-by: Dapeng Zhang <ufo54153@gmail.com>

src/AutoRest.CSharp/Common/Output/Builders/MultipartSerializationMethodsBuilder.cs

annelo-msft · 2024-04-30T17:05:58Z

...ts/Platform-OpenAI-TypeSpec/src/Generated/Models/CreateTranscriptionRequest.Serialization.cs

+        internal virtual MultipartFormDataBinaryContent ToMultipartBinaryBody()
+        {
+            MultipartFormDataBinaryContent content = new MultipartFormDataBinaryContent();
+            content.Add(File, "file", "file", "application/octet-stream");


We should not pass a value for filename here if there is no file name.

annelo-msft · 2024-04-30T17:08:21Z

...ojects/Platform-OpenAI-TypeSpec/src/Generated/Models/CreateImageEditRequest.Serialization.cs

+            content.Add(Image, "image", "image", "application/octet-stream");
+            if (Optional.IsDefined(Mask))
+            {
+                content.Add(Mask, "mask", "mask", "application/octet-stream");


This will not work -- OpenAI needs to be able to pass the file name to this method to correctly format the header for the Mask property in the multipart/form-data content payload.

annelo-msft · 2024-05-01T23:47:24Z

test/UnbrandedProjects/Platform-OpenAI-TypeSpec/src/Generated/AudioTranscriptions.cs

        /// <param name="options"> The request options, which can override default behaviors of the client pipeline on a per-call basis. </param>
        /// <exception cref="ArgumentNullException"> <paramref name="content"/> is null. </exception>
        /// <exception cref="ClientResultException"> Service returned a non-success status code. </exception>
        /// <returns> The response returned from the service. </returns>
-        public virtual async Task<ClientResult> CreateAsync(BinaryContent content, RequestOptions options = null)
+        public virtual async Task<ClientResult> CreateAsync(BinaryContent content, string contentType, RequestOptions options = null)
        {
            Argument.AssertNotNull(content, nameof(content));


We should add Assert.NotNullOrEmpty(contentType, nameof(contentType)); here -- I think I had left this out of the original design. Thanks to @joseharriaga for catching this!

annelo-msft · 2024-05-02T15:30:15Z

...dedProjects/Platform-OpenAI-TypeSpec/src/Generated/Models/CreateFileRequest.Serialization.cs

+        internal virtual MultipartFormDataBinaryContent ToMultipartBinaryBody()
+        {
+            MultipartFormDataBinaryContent content = new MultipartFormDataBinaryContent();
+            content.Add(File, "file", "file", "application/octet-stream");


We should also not be hard-coding the content-type here.

annelo-msft · 2024-05-02T15:31:18Z

...edProjects/Platform-OpenAI-TypeSpec/src/Generated/Internal/MultipartFormDataBinaryContent.cs

+        {
+            get
+            {
+                return _multipartContent.Headers.ContentType.ToString();


It looks like the Debug.Assert(_multipartContent.Headers.ContentType is not null); was missed here from the design spec

annelo-msft · 2024-05-02T15:33:14Z

...edProjects/Platform-OpenAI-TypeSpec/src/Generated/Internal/MultipartFormDataBinaryContent.cs

+            Span<char> chars = new char[70];
+            byte[] random = new byte[70];
+            _random.NextBytes(random);
+            int mask = 255 >> 2;


It looks like the comment explaining this is missing.

annelo-msft · 2024-05-02T15:33:33Z

...edProjects/Platform-OpenAI-TypeSpec/src/Generated/Internal/MultipartFormDataBinaryContent.cs

+            byte[] random = new byte[70];
+            _random.NextBytes(random);
+            int mask = 255 >> 2;
+            for (int i = 0; i < 70; i++)


There should be a Debug.Assert(_boundaryValues.Length - 1 == mask); preceding this line.

annelo-msft · 2024-05-02T15:34:45Z

...edProjects/Platform-OpenAI-TypeSpec/src/Generated/Internal/MultipartFormDataBinaryContent.cs

+            Argument.AssertNotNull(content, nameof(content));
+            Argument.AssertNotNullOrEmpty(name, nameof(name));
+
+            string value = content.ToString("G", CultureInfo.InvariantCulture);


The comment is missing from this line.

annelo-msft · 2024-05-02T15:36:01Z

...edProjects/Platform-OpenAI-TypeSpec/src/Generated/Internal/MultipartFormDataBinaryContent.cs

+
+        public static void AddFilenameHeader(HttpContent content, string name, string filename)
+        {
+            ContentDispositionHeaderValue header = new ContentDispositionHeaderValue("form-data") { Name = name, FileName = filename };


The comment is missing from this method.

annelo-msft · 2024-05-02T15:36:37Z

...edProjects/Platform-OpenAI-TypeSpec/src/Generated/Internal/MultipartFormDataBinaryContent.cs

+
+        public static void AddContentTypeHeader(HttpContent content, string contentType)
+        {
+            MediaTypeHeaderValue header = new MediaTypeHeaderValue(contentType);


Why are we adding this in a separate method? I don't believe this is needed and should be removed.

Each part(not just file) of the multipart/form-data payload may set the content-type to indicate its content type(see following example). We need provide a way to set content-type of a part.

The BCL type will do this automatically for you.

BCL type will give default one. The request is customer can specify the content-type to each part.

chunyu3 added 30 commits March 12, 2024 12:44

merge latest autorest.csharp

20ed63c

Merge branch 'feature/v3' of https://github.com/Azure/autorest.csharp …

11a8146

…into multipart-adoption

add multipart serialization

eb1715e

deserialize

f151cb2

remove serialization model for form data

1d70660

refine code

07c6917

Merge branch 'feature/v3' of https://github.com/Azure/autorest.csharp …

89f9197

…into multipart-adoption

Merge branch 'feature/v3' of https://github.com/Azure/autorest.csharp …

6f5a3cb

…into multipart-adoption

add shared code

7733aa9

add FromResponse method

d5b7bc3

implement FromResponse for multipart

62e52ab

implement serializeValue

b63b845

refine fromResponse method

63be1d3

refact

9dfd302

Merge branch 'feature/v3' of https://github.com/Azure/autorest.csharp …

085ab0c

…into multipart-adoption

add content type in valueSerialization

fa1a4c0

remove toBinaryExpress from additionalProperty

9c2fa8e

support both azure and non-azure

b713e00

support azure and non-azure

8a6a910

add MultipartFormDataRequestContentProvider

73fb4bb

remove multipart response and implement MultipartFormDataRequestConte…

9f0c10d

…ntProvider

MultipartFormDataRequestContentProvider

87b4279

Merge branch 'feature/v3' of https://github.com/Azure/autorest.csharp …

21cd084

…into multipart-adoption

MultipartFormDataRequestContentProvider

830f748

generate mutlipart/form-data method

e124034

serialize multipart model and generate mulipart operation

1f6089b

use ModelReaderWriter to convert model to json

219a218

implement mulitpart for non-branding

493024a

add test project

26ecd48

implement serializeMultipart function

e0c3be7

ArcturusZhang reviewed Apr 26, 2024

View reviewed changes

src/AutoRest.CSharp/LowLevel/Output/ConvenienceMethod.cs Outdated Show resolved Hide resolved

ArcturusZhang reviewed Apr 26, 2024

View reviewed changes

src/AutoRest.CSharp/LowLevel/Output/OperationMethodChainBuilder.cs Outdated Show resolved Hide resolved

ArcturusZhang reviewed Apr 26, 2024

View reviewed changes

src/AutoRest.CSharp/LowLevel/Output/OperationMethodChainBuilder.cs Outdated Show resolved Hide resolved

ArcturusZhang reviewed Apr 26, 2024

View reviewed changes

src/AutoRest.CSharp/LowLevel/Output/OperationMethodChainBuilder.cs Outdated Show resolved Hide resolved

lirenhe reviewed Apr 26, 2024

View reviewed changes

src/AutoRest.CSharp/Common/Output/Builders/MultipartSerializationMethodsBuilder.cs Show resolved Hide resolved

resove comments

9269bb9

ArcturusZhang reviewed Apr 26, 2024

View reviewed changes

...rp/Common/Output/Models/Types/HelperTypeProviders/MultipartFormDataRequestContentProvider.cs Outdated Show resolved Hide resolved

archerzz reviewed Apr 26, 2024

View reviewed changes

src/AutoRest.CSharp/LowLevel/Generation/DpgClientWriter.cs Outdated Show resolved Hide resolved

src/TypeSpec.Extension/Emitter.Csharp/src/type/usage.ts Show resolved Hide resolved

azure-sdk mentioned this pull request Apr 26, 2024

Update autorest version to prerelease 3.0.0-alpha.20240426.1.x Azure/azure-sdk-for-net#43670

Draft

regen

055380d

chunyu3 requested review from archerzz, lirenhe and ArcturusZhang April 26, 2024 07:55

ArcturusZhang reviewed Apr 26, 2024

View reviewed changes

src/AutoRest.CSharp/Common/Output/Builders/MultipartSerializationMethodsBuilder.cs Outdated Show resolved Hide resolved

Update src/AutoRest.CSharp/Common/Output/Builders/MultipartSerializat…

da4891c

…ionMethodsBuilder.cs Co-authored-by: Dapeng Zhang <ufo54153@gmail.com>

chunyu3 requested a review from ArcturusZhang April 26, 2024 08:19

ArcturusZhang reviewed Apr 26, 2024

View reviewed changes

ArcturusZhang approved these changes Apr 26, 2024

View reviewed changes

chunyu3 merged commit dac2fba into Azure:feature/v3 Apr 26, 2024
8 checks passed

annelo-msft reviewed Apr 30, 2024

View reviewed changes

annelo-msft reviewed May 1, 2024

View reviewed changes

annelo-msft reviewed May 2, 2024

View reviewed changes

chunyu3 mentioned this pull request May 28, 2024

calculate multipart usage microsoft/typespec#3458

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multipart form-data #4613

Multipart form-data #4613

chunyu3 commented Apr 22, 2024 •

edited

annelo-msft Apr 30, 2024

annelo-msft Apr 30, 2024

annelo-msft May 1, 2024

annelo-msft May 2, 2024

annelo-msft May 2, 2024

annelo-msft May 2, 2024

annelo-msft May 2, 2024 •

edited

annelo-msft May 2, 2024

annelo-msft May 2, 2024

annelo-msft May 2, 2024

chunyu3 May 6, 2024

annelo-msft May 6, 2024

chunyu3 May 7, 2024

Multipart form-data #4613

Multipart form-data #4613

Conversation

chunyu3 commented Apr 22, 2024 • edited

Description

Checklist

Ready to Land?

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annelo-msft May 2, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chunyu3 commented Apr 22, 2024 •

edited

annelo-msft May 2, 2024 •

edited