[Draft, DO-Not-Merge] multipart #40967

chunyu3 · 2024-01-03T06:36:11Z

Contributing to the Azure SDK

This is a prototype for end-to-end multipart/form-data support. It contains the libraries and APIs needed in System.CodeModel and Azure.Core.

Add Mutipart helper MultipartFormData to help construct a multipart/form-data request content (RequestContent for Azure, and RequestBody to others) and Deserialize a Mulitpart/form-data response body (BinaryData) to Model

Please see our CONTRIBUTING.md if you are not familiar with contributing to this repository or have questions.

For specific information about pull request etiquette and best practices, see this section.

annelo-msft · 2024-01-03T16:14:49Z

sdk/core/Azure.Core/src/Shared/Multipart/MultipartFormDataRquestContent.cs

The name of Request is mispelled in this file

annelo-msft · 2024-01-03T16:15:15Z

sdk/core/Azure.Core/src/RequestContent.cs

This will be added in Azure.Core 2.0. Do we need it before that ships?

Can the options be defaulted/optional?

@annelo-msft This method is needed for multipart feature. I added there for current prototyping. When Azure.Core 2.0 released and I will remove the duplicate one. Thanks

And I think we can provide a default value ModelReaderWriterOptions.Json for options parameter.

annelo-msft · 2024-01-03T16:15:39Z

sdk/core/Azure.Core/src/Azure.Core.csproj

We don't ship betas of Azure.Core. Please add any beta features to Azure.Core.Experimental first.

OK. Thanks.

Yes. I will put all Azure.Core library files in Azure.Core.Experimental first.

Great! You should also be able to put any System.ClientModel files in Azure.Core.Experimental as well in order to beta them before shipping them GA in one of the GA Core libraries.

annelo-msft · 2024-01-03T16:17:19Z

eng/Packages.Data.props

We cannot arbitrarily upgrade package dependencies - we have to be aware of the versions Functions and PowerShell use to prevent runtime errors from trying to load multiple versions into the same environment.

We should consider adding a comment above these package references that links to the specific guidance for vetting interactions with Functions, PowerShell, etc.

I like that idea @christothes - would you be willing to put up a PR for it?

I created #40978 to track this. I think we have some folks that have recently investigated this that might be better suited to pick it up.

Thanks @annelo-msft @christothes . But we need to use the 'MediaType' property of BinaryData which is released in latest System.Memory.Data version 8.0.0.
So what's the process to upgrade the dependency library? Thanks

I can't find anything indicating that PowerShell has a dependency on this package, so I don't think this update would cause versioning issues there.

https://github.com/PowerShell/PowerShell/blob/master/src/System.Management.Automation/System.Management.Automation.csproj

https://github.com/PowerShell/PowerShell/blob/master/tools/cgmanifest.json

https://grep.app/search?q=system.memory.data&filter[repo][0]=PowerShell/PowerShell

Also I don't have any context around this PR but just wanted to check - this is changing the reference in the Track 1 dependencies list, was that intentional?

Thank you @m-redding!! Super helpful. I may see if I can do this change in the new System.ClientModel PR given @KrzysztofCwalina's comment: #41016 (comment)

@JoshLove-msft, do you know how we can understand if this would be a problem for Functions?

Functions runs against net6.0 and, for the in-proc model, will silently downgrade dependencies that the host has. We should stick with a package in the 6.0 line, if possible. If we need this, we'll have to run it past Fabio Cavalante to ensure that it won't conflict with the host.

We shouldn't be bumping the T1 libraries section regardless. These bumps should be made only in the T2 sections.

annelo-msft · 2024-01-03T16:18:33Z

sdk/core/System.ClientModel/src/Multipart/MultipartContent.cs

Please put this in Azure.Core.Experimental for first beta release.

@annelo-msft This is in System.ClientModel library which will be common library for both azure and non-azure.

Types in the System. namespace can also be added to Azure.Core.Experimental in order to beta them prior to GA.

annelo-msft

Please put this feature in Azure.Core.Experimental for a first beta release.

KrzysztofCwalina · 2024-01-03T16:51:47Z

Please add a description to this PR.

KrzysztofCwalina

So, I realize that lot's of this code was just copied from ASP.NET, but the code is internal in ASP.NET (probably for a reason). If we want to make the APIs public, we need to do proper cleanup and review with the BCL/ASP.NET team.

KrzysztofCwalina · 2024-01-03T16:52:50Z

sdk/core/Azure.Core/src/RequestContent.cs

Why is this hear and not on the base type?

KrzysztofCwalina · 2024-01-03T16:53:47Z

sdk/core/Azure.Core/src/RequestContent.cs

Can the options be defaulted/optional?

KrzysztofCwalina · 2024-01-03T17:02:40Z

sdk/core/Azure.Core/src/RequestContent.cs

Can we use ToMemory? It does not allocate but ToArray might. I think, you will have to multitraget to do it.

KrzysztofCwalina · 2024-01-03T17:03:15Z

sdk/core/Azure.Core/src/RequestContent.cs

Same as above. Please use ToMemory.

KrzysztofCwalina · 2024-01-03T17:03:38Z

sdk/core/Azure.Core/src/RequestContent.cs

KrzysztofCwalina · 2024-01-03T18:00:12Z

sdk/core/System.ClientModel/src/Multipart/MultipartFormDataContent.cs

is this the right namespace?

KrzysztofCwalina · 2024-01-03T18:01:32Z

sdk/core/System.ClientModel/src/Multipart/MultipartFormDataContent.cs

Can this throw?

KrzysztofCwalina · 2024-01-03T18:02:09Z

sdk/core/System.ClientModel/src/Multipart/MultipartFormDataContent.cs

So many substrings. Please use slicing.

KrzysztofCwalina · 2024-01-03T18:03:26Z

sdk/core/System.ClientModel/src/Multipart/MultipartFormDataContent.cs

why is the warning disabled?

KrzysztofCwalina · 2024-01-03T18:05:26Z

sdk/core/System.ClientModel/src/Multipart/MultipartReaderStream.cs

what is this for?

chunyu3 · 2024-01-04T07:47:39Z

Please put this feature in Azure.Core.Experimental for a first beta release.

Yes. I will put all Azure.Core library files in Azure.Core.Experimental first.

chunyu3 · 2024-01-04T07:49:49Z

Please add a description to this PR.

This is a prototype of End-to-End Multipart support. I added this in the description. Thanks.

KrzysztofCwalina · 2024-01-04T18:22:11Z

sdk/core/Azure.Core/src/RequestContent.cs

+            }
+            private BinaryData GetData()
+            {
+                BinaryData data = ModelReaderWriter.Write(_model, _options); // will call the write function in serialization file


our ModelReaderWriter can write to a sequence of buffers. But this Write method (returning BinaryData) forces the sequence to be copied to one contiguous buffer. Can we avoid that? Can we keep a sequence of buffers in ModelReadWriteRequestContent?

Hello @KrzysztofCwalina I only can find the api public static BinaryData Write<T>(T model, ModelReaderWriterOptions? options = default) where T : IPersistableModel<T> in ModelReaderWriter.cs. And I don't see a API which return a sequence of buffers.
Is it a new API which will be released in later version?

jsquire · 2024-02-26T16:01:54Z

eng/Packages.Data.props

-    <PackageReference Update="System.Text.Encodings.Web" Version="4.5.1" />
-    <PackageReference Update="System.Text.Json" Version="4.7.2" />
+    <PackageReference Update="System.Text.Encodings.Web" Version="8.0.0" />
+    <PackageReference Update="System.Text.Json" Version="8.0.0" />


We know that these are not safe to upgrade past the 6.0 line due to conflicts with Functions. We also know that 6.0 is good with the currently supported versions of PowerShell.

jsquire · 2024-02-26T16:02:33Z

eng/Packages.Data.props

We shouldn't be bumping the T1 libraries section regardless. These bumps should be made only in the T2 sections.

github-actions · 2024-05-03T05:04:45Z

Hi @chunyu3. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days.

chunyu3 requested review from JoshLove-msft, KrzysztofCwalina, annelo-msft, benbp, christothes, hallipr and weshaggard as code owners January 3, 2024 06:36

chunyu3 marked this pull request as draft January 3, 2024 06:36

github-actions bot added the Azure.Core label Jan 3, 2024

chunyu3 changed the title ~~multipart~~ [Draft] multipart Jan 3, 2024

annelo-msft reviewed Jan 3, 2024

View reviewed changes

sdk/core/Azure.Core/src/Shared/Multipart/MultipartFormDataRquestContent.cs Outdated

Copy link

Member

annelo-msft Jan 3, 2024

Choose a reason for hiding this comment

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

The name of Request is mispelled in this file

annelo-msft reviewed Jan 3, 2024

View reviewed changes

annelo-msft requested changes Jan 3, 2024

View reviewed changes

christothes mentioned this pull request Jan 3, 2024

/eng/Packages.Data.props should include a comment linking to dependency vetting guidance #40978

Closed

KrzysztofCwalina requested changes Jan 3, 2024

View reviewed changes

chunyu3 changed the title ~~[Draft] multipart~~ [Draft, DO-NOT-Merge] multipart Jan 4, 2024

chunyu3 changed the title ~~[Draft, DO-NOT-Merge] multipart~~ [Draft, DO-Not-Merge] multipart Jan 4, 2024

chunyu3 added the Do Not Merge label Jan 4, 2024

chunyu3 force-pushed the multipart branch from cf29b2a to af5379a Compare January 4, 2024 09:39

multipart helper

068947e

chunyu3 force-pushed the multipart branch from af5379a to 068947e Compare January 4, 2024 09:40

KrzysztofCwalina reviewed Jan 4, 2024

View reviewed changes

chunyu3 added 2 commits February 6, 2024 16:19

refact

8546a82

update

7850f34

convert mulitpart to stream

bbb7f1d

chunyu3 force-pushed the multipart branch from 3f6b426 to bbb7f1d Compare February 19, 2024 06:54

chunyu3 added 4 commits February 19, 2024 15:24

remove unused api

ca7517b

store content as object

5090bbb

remove unused code

7ae15c2

set content length default value

2123c20

chunyu3 force-pushed the multipart branch from 3ea0b5f to 2123c20 Compare February 21, 2024 05:49

chunyu3 added 2 commits February 21, 2024 15:26

add deserialize multipart form-data response

64a9b1c

remove duplicate code

9810b23

chunyu3 force-pushed the multipart branch from 1a08725 to 9810b23 Compare February 21, 2024 07:34

MultipartBodyPart content type only accept BinaryData and MultipartFile

0911bd9

chunyu3 force-pushed the multipart branch from 09fd5e0 to 0911bd9 Compare February 22, 2024 13:39

jsquire reviewed Feb 26, 2024

View reviewed changes

github-actions bot added the no-recent-activity There has been no recent activity on this issue. label May 3, 2024

chunyu3 closed this May 6, 2024

[Draft, DO-Not-Merge] multipart #40967

[Draft, DO-Not-Merge] multipart #40967

Uh oh!

Conversation

chunyu3 commented Jan 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contributing to the Azure SDK

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

annelo-msft left a comment

Choose a reason for hiding this comment

Uh oh!

KrzysztofCwalina commented Jan 3, 2024

Uh oh!

KrzysztofCwalina left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chunyu3 commented Jan 4, 2024

Uh oh!

chunyu3 commented Jan 4, 2024

Uh oh!

chunyu3 commented Jan 3, 2024 •

edited

Loading