[Files] Transform string into part for metadata in upload_files request#207
[Files] Transform string into part for metadata in upload_files request#207AMoldova-NI wants to merge 3 commits into
Conversation
Co-authored-by: Copilot <copilot@github.com>
AMoldova-NI
requested review from
cameronwaterman,
rbell517 and
spanglerco
as code owners
AMoldova-NI
changed the title
There was a problem hiding this comment.
Pull request overview
Fixes FileClient.upload_file() multipart encoding so the optional metadata argument is sent as a non-file form part (preventing the File service from rejecting it as a file), addressing the 400 Bad Request described in issue #206.
Changes:
- Encode
metadataas a multipart part tuple(None, json, "application/json")instead of passing a raw JSON string to the multipartPart. - Rename the intermediate variable from
metadata_strtometadata_partto reflect the new payload shape.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| self, client: FileClient, binary_file_data: BinaryIO, random_filename_extension: str | ||
| ): | ||
| file_name = random_filename_extension | ||
| metadata = {"CustomProp": "CustomValue"} | ||
|
|
||
| # Upload the file with metadata | ||
| file_id = client.upload_file(file=binary_file_data, metadata=metadata) |
There was a problem hiding this comment.
I think this is a real problem but we can just change the fixture scope to function so it gets a new BytesIO in each test. It was previously only used by one test, so this problem would not have manifested before.
| # Upload the file with metadata | ||
| file_id = client.upload_file(file=binary_file_data, metadata=metadata) | ||
|
|
||
| # Verify the file was created with correct metadata | ||
| files = client.get_files(ids=[file_id]) | ||
| assert files.total_count == 1 | ||
| assert len(files.available_files) == 1 | ||
| assert files.available_files[0].id == file_id | ||
| assert files.available_files[0].properties is not None | ||
| assert files.available_files[0].properties["Name"] == file_name | ||
| assert ( | ||
| len(files.available_files[0].properties.keys()) == len(metadata) + 1 | ||
| ) # Name + 1 custom property | ||
|
|
||
| client.delete_file(id=file_id) | ||
|
|
There was a problem hiding this comment.
I think the best approach is to change the test_file fixture to take the metadata so you can reuse it and its built-in file clean up rather than duplicating that clean up. You can still verify the delete if you want, but you can also just drop that and rely on the fixture's cleanup and only assert the get_files response has the properties you want.
| self, client: FileClient, binary_file_data: BinaryIO, random_filename_extension: str | ||
| ): | ||
| file_name = random_filename_extension | ||
| metadata = {"CustomProp": "CustomValue"} | ||
|
|
||
| # Upload the file with metadata | ||
| file_id = client.upload_file(file=binary_file_data, metadata=metadata) |
There was a problem hiding this comment.
I think this is a real problem but we can just change the fixture scope to function so it gets a new BytesIO in each test. It was previously only used by one test, so this problem would not have manifested before.
| # Upload the file with metadata | ||
| file_id = client.upload_file(file=binary_file_data, metadata=metadata) | ||
|
|
||
| # Verify the file was created with correct metadata | ||
| files = client.get_files(ids=[file_id]) | ||
| assert files.total_count == 1 | ||
| assert len(files.available_files) == 1 | ||
| assert files.available_files[0].id == file_id | ||
| assert files.available_files[0].properties is not None | ||
| assert files.available_files[0].properties["Name"] == file_name | ||
| assert ( | ||
| len(files.available_files[0].properties.keys()) == len(metadata) + 1 | ||
| ) # Name + 1 custom property | ||
|
|
||
| client.delete_file(id=file_id) | ||
|
|
There was a problem hiding this comment.
I think the best approach is to change the test_file fixture to take the metadata so you can reuse it and its built-in file clean up rather than duplicating that clean up. You can still verify the delete if you want, but you can also just drop that and rely on the fixture's cleanup and only assert the get_files response has the properties you want.
What does this Pull Request accomplish?
Fix
upload_filesmethod to accept metadata field with custom data.Why should this Pull Request be merged?
This fixes a bug issued here #206
What testing has been done?
Testing done using the script provided in the issue