Skip to content
GitLab
    • Explore Projects Groups Snippets
Projects Groups Snippets
  • /
  • Help
    • Help
    • Support
    • Community forum
    • Submit feedback
    • Contribute to GitLab
  • Sign in / Register
  • N node-http-proxy
  • Project information
    • Project information
    • Activity
    • Labels
    • Members
  • Repository
    • Repository
    • Files
    • Commits
    • Branches
    • Tags
    • Contributors
    • Graph
    • Compare
  • Issues 482
    • Issues 482
    • List
    • Boards
    • Service Desk
    • Milestones
  • Merge requests 102
    • Merge requests 102
  • CI/CD
    • CI/CD
    • Pipelines
    • Jobs
    • Schedules
  • Deployments
    • Deployments
    • Environments
    • Releases
  • Packages and registries
    • Packages and registries
    • Package Registry
    • Infrastructure Registry
  • Monitor
    • Monitor
    • Incidents
  • Analytics
    • Analytics
    • Value stream
    • CI/CD
    • Repository
  • Wiki
    • Wiki
  • Snippets
    • Snippets
  • Activity
  • Graph
  • Create a new issue
  • Jobs
  • Commits
  • Issue Boards
Collapse sidebar
  • http ... PARTY!
  • node-http-proxy
  • Merge requests
  • !748

[bugfix] handle multi ? urls

  • Review changes

  • Download
  • Email patches
  • Plain diff
Merged Administrator requested to merge github/fork/samccone/sjs/fix-common into master 10 years ago
  • Overview 10
  • Commits 1
  • Pipelines 0
  • Changes 2

Created by: samccone

Checking approval status

Merged by (Jul 23, 2025 8:33am UTC)

Loading

Activity


  • Administrator
    Administrator @root · 10 years ago
    Author Contributor

    Created by: samccone

    Composite PR of

    #744 #746

  • Administrator changed title from Update common.js to [bugfix] handle multi ? urls 10 years ago

    changed title from Update common.js to [bugfix] handle multi ? urls

  • Administrator
    Administrator @root started a thread on commit 70ed1c42 10 years ago
    lib/http-proxy/common.js
    155 155
    156 156 // Only join the query string if it exists so we don't have trailing a '?'
    157 157 // on every request
    158 lastSegs[1] && retSegs.push(lastSegs[1]);
    158
    159 // Handle case where there could be multiple ? in the URL.
    160 retSegs.concat(lastSegs);
    • Administrator
      Administrator @root · 10 years ago
      Author Contributor

      Created by: jcrugzz

      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.

  • Administrator
    Administrator @root started a thread on commit 70ed1c42 10 years ago
    test/lib-http-proxy-common-test.js
    203 203 expect(outgoing.path).to.eql('/forward/static/path');
    204 204 })
    205 205
    206 it('should not modify the query string', function () {
    206 it.only('should not modify the query string', function () {
    • Administrator
      Administrator @root · 10 years ago
      Author Contributor

      Created by: jcrugzz

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

  • Administrator
    Administrator @root started a thread on commit 70ed1c42 10 years ago
    test/lib-http-proxy-common-test.js
    203 203 expect(outgoing.path).to.eql('/forward/static/path');
    204 204 })
    205 205
    206 it('should not modify the query string', function () {
    206 it.only('should not modify the query string', function () {
    • Administrator
      Administrator @root · 10 years ago
      Author Contributor

      Created by: samccone

      ooops

  • Administrator
    Administrator @root started a thread on commit 70ed1c42 10 years ago
    lib/http-proxy/common.js
    155 155
    156 156 // Only join the query string if it exists so we don't have trailing a '?'
    157 157 // on every request
    158 lastSegs[1] && retSegs.push(lastSegs[1]);
    158
    159 // Handle case where there could be multiple ? in the URL.
    160 retSegs.concat(lastSegs);
    • Administrator
      Administrator @root · 10 years ago
      Author Contributor

      Created by: koolc

      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('?')
  • Administrator mentioned in merge request !744 (closed) 10 years ago

    mentioned in merge request !744 (closed)

  • Administrator
    Administrator @root started a thread on commit 70ed1c42 10 years ago
    lib/http-proxy/common.js
    155 155
    156 156 // Only join the query string if it exists so we don't have trailing a '?'
    157 157 // on every request
    158 lastSegs[1] && retSegs.push(lastSegs[1]);
    158
    159 // Handle case where there could be multiple ? in the URL.
    160 retSegs.concat(lastSegs);
    • Administrator
      Administrator @root · 10 years ago
      Author Contributor

      Created by: jcrugzz

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

      because it doesnt create a new array

  • Administrator
    Administrator @root started a thread on commit 70ed1c42 10 years ago
    lib/http-proxy/common.js
    155 155
    156 156 // Only join the query string if it exists so we don't have trailing a '?'
    157 157 // on every request
    158 lastSegs[1] && retSegs.push(lastSegs[1]);
    158
    159 // Handle case where there could be multiple ? in the URL.
    160 retSegs.concat(lastSegs);
    • Administrator
      Administrator @root · 10 years ago
      Author Contributor

      Created by: koolc

      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?

  • Administrator
    Administrator @root started a thread on commit 70ed1c42 10 years ago
    lib/http-proxy/common.js
    155 155
    156 156 // Only join the query string if it exists so we don't have trailing a '?'
    157 157 // on every request
    158 lastSegs[1] && retSegs.push(lastSegs[1]);
    158
    159 // Handle case where there could be multiple ? in the URL.
    160 retSegs.concat(lastSegs);
    • Administrator
      Administrator @root · 10 years ago
      Author Contributor

      Created by: jcrugzz

      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.

  • Administrator closed 10 years ago

    closed

  • Administrator
    Administrator @root · 10 years ago
    Author Contributor

    Created by: samccone

    ah sorry for the delay @jcrugzz I was traveling for the last 24 hours but it looks like this was resolved :thumbsup:

  • Administrator
    Administrator @root started a thread on commit 70ed1c42 10 years ago
    lib/http-proxy/common.js
    155 155
    156 156 // Only join the query string if it exists so we don't have trailing a '?'
    157 157 // on every request
    158 lastSegs[1] && retSegs.push(lastSegs[1]);
    158
    159 // Handle case where there could be multiple ? in the URL.
    160 retSegs.concat(lastSegs);
    • Administrator
      Administrator @root · 10 years ago
      Author Contributor

      Created by: koolc

      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! ^-^!

Please register or sign in to reply
0 Assignees
None
Assign to
0 Reviewers
None
Request review from
Labels
0
None
0
None
    Assign labels
  • Manage project labels

Milestone
No milestone
None
None
Time tracking
No estimate or time spent
Lock merge request
Unlocked
1
1 participant
Administrator
Reference: http-party/node-http-proxy!748
Source branch: github/fork/samccone/sjs/fix-common

Menu

Explore Projects Groups Snippets