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

url: introduce URL_FLAGS_IS_DEFAULT_SCHEME_PORT flag #20479

Closed
wants to merge 6 commits into from

Conversation

AyushG3112
Copy link
Contributor

@AyushG3112 AyushG3112 commented May 2, 2018

Introduce URL_FLAGS_IS_DEFAULT_SCHEME_PORT flag which is retured
when the parser detects that the port passed is the default port
for that scheme.

Fixes: #20465

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

Sorry, something went wrong.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels May 2, 2018
@AyushG3112
Copy link
Contributor Author

If anyone could suggest where to add tests, that would be great. The fixture used in the existing test is the one provided by W3C and I'm not sure if I should modify that.

src/node_url.cc Outdated
@@ -1748,6 +1755,9 @@ void URL::Parse(const char* input,
return;
}
// the port is valid
if (IsDefaultSchemePort(url->scheme, static_cast<int>(port))) {
url->flags |= URL_FLAGS_IS_DEFAULT_SCHEME_PORT;
}
url->port = NormalizePort(url->scheme, static_cast<int>(port));
Copy link
Member

Choose a reason for hiding this comment

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

The job done by IsDefaultSchemePort is pretty similar to what NormalizePort does already. Can we instead check url->port == -1 as an indicator for default scheme port?

@@ -270,13 +271,17 @@ function onParseHostnameComplete(flags, protocol, username, password,

function onParsePortComplete(flags, protocol, username, password,
host, port, path, query, fragment) {
this[context].port = port;
if ((flags & URL_FLAGS_IS_DEFAULT_SCHEME_PORT) !== 0) {
this[context].port = null;
Copy link
Member

Choose a reason for hiding this comment

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

In the case where URL_FLAGS_IS_DEFAULT_SCHEME_PORT is set, port should already be null. Unless I'm missing something there doesn't seem to be a need to have a conditional here.

Copy link
Contributor Author

@AyushG3112 AyushG3112 May 3, 2018

Choose a reason for hiding this comment

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

The reason I added the conditional is because the spec explicitly mentions setting the port to null in case of default scheme port.

I can remove it if you think we can just rely on C++ land to send us null in that situation though.

@TimothyGu
Copy link
Member

This PR also lacks tests. Please check out https://github.com/nodejs/node/blob/master/test/fixtures/url-setter-tests.js, add a test in there, and then upstream that test to https://github.com/w3c/web-platform-tests/blob/master/url/setters_tests.json.

@AyushG3112
Copy link
Contributor Author

@TimothyGu Fixed. PTAL. Also, to upstream the test, do I just open a PR for https://github.com/w3c/web-platform-tests/blob/master/url/setters_tests.json, or is there any other mechanism?

@AyushG3112
Copy link
Contributor Author

/cc @nodejs/url PTAL

@AyushG3112
Copy link
Contributor Author

AyushG3112 commented May 7, 2018

Ping

@joyeecheung
Copy link
Member

Also, to upstream the test, do I just open a PR for https://github.com/w3c/web-platform-tests/blob/master/url/setters_tests.json, or is there any other mechanism?

@AyushG3112 Yes I think so.

@AyushG3112
Copy link
Contributor Author

Test has landed at upstream W3C via web-platform-tests/wpt#10892, and I updated the ordering in test/fixtures/url-setter-tests.js to match the upstream contents.

@AyushG3112
Copy link
Contributor Author

@jasnell could you please run the CI on this? Also, would this be semver major or patch? Because this fixes a bug but changes the output of an action.

@jasnell
Copy link
Member

jasnell commented May 9, 2018

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

It's semver-patch :-)

@AyushG3112
Copy link
Contributor Author

Do any of the failures look related?

@targos
Copy link
Member

targos commented May 10, 2018

Failures look unrelated. New try: https://ci.nodejs.org/job/node-test-pull-request/14778/

@AyushG3112
Copy link
Contributor Author

Looks like builds timed out this time. Infra issue?

@AyushG3112
Copy link
Contributor Author

AyushG3112 commented May 11, 2018

Could anyone run the CI on this again please? thanks!

@jasnell
Copy link
Member

jasnell commented May 12, 2018

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request May 14, 2018
…d if new port is scheme default, a=testonly

Automatic update from web-platform-testsURL: host setter with default port against URL with non-default port

See nodejs/node#20479.
--

wpt-commits: f0fe4791f5b87491d8d9662832fae543e4edbca1
wpt-pr: 10892
Introduce `URL_FLAGS_IS_DEFAULT_SCHEME_PORT` flag which is retured
when the parser detects that the port passed is the default port
for that scheme.

Fixes: nodejs#20465
@AyushG3112 AyushG3112 force-pushed the fix-url-port-host-flag branch from 92051fa to 70d5dc2 Compare May 15, 2018 04:06
@AyushG3112
Copy link
Contributor Author

Rebased to master in hope of CI passing. Can this have another CI run please?

src/node_url.cc Outdated
@@ -1749,6 +1749,9 @@ void URL::Parse(const char* input,
}
// the port is valid
url->port = NormalizePort(url->scheme, static_cast<int>(port));
if (url->port == -1) {
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer no curlies, it's the more common style in our C++ code.

src/node_url.h Outdated
@@ -50,7 +50,8 @@ using v8::Value;
XX(URL_FLAGS_HAS_HOST, 0x80) \
XX(URL_FLAGS_HAS_PATH, 0x100) \
XX(URL_FLAGS_HAS_QUERY, 0x200) \
XX(URL_FLAGS_HAS_FRAGMENT, 0x400)
XX(URL_FLAGS_HAS_FRAGMENT, 0x400) \
XX(URL_FLAGS_IS_DEFAULT_SCHEME_PORT, 0x800)
Copy link
Member

Choose a reason for hiding this comment

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

Can we add the final \ if it doesn't break this. It makes diffs cleaner.

@apapirovski
Copy link
Member

@AyushG3112
Copy link
Contributor Author

@apapirovski PTAL

@apapirovski
Copy link
Member

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

then we land this.

Copy link
Member

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

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

Reaffirming.

@AyushG3112
Copy link
Contributor Author

parallel/test-http2-compat-client-upload-reject failed with ECONNRESET on a few platforms, is it related by any chance?

@apapirovski
Copy link
Member

@AyushG3112 That one is definitely unrelated. We have a bug in http2 at the moment. (As an aside, I'm working on changes to http2 that will get rid of those failures.)

@AyushG3112
Copy link
Contributor Author

@apapirovski what would the next step here be then? Should we wait for the fix to land before landing this, or run the CI on the failed platforms till it is green, or ,assuming parallel/test-http2-compat-client-upload-reject is the only failure, ignore it?

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 18, 2018
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request May 18, 2018

Verified

This commit was signed with the committer’s verified signature. The key has expired.
BridgeAR Ruben Bridgewater
Introduce `URL_FLAGS_IS_DEFAULT_SCHEME_PORT` flag which is retured
when the parser detects that the port passed is the default port
for that scheme.

PR-URL: nodejs#20479
Fixes: nodejs#20465
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@BridgeAR
Copy link
Member

Landed in 11892b0 🎉

@BridgeAR BridgeAR closed this May 18, 2018
@AyushG3112 AyushG3112 deleted the fix-url-port-host-flag branch May 20, 2018 09:09
MylesBorins pushed a commit that referenced this pull request May 22, 2018

Unverified

The email in this signature doesn’t match the committer email.
Introduce `URL_FLAGS_IS_DEFAULT_SCHEME_PORT` flag which is retured
when the parser detects that the port passed is the default port
for that scheme.

PR-URL: #20479
Fixes: #20465
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@addaleax addaleax mentioned this pull request May 22, 2018
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 3, 2019
…d if new port is scheme default, a=testonly

Automatic update from web-platform-testsURL: host setter with default port against URL with non-default port

See nodejs/node#20479.
--

wpt-commits: f0fe4791f5b87491d8d9662832fae543e4edbca1
wpt-pr: 10892

UltraBlame original commit: 497bd5c8c632f4828e13d82f8c7f0f6d7553126c
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 3, 2019
…d if new port is scheme default, a=testonly

Automatic update from web-platform-testsURL: host setter with default port against URL with non-default port

See nodejs/node#20479.
--

wpt-commits: f0fe4791f5b87491d8d9662832fae543e4edbca1
wpt-pr: 10892

UltraBlame original commit: 497bd5c8c632f4828e13d82f8c7f0f6d7553126c
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 3, 2019
…d if new port is scheme default, a=testonly

Automatic update from web-platform-testsURL: host setter with default port against URL with non-default port

See nodejs/node#20479.
--

wpt-commits: f0fe4791f5b87491d8d9662832fae543e4edbca1
wpt-pr: 10892

UltraBlame original commit: 497bd5c8c632f4828e13d82f8c7f0f6d7553126c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

assigning a hostname with port 80 to URL.host will not override the existing port
8 participants