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

Support binary data in MTOM #1245

Merged
merged 2 commits into from
Aug 4, 2024
Merged

Support binary data in MTOM #1245

merged 2 commits into from
Aug 4, 2024

Conversation

CyBot
Copy link
Contributor

@CyBot CyBot commented Jul 11, 2024

options.data += `${part.body}\r\n--${boundary}${

converted the body data to a string, making it impossible to send binary data

@w666
Copy link
Collaborator

w666 commented Jul 16, 2024

Hi @CyBot,

New feature should not be used by default, please make it optional.
New feature requires a test.
Also, please make sure all current tests still work.

And this did not pass lint check, should be fixe as well.

@CyBot
Copy link
Contributor Author

CyBot commented Jul 16, 2024

IMO it should be the default, because currently it's not working correctly. The whole point of MTOM is sending binary data.
As such it's not a new feature, it's a bugfix.
I can write a test when I get around to it

P.S.: I'll look into the failing test

@CyBot
Copy link
Contributor Author

CyBot commented Jul 17, 2024

The feature that is breaking the test now was not working.
In the test, a readStream is created and passed as the body. All that was put into the request was [object Object].
It could be fixed/implemented, but I think that means the whole http client building has to be made asynchronous, to read the stream data into a buffer.
Right now, I think the test should be adapted to actually test a working feature.

@w666 w666 self-requested a review July 17, 2024 07:22
@CyBot
Copy link
Contributor Author

CyBot commented Jul 18, 2024

I fixed the tests - they now pass the attachment body as a buffer, not a ReadStream.

The test fix should maybe go into a separate commit, I can split them and create a separate PR if requested

 Uncomment test code that was disabled when migrated
 from Request to Axios.
@w666
Copy link
Collaborator

w666 commented Jul 19, 2024

Hi @CyBot,

Could you allow me to push to your forked repo? I made some changes in the tests.
If no, I will just raise another PR when I merge your PR.

In general, it looks good, seems like it works as expected now.

@w666
Copy link
Collaborator

w666 commented Jul 23, 2024

@CyBot I pushed some changes, added additional assertion on server side and uncommented assertions that were disabled some time ago.

@w666
Copy link
Collaborator

w666 commented Jul 23, 2024

Approved, I will include this into next release. Thanks for the fix.

@w666 w666 merged commit d491572 into vpulim:master Aug 4, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants