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

Add support for ArrayBuffer request bodies #1776

Merged
merged 1 commit into from
Jan 11, 2021

Conversation

imiric
Copy link
Contributor

@imiric imiric commented Dec 23, 2020

This is part of #1020, just for supporting ArrayBuffer request bodies. It works fine for submitting binary data from open(), e.g.:

var img = open('image.png', 'b');

exports.default = function() {
  var arr = new Uint8Array(img);
  http.post('http://127.0.0.1:9000/postbin', arr.buffer,
            { headers: { 'Content-Type': 'image/png' }});
}

It should probably have much more tests, but let me know what other scenarios we should cover. All the other typed arrays?

I wasn't able to get it to work with submitting the typed array itself instead of the ArrayBuffer, i.e. submitting arr instead of arr.buffer above (as mentioned here). Goja's Export() on it returns an empty map[string]interface{}{}, so I couldn't get the ArrayBuffer from it. We should probably raise a warning in that case, unless we can work around it.

@imiric imiric requested review from mstoykov and na-- December 23, 2020 15:56
Copy link
Contributor

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

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

LGTM!

I think @na-- and I agreed upon just supporting ArrayBuffer for now. The Support for Int8Array(for example) seemed a little funky and at least for an MVP it is fine especially if we can error out to the user correctly.

Unfortunately, as you have noticed if someone provides Int8Array it enters the map[string]interface{} case with 0 keys so changing the error in the default won't be sufficient. IMO there is no case where this is what the user wanted - so we can error on that one as well (possibly in the handleObjectBody)?

The map[string]goja.Value is only used from the SubmitForm code it seems? Is there a case where we could want to submit a form with no data? I think not, I think it needs at least some info inside of it or am I wrong?

@imiric
Copy link
Contributor Author

imiric commented Jan 11, 2021

@mstoykov I don't think it would be safe to error out in that case, as we have no way to determine whether the user passed a typed array or an empty object (that I can see). If anything it should probably be logged as a warning, but given that we can't distinguish between those scenarios I'm inclined to leave it as is.

@na-- What do you think?

@na--
Copy link
Member

na-- commented Jan 11, 2021

Hmm it'd have been nice if we could prevent footguns like that, but if there doesn't seem to be a good way to differentiate the typed arrays from other potential arguments, I'm not sure if there's anything we can do about it 🤷‍♂️

@mstoykov, goja seems to have custom export() and exportType() methods for arrayBufferObject, but not for typedArrayObject. Maybe that's why they get exported as map[string]interface{}, some default export for all JS objects? If so, it might be worth to make a small goja PR that adds these methods, which would then allow us to differentiate and work with these types in Go?

Other than that, the code of the PR mostly LGTM. I'd only advise moving the test to its own standalone function, I don't see why we have to cram it into the already huge composite http test we have.

@mstoykov
Copy link
Contributor

@mstoykov, goja seems to have custom export() and exportType() methods for arrayBufferObject, but not for typedArrayObject. Maybe that's why they get exported as map[string]interface{}, some default export for all JS objects? If so, it might be worth to make a small goja PR that adds these methods, which would then allow us to differentiate and work with these types in Go?

What do you propose they are exported to? ArrayBuffer?

mstoykov
mstoykov previously approved these changes Jan 11, 2021
@codecov-io
Copy link

codecov-io commented Jan 11, 2021

Codecov Report

Merging #1776 (6c40775) into master (ccffbca) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1776      +/-   ##
==========================================
- Coverage   71.43%   71.42%   -0.01%     
==========================================
  Files         178      178              
  Lines       13777    13783       +6     
==========================================
+ Hits         9841     9845       +4     
  Misses       3323     3323              
- Partials      613      615       +2     
Flag Coverage Δ
ubuntu 71.40% <100.00%> (-0.01%) ⬇️
windows 69.97% <100.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
js/modules/k6/http/request.go 80.14% <100.00%> (+0.14%) ⬆️
js/lib/lib.go 63.63% <0.00%> (-36.37%) ⬇️
js/bundle.go 90.60% <0.00%> (+0.06%) ⬆️
lib/executor/vu_handle.go 95.23% <0.00%> (+1.90%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ccffbca...6c40775. Read the comment docs.

@na--
Copy link
Member

na-- commented Jan 11, 2021

What do you propose they are exported to? ArrayBuffer?

No, their constituent types, see this

@imiric imiric force-pushed the feat/1020-better-binary-data branch from c51e61e to 6c40775 Compare January 11, 2021 15:43
@imiric imiric requested a review from mstoykov January 11, 2021 15:59
@imiric imiric merged commit 28e362d into master Jan 11, 2021
@imiric imiric deleted the feat/1020-better-binary-data branch January 11, 2021 16:10
@na-- na-- added this to the v0.30.0 milestone Jan 11, 2021
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.

4 participants