url: improve `isURL` detection by anonrig · Pull Request #47886 · nodejs/node

anonrig · 2023-05-05T19:39:30Z

Previously we were checking for protocol and origin attributes, but due to the lazy-loading nature of Ada 2.0 with url_aggregator we shifted from checking for origin. This was also breaking the detection of the legacy URL parse function. This PR fixes that in the least impactful way possible, by checking if auth property is defined.

Fixes #47624

cc @nodejs/url @Trott

nodejs-github-bot · 2023-05-05T19:39:34Z

Review requested:

@nodejs/url

lib/internal/url.js

nodejs-github-bot · 2023-05-05T20:32:30Z

CI: https://ci.nodejs.org/job/node-test-pull-request/51671/

nodejs-github-bot · 2023-05-05T23:26:14Z

CI: https://ci.nodejs.org/job/node-test-pull-request/51672/

nodejs-github-bot · 2023-05-06T00:20:49Z

CI: https://ci.nodejs.org/job/node-test-pull-request/51673/

Trott · 2023-05-06T03:32:09Z

Can we add a test that checks without --expose-internals? Something like this would work, I think:

'use strict';

const common = require('../common');
if (!common.hasCrypto) { common.skip('missing crypto'); }
const assert = require('assert');

const opt = require('url').parse('https://httpbin.org/get');
require('https')
  .request(
    { ...opt, path: opt.path + '?A=B' },
    (s) => s.once('data',
                (d) => assert.ok(d.toString('UTF8').includes('https://httpbin.org/get?A=B'))))
  .end();

This will test the behavior the user experiences rather than internal implementation details.

It can probably be improved (e.g., to aggregate the data chunks rather than assume it will all be in the first chunk), but you get the idea.

anonrig · 2023-05-06T03:36:36Z

Of course, @Trott can you block the pull request to avoid accidentaly merging this pr?

Trott · 2023-05-06T03:37:39Z

(Removing commit queue label pending "do we need test that checks the public API/behavior" resolution.)

Trott · 2023-05-06T03:37:57Z

(Removing commit queue label pending "do we need test that checks the public API/behavior" resolution.)

And now putting it back because we can always add the test in a subsequent PR.

VoltrexKeyva · 2023-05-06T04:44:26Z

lib/internal/url.js

 * We use `href` and `protocol` as they are the only properties that are
 * easy to retrieve and calculate due to the lazy nature of the getters.
+ *
+ * We check for auth attribute to distinguish legacy url instance with


Suggested change

* We check for auth attribute to distinguish legacy url instance with

* We check for auth attribute to distinguish legacy URL instance with

For consistency.

URL is the name of the WHATWG URL object. Perhaps to avoid confusion, it should be urlObject here to be consistent with https://nodejs.org/dist/latest-v20.x/docs/api/url.html#legacy-urlobject?

Suggested change

* We check for auth attribute to distinguish legacy url instance with

* We check for auth attribute to distinguish legacy urlObject instance with

Trott

Needs a public API test.

Trott · 2023-05-06T13:51:47Z

Of course, @Trott can you block the pull request to avoid accidentaly merging this pr?

Sure. Done.

Trott · 2023-05-06T14:13:55Z

My test suggestion is not great as it depends on the behavior of an external service. I'm going to unblock this. We can always add another test later.

nodejs-github-bot · 2023-05-07T19:41:23Z

Landed in 528aaca

PR-URL: #47886 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Matthew Aitken <maitken033380023@gmail.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

PR-URL: nodejs#47886 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Matthew Aitken <maitken033380023@gmail.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

PR-URL: #47886 Backport-PR-URL: #50105 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Matthew Aitken <maitken033380023@gmail.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

PR-URL: nodejs/node#47886 Backport-PR-URL: nodejs/node#50105 Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Matthew Aitken <maitken033380023@gmail.com> Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

nodejs-github-bot added needs-ci PRs that need a full CI run. whatwg-url Issues and PRs related to the WHATWG URL implementation. labels May 5, 2023

anonrig commented May 5, 2023

View reviewed changes

lib/internal/url.js Outdated Show resolved Hide resolved

anonrig requested a review from TimothyGu May 5, 2023 19:51

Trott approved these changes May 5, 2023

View reviewed changes

anonrig added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels May 5, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 5, 2023

url: improve isURL detection

c5725a1

anonrig force-pushed the url-fix-detection branch from 4a0b183 to c5725a1 Compare May 5, 2023 20:52

KhafraDev approved these changes May 5, 2023

View reviewed changes

anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label May 5, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 5, 2023

github-actions bot mentioned this pull request May 6, 2023

CI Reliability 2023-05-06 nodejs/reliability#557

Open

22 tasks

anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label May 6, 2023

Trott removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 6, 2023

Trott added the commit-queue Add this label to land a pull request using GitHub Actions. label May 6, 2023

VoltrexKeyva approved these changes May 6, 2023

View reviewed changes

Trott requested changes May 6, 2023

View reviewed changes

Trott removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 6, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 7, 2023

nodejs-github-bot merged commit 528aaca into main May 7, 2023

nodejs-github-bot deleted the url-fix-detection branch May 7, 2023 19:41

github-actions bot mentioned this pull request May 13, 2023

CI Reliability 2023-05-13 nodejs/reliability#564

Open

20 tasks

targos mentioned this pull request May 15, 2023

v20.2.0 release proposal #48020

Merged

anonrig mentioned this pull request Jun 5, 2023

[v18.x] backport Ada v2.x to Node.js v18 #48345

Closed

19 tasks

anonrig added the backport-open-v18.x label Jun 5, 2023

Zhangdroid mentioned this pull request Jul 26, 2023

url: fix isURL detection by checking path #48928

Closed

aduh95 mentioned this pull request Oct 9, 2023

[v18.x backport] url test #50105

Closed

targos mentioned this pull request Nov 28, 2023

v18.19.0 release proposal #50953

Merged

richardlau added backported-to-v18.x and removed backport-open-v18.x labels Mar 18, 2024

	* We check for auth attribute to distinguish legacy url instance with
	* We check for auth attribute to distinguish legacy URL instance with

Uh oh!

Conversation

anonrig commented May 5, 2023

Uh oh!

nodejs-github-bot commented May 5, 2023

Uh oh!

Uh oh!

nodejs-github-bot commented May 5, 2023

Uh oh!

nodejs-github-bot commented May 5, 2023

Uh oh!

nodejs-github-bot commented May 6, 2023

Uh oh!

Trott commented May 6, 2023

Uh oh!

anonrig commented May 6, 2023

Uh oh!

Trott commented May 6, 2023

Uh oh!

Trott commented May 6, 2023

Uh oh!

VoltrexKeyva May 6, 2023

Choose a reason for hiding this comment

Uh oh!

Trott May 6, 2023

Choose a reason for hiding this comment

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

Trott commented May 6, 2023

Uh oh!

Trott commented May 6, 2023

Uh oh!

nodejs-github-bot commented May 7, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants