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.

Sign up for GitHub

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

[bugfix] handle multi ? urls #748

Merged
merged 1 commit into from
Dec 2, 2014
Merged

[bugfix] handle multi ? urls #748

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions lib/http-proxy/common.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ common.urlJoin = function() {
lastSegs = last.split('?'),
retSegs;

args[lastIndex] = lastSegs[0];
args[lastIndex] = lastSegs.shift();

//
// Join all strings, but remove empty strings so we don't get extra slashes from
Expand All @@ -155,7 +155,9 @@ common.urlJoin = function() {

// Only join the query string if it exists so we don't have trailing a '?'
// on every request
lastSegs[1] && retSegs.push(lastSegs[1]);

// Handle case where there could be multiple ? in the URL.
retSegs.concat(lastSegs);
Copy link
Contributor

Choose a reason for hiding this comment

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

use retSegs.push.apply(retSegs, lastSegs); as that modifies the retSegs array. Concat returns a new array and we want to prevent new array creation. This should also fix the test from failing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you confirm the code is correct? o?o

I think, the code retSegs.concat(lastSegs) doesn't seemingly change the array retSegs value when it doesn't assigned to retSegs like retSegs = retSegs.concat(lastSegs);, so wouldn't the retSegs value in the last statement return retSegs.join('?') be original? You can try by a multiple '?' urls.

The codes recommended:

    retSegs.push.apply(retSegs, lastSegs);
    return retSegs.join('?');

or

   return retSegs.concat(lastSegs).join('?')

Copy link
Contributor

Choose a reason for hiding this comment

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

retSegs.push.apply(retSegs, lastSegs);
return retSegs.join('?');

because it doesnt create a new array

Copy link
Contributor

Choose a reason for hiding this comment

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

To use 'concat' can create a new array indeed, but why to need a new array? And that, the new array isn't used by you to the codes after it. So, the variable retSegs value in return statement is still the old value, that is to say, the array length of the variable retSegs is still 1.

If you want to use 'concat' function, an assignment statement is required, like retSegs = retSegs.concat(lastSegs), because of the example below.

var a = [1, 2];
var b = a.concat([3,4]); // to assign the variable 'b'.
console.log(a, b)
// output: [1, 2]  [1, 2, 3, 4]

var c = a.join('-');     // ! This is your case: 'a' doesn't been changed and is still old value.
var d = b.join('-');     // ! This is my meaning: new elements have been added into 'b'.
console.log(c, d);
// output: 1-2  1-2-3-4

/* expected result: 1-2-3-4, instead of: 1-2 */

So, I think that the code retSegs.concat(lastSegs) should be changed to retSegs = retSegs.concat(lastSegs). Do you?

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand there is a bit of a language barrier here but the code should be...

retSegs.push.apply(retSegs, lastSegs);
return retSegs.join('?');

We DO NOT want to create a new array. I understand how concat works which is why I do not want to use it.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, I see now, and the codes in master are already right.
I just had doubts when I saw the codes in the branch http-proxy/common.js#L160 from #748 are below:

  // Handle case where there could be multiple ? in the URL.
  retSegs.concat(lastSegs);

  return retSegs.join('?')

So sorry! ^-^!


return retSegs.join('?')
};
6 changes: 3 additions & 3 deletions test/lib-http-proxy-common-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,13 +203,13 @@ describe('lib/http-proxy/common.js', function () {
expect(outgoing.path).to.eql('/forward/static/path');
})

it('should not modify the query string', function () {
it.only('should not modify the query string', function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be set to only so we run all the tests ;).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ooops

var outgoing = {};
common.setupOutgoing(outgoing, {
target: { path: '/forward' },
}, { url: '/?foo=bar//&target=http://foobar.com/' });
}, { url: '/?foo=bar//&target=http://foobar.com/?a=1%26b=2&other=2' });

expect(outgoing.path).to.eql('/forward/?foo=bar//&target=http://foobar.com/');
expect(outgoing.path).to.eql('/forward/?foo=bar//&target=http://foobar.com/?a=1%26b=2&other=2');
})
});

Expand Down