[multipart] Support `bytes[]/content type` #2380

msyyc · 2024-01-23T08:32:54Z

pending on Azure/azure-sdk-for-python#33948, Azure/azure-sdk-for-python#34021

packages/autorest.python/autorest/codegen/templates/vendor.py.jinja2

packages/typespec-python/test/azure/mock_api_tests/test_payload_multipart_file_array.py

msyyc · 2024-01-24T01:38:58Z

packages/typespec-python/test/azure/mock_api_tests/test_payload_multipart_file_array.py

+    body.update(data)
+    with pytest.raises(TypeError):
+        # caused by deepcopy when DPG model init
+        op(model_class(body))


DPG model will raise error when deepcody of init if property value is io. @iscai-msft

iscai-msft · 2024-01-25T23:53:27Z

packages/autorest.python/autorest/codegen/models/parameter.py

@@ -23,7 +23,9 @@
 from .constant_type import ConstantType
 from .utils import add_to_description
 from .combined_type import CombinedType
-from .model_type import JSONModelType
+from .model_type import JSONModelType, DPGModelType


wit this dpg PR Azure/typespec-azure#166 all we have to do is create a new type MultipartFileType and then type this type as Union[binary, Tuple[string, binary], Tuple[string, binary, string]]

packages/autorest.python/autorest/codegen/serializers/builder_serializer.py

iscai-msft · 2024-01-26T23:54:49Z

packages/typespec-python/test/azure/generated/payload-multipart/payload/multipart/_vendor.py


+# file-like tuple could be `(filename, IO (or bytes))` or `(filename, IO (or bytes), content_type)`
+FileType = Union[IOBase, bytes]


I would use the type hints I have here from core https://github.com/Azure/azure-sdk-for-python/blob/main/sdk/core/azure-core/azure/core/rest/_helpers.py#L79

iscai-msft · 2024-01-26T23:58:50Z

packages/typespec-python/test/azure/generated/payload-multipart/payload/multipart/_vendor.py

        return file
-    return NamedBytesIO("auto-name-" + str(uuid.uuid4()), file)
+    return ("auto-name-" + str(uuid.uuid4()), file)


why are we giving auto names to these file fields? That's def not what we should be doing. We can get the field name from the typespec and then create a tuple of all of the files entries.

So

class MyMultipartModel: profile_image: MultipartFile = rest_field("profileImage") pictures: MultipartFile[] = rest_field("pictures") def my_multipart_func(body: MyMultipartModel): _files = [("profileImage", body.profile_image)] _files.extend([("pictures", p)] for p in body.pictures) request = HttpRequest(..., files=_files) ...

why are we giving auto names to these file fields?
-> According to https://www.rfc-editor.org/rfc/rfc7578#section-4.2, file name SHOULD be supplied. In my local test, the flask of python/multer of js will not parse request into files part if no file name provided. So I think SDK need to provide a name for it if users don't provide.

why are we giving auto names to these file fields? That's def not what we should be doing. We can get the field name from the typespec and then create a tuple of all of the files entries.

So

class MyMultipartModel: profile_image: MultipartFile = rest_field("profileImage") pictures: MultipartFile[] = rest_field("pictures") def my_multipart_func(body: MyMultipartModel): _files = [("profileImage", body.profile_image)] _files.extend([("pictures", p)] for p in body.pictures) request = HttpRequest(..., files=_files) ...

Current logic is actually similar to yours and it also compatible when body type is JSON

SHOULD doesn't mean MUST. If the IO is not a file descriptor and we don't have a name, we don't set it. No autoname, this will be sent to the servirce, that may save it, or even act based on the received name.

Reading the code more, this function should not exist

iscai-msft · 2024-01-27T00:00:06Z

packages/autorest.python/autorest/codegen/models/parameter.py

@@ -278,6 +280,25 @@ def has_json_model_type(self) -> bool:
            return self.type.target_model_subtype((JSONModelType,)) is not None
        return isinstance(self.type, JSONModelType)

+    @property


also we can actually support data and files entered, and pass them as HttpRequest(data=data, files=files) once this pr gets merged and released Azure/azure-sdk-for-python#34021

Before #2380 (comment), it passes data and files. Now the code merge all part into files and it works. SDK users will not be influenced no matter SDK merge all in files or pass data/files so I think current logic is OK.

packages/typespec-python/test/azure/mock_api_tests/test_payload_multipart_file_array.py

…multipart-file-array-dev5

...ec-python/test/azure/generated/payload-multipart/payload/multipart/operations/_operations.py

msyyc · 2024-02-02T01:55:32Z

packages/typespec-python/test/generic_mock_api_tests/test_payload_multipart.py

@@ -52,7 +52,7 @@ def client():
 def test_multi_part(client: MultiPartClient, op_name, model_class, data, file):
    op = getattr(client.form_data, op_name)
    # test bytes
-    body = {k: open(str(v), "rb").read() for k, v in file.items()}
+    body = {k: ("blob", open(str(v), "rb").read(), "application/octet-stream") for k, v in file.items()}


Lack test for binary_array_parts/complex, and you can refer to my previous commit:

autorest.python/packages/typespec-python/test/azure/mock_api_tests/test_payload_multipart.py

Lines 56 to 71 in fba4a91

"binary_array_parts",

models.BinaryArrayPartsRequest,

{"id": "123"},

{"pictures": [PNG, PNG]},

{},

),

(

"complex",

models.ComplexPartsRequest,

{"id": "123", "previousAddresses": [models.Address(city="Y"), models.Address(city="Z")], "address": models.Address(city="X")},

{"pictures": [PNG, PNG], "profileImage": JPG},

{},

),

],

)

def test_multi_part(client: MultiPartClient, op_name, model_class, data, file, file_info):

oh ok, i was confused by the presence of this file since it's generic, it should be in generic_mock_api_+tests

msyyc added 3 commits January 23, 2024 16:29

init

94e6e3d

rename file

a45a47c

lint

575568f

iscai-msft reviewed Jan 23, 2024

View reviewed changes

packages/autorest.python/autorest/codegen/templates/vendor.py.jinja2 Outdated Show resolved Hide resolved

msyyc commented Jan 24, 2024

View reviewed changes

packages/typespec-python/test/azure/mock_api_tests/test_payload_multipart_file_array.py Outdated Show resolved Hide resolved

msyyc commented Jan 24, 2024

View reviewed changes

msyyc requested a review from iscai-msft January 25, 2024 02:21

review

b8fd3ff

iscai-msft reviewed Jan 25, 2024

View reviewed changes

packages/autorest.python/autorest/codegen/serializers/builder_serializer.py Outdated Show resolved Hide resolved

msyyc added 2 commits January 26, 2024 13:51

update

97932ba

fix cspell

ca46a17

msyyc requested a review from iscai-msft January 26, 2024 06:26

iscai-msft reviewed Jan 27, 2024

View reviewed changes

packages/typespec-python/test/azure/mock_api_tests/test_payload_multipart_file_array.py Outdated Show resolved Hide resolved

msyyc and others added 3 commits January 29, 2024 09:59

Merge branch 'main' into multipart-file-array-dev5

44747f3

update test

fba4a91

update test

f7d4ce6

msyyc requested a review from iscai-msft January 29, 2024 06:30

msyyc marked this pull request as ready for review January 29, 2024 06:49

msyyc requested a review from tadelesh as a code owner January 29, 2024 06:49

iscai-msft added 9 commits January 29, 2024 18:01

accept files input, doesn't work

f0eb959

don't use unreleased tcgc

7280ce8

Merge branch 'main' of https://github.com/Azure/autorest.python into …

a847978

…multipart-file-array-dev5

Merge branch 'main' of https://github.com/Azure/autorest.python into …

6493106

…multipart-file-array-dev5

revert test changes to multipart

3d78277

try to pass payload multipart, waiting on tcgc

fbc88e6

can generate list of multipart file types

e72641e

regen

5518329

update to dev 4 version

9c4db12

iscai-msft added 18 commits January 31, 2024 23:05

black

915f34b

Merge branch 'main' of https://github.com/Azure/autorest.python into …

7062043

…multipart-file-array-dev5

linting checks

4a887a1

fix gen and add dpg changes

b41e539

remove multipartFile section from types

ca66468

run black

8734335

remove files added to azure mock api tests

fe19141

use eggs for azure-core and corehttp in main

0375e3d

import email.utils

1d977fd

generate with optional filetype

8f53d03

give typehint to _data

0da11ba

fix typing for files

7bfd08f

bump azure-core versions

6704313

regen with fixed files typing

dcf59fa

prepare changelog and package.json

3eb1f38

black

2c256e6

fix corehttp dep in setup.py

6d3da87

mypy fix

2533326

msyyc commented Feb 2, 2024

View reviewed changes

...ec-python/test/azure/generated/payload-multipart/payload/multipart/operations/_operations.py Outdated Show resolved Hide resolved

msyyc commented Feb 2, 2024

View reviewed changes

iscai-msft added 8 commits February 2, 2024 13:11

use FilesType typing for _files

c15d6b4

fix mypy hopefully

43520d4

regen with correct relative import of FileType

e8d1b7a

improve code structure

ad3174d

fix generation for autorest

9731549

add complex tests

e49b726

fix mypy and pyright of generated helper functions

7ad0dbb

remove unused imports in generated code

a7a41c9

iscai-msft merged commit 59f42d1 into main Feb 2, 2024
10 checks passed

iscai-msft deleted the multipart-file-array-dev5 branch February 2, 2024 21:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[multipart] Support `bytes[]/content type` #2380

[multipart] Support `bytes[]/content type` #2380

msyyc commented Jan 23, 2024 •

edited

Loading

msyyc Jan 24, 2024

iscai-msft Jan 25, 2024

iscai-msft Jan 26, 2024

iscai-msft Jan 26, 2024

msyyc Jan 29, 2024

msyyc Jan 29, 2024

lmazuel Jan 29, 2024

lmazuel Jan 29, 2024

iscai-msft Jan 27, 2024

msyyc Jan 29, 2024

msyyc Feb 2, 2024

iscai-msft Feb 2, 2024


		# file-like tuple could be `(filename, IO (or bytes))` or `(filename, IO (or bytes), content_type)`
		FileType = Union[IOBase, bytes]

	"binary_array_parts",
	models.BinaryArrayPartsRequest,
	{"id": "123"},
	{"pictures": [PNG, PNG]},
	{},
	),
	(
	"complex",
	models.ComplexPartsRequest,
	{"id": "123", "previousAddresses": [models.Address(city="Y"), models.Address(city="Z")], "address": models.Address(city="X")},
	{"pictures": [PNG, PNG], "profileImage": JPG},
	{},
	),
	],
	)
	def test_multi_part(client: MultiPartClient, op_name, model_class, data, file, file_info):

[multipart] Support bytes[]/content type #2380

[multipart] Support bytes[]/content type #2380

Conversation

msyyc commented Jan 23, 2024 • edited Loading

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

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

[multipart] Support `bytes[]/content type` #2380

[multipart] Support `bytes[]/content type` #2380

msyyc commented Jan 23, 2024 •

edited

Loading