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

fix: Don't mutate passed URL object #470

Merged
merged 4 commits into from
Sep 21, 2023
Merged

Conversation

tremby
Copy link
Contributor

@tremby tremby commented Sep 18, 2023

Before this change, a passed URL object would be mutated if query parameters were appended via the data option.

Now we clone the object instead, so the original is not affected.


Background:

I am passing URL objects to the request method, and then adding query parameters via the data option. Example:

import { request } from "urllib";

const url = new URL("http://example.com");

await request(url, { data: { param1: "value1" } });

I found that if I then make another request to the same URL but with different parameters:

await request(url, { data: { param2: "value2" } });

...a request is actually made to http://example.com?param1=value1&param2=value2.

My URL object is being mutated by the urllib library. I see this as a bug.

Before this change, a passed URL object would be mutated if query parameters were appended via the `data` option.

Now we clone the object instead, so the original is not affected.
@fengmk2 fengmk2 added the bug Something isn't working label Sep 19, 2023
@fengmk2 fengmk2 self-assigned this Sep 19, 2023
@fengmk2 fengmk2 changed the title Don't mutate passed URL object fix: Don't mutate passed URL object Sep 19, 2023
@codecov
Copy link

codecov bot commented Sep 19, 2023

Codecov Report

Merging #470 (5f043e8) into master (8c4ec6c) will increase coverage by 0.00%.
Report is 4 commits behind head on master.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #470   +/-   ##
=======================================
  Coverage   99.64%   99.64%           
=======================================
  Files           8        9    +1     
  Lines        1411     1423   +12     
  Branches      253      253           
=======================================
+ Hits         1406     1418   +12     
  Misses          5        5           
Files Changed Coverage Δ
src/HttpClient.ts 99.57% <100.00%> (+<0.01%) ⬆️

... and 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@@ -201,7 +201,8 @@ export class HttpClient extends EventEmitter {
// url maybe url.parse(url) object in urllib2
requestUrl = new URL(urlFormat(url));
} else {
requestUrl = url;
// Don't mutate the URL object the user passed in
requestUrl = new URL(url);
Copy link
Member

Choose a reason for hiding this comment

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

should change url to a string before pass to URL

requestUrl = new URL(url.toString());

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The URL constructor does that automatically; it's perfectly happy being passed a URL object or anything else with a toString method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, weird, the node:url URL type wants a string, but the whatwg URL type (in browsers and with types provided by the dom module of typescript) is happy to take another URL. I'll add the .toString(), and a test.

}
// url maybe url.parse(url) object in urllib2
// or even if not, we clone to avoid mutating it
requestUrl = new URL(urlFormat(url));
Copy link
Member

@fengmk2 fengmk2 Sep 20, 2023

Choose a reason for hiding this comment

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

https://nodejs.org/dist/latest-v18.x/docs/api/url.html#urlformaturlobject
urlFormat is only for the url.parse() return object.

If the url is a URL instance, I think we should use new URL(url.toString()) is better.

And the urlformat() is Legacy, it should not be used for non-compatibility issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see

@fengmk2 fengmk2 merged commit 5b8867f into node-modules:master Sep 21, 2023
22 checks passed
@fengmk2
Copy link
Member

fengmk2 commented Sep 21, 2023

@tremby Thanks for your patient contribution!

fengmk2 pushed a commit that referenced this pull request Sep 21, 2023
[skip ci]

## [3.19.3](v3.19.2...v3.19.3) (2023-09-21)

### Bug Fixes

* Don't mutate passed URL object ([#470](#470)) ([5b8867f](5b8867f))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants