Skip to content
This repository has been archived by the owner on Aug 4, 2023. It is now read-only.

fixes stream validation #464

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

smoebody
Copy link
Contributor

this test works before 17e77be

Sandy Chapman and others added 2 commits April 12, 2016 15:04
…ull schema is failing validation.

Added code to ensure unit test passes by doing a check if no data is passed to end and no data is written. In this situation some code expects undefined to be passed to the validators.

Delay converting response buffers to string as long as possible. Also, don't convert buffers sourced from a file to a string as it'll mess with the encoding. There's nothing to validate on files anyway.
@smoebody smoebody force-pushed the stream-response-validation-bug branch from f065acf to 7f9cb81 Compare December 20, 2016 09:54
@whitlockjc
Copy link
Member

What is the failure for this? Can you wire up an error handler and dump the whole object? (This isn't as easy as JSON.stringify(err), you'll need to write a function to map over the error keys and build the object that way.)

@smoebody
Copy link
Contributor Author

uh, i don't know what you mean. err is null by the way. the problem is, that the response body does not equal the file content. im simply using the hash function of the crypto module to create a comparable hash for the content of the file and the response data.

@whitlockjc
Copy link
Member

Oh, I got you. I thought stream validation was throwing an error where it wasn't suppose to. Let me look further.

@smoebody smoebody mentioned this pull request Jan 11, 2017
@smoebody smoebody changed the title added test for stream validation bug #463 fixed stream validation Mar 13, 2017
@smoebody smoebody changed the title fixed stream validation fixes stream validation Mar 13, 2017
@smoebody
Copy link
Contributor Author

awh, please! timeout on node 0.12. srsly?! how can i retrigger travis tests?

@whitlockjc
Copy link
Member

Triggered.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@smoebody
Copy link
Contributor Author

I signed it!

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

@smoebody
Copy link
Contributor Author

I signed it!

@smoebody
Copy link
Contributor Author

@whitlockjc can you please have a look at master? it seems that the tests fail recently. i checked it out and performed it locally and the same errors occur as of this PR. Thanks

@whitlockjc
Copy link
Member

I'll fix master and when I do, you'll need to rebase this. I don't know what's up with the CLA showins as "failed".

Would you mind submitting something similar for sway? I assume it's got the same issue.

@lerit
Copy link

lerit commented Dec 4, 2018

@whitlockjc
can you please help merge this?
this resolve my problem,before that,i can not pipe or res.download(get corrupt file ),with this pull,i can get correct file
thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants