Skip to content
This repository has been archived by the owner on Jul 12, 2019. It is now read-only.

Pulling in upstream updates to restler #3

Merged
merged 79 commits into from
Mar 31, 2015
Merged

Pulling in upstream updates to restler #3

merged 79 commits into from
Mar 31, 2015

Conversation

zbirkenbuel
Copy link

Not quite to the extent that winston was, but restler has advanced since we forked. This branch pulls in all of those changes. Again, probably easier to compare my branch to upstream master which reflects the changes we've made to the codebase:

https://github.com/danwrong/restler/compare/master...HBOCodeLabs:zackb-massive?expand=1

the changes shown effectively reflect what @jstamerj committed to master in the one and only merge: 1a736d2

evdb and others added 30 commits May 17, 2012 15:15
Including Authorization header if an empty password (not undefined) is specified to handle the case where an api key is passed via username but no password is required (for example, asana.com)
Changed EventEmitter inheritens to Node0.10 style.
Fixed tests that were failing against xml2js.

Added a npm test script to the package.json

Minor refactor of the tests.
…ndFixTests

Updated packages to latest version.
Updated instructions for running the tests.

Added install instructions.
Added missing documentation for PATCH HTTP method
Fixes danwrong#115: Allow users to pass through rejectUnauthorized
Set Content-Length for empty data response.
Allow empty-password authorization header
…ptypassword

Allow empty password auth header with test.
Adding node-querystring due to incomplete qs in node code.
Version bump in accordance with semver.
Updates to documentation and supported node engines.
Gustavo Henke and others added 20 commits March 8, 2014 23:51
Add missing doc for rejectUnauthorized option
Fix Data object support (with test)
Allow writing buffer directly to req body
Retina-ready svg icons indicating current version and Node.js dependency rather than as text, which was harder to find
@zbirkenbuel
Copy link
Author

@shimeez
Copy link

shimeez commented Mar 30, 2015

@jstamerj Wasn't there some bug that caused us to double encode bodies on retries? Was this a fix in restler?

@zbirkenbuel
Copy link
Author

@shimeez, I'm assuming that's what the move of data from the options blob to the root object was about. The link to @jstamerj 's merge should show the same changes here.

@jstamerj
Copy link

@zbirkenbuel @shimeez
Right -- Restler code would automatically retry some failures, but it would screw up the request body on all the retries. This once caused ATV to flood the playback marker service with requests. The initial request would fail for some other reason, and then the retries would repeatedly fail because restler was sending bad data in the request body. In some cases it was sending a GET request with a request body which obviously does not make sense.

The first time through, the code would convert this.options.data from a string to a buffer. On retries, it would stringify this buffer (which produces a much different string than the original), and convert that string into a buffer.

I submitted a PR to restler (danwrong#124), but it looks like they did not take the fix.

It looks like they fixed it in a different way 'tho (danwrong@a5901e6), so prehaps we can get rid of our restler fork.

@zbirkenbuel
Copy link
Author

@jstamerj, yeah, I was just reading through the commit history since there was a merge conflict I resolved in that area. I does look to be handled, I'd also vote for just pulling from restler at this point instead of our own fork.

@zbirkenbuel
Copy link
Author

@jstamerj - it looks like they tried to fix it, but not in all of the scenarios we've encountered. Using just mainline restler we get an error in one of our Newton-Core tests where it tries to retry but adds a very small body where there should be none. Switching back to this branch seems to work just fine. This branch breaks some of restler's unit tests, but I can focus on that instead of more changes in Newton.

@jvaleske
Copy link

Looks good.

zbirkenbuel pushed a commit that referenced this pull request Mar 31, 2015
Pulling in upstream updates to restler
@zbirkenbuel zbirkenbuel merged commit 786ebb2 into master Mar 31, 2015
@zbirkenbuel zbirkenbuel deleted the zackb-massive branch March 31, 2015 17:33
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.