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: encode null character at the end of file path #54066

Closed
wants to merge 1 commit into from

Conversation

EarlyRiser42
Copy link
Contributor

@EarlyRiser42 EarlyRiser42 commented Jul 27, 2024

Fixes: #51167

Description

To maintain current performance while addressing the whitespace trimming issue, I opted to pre-encode the pathname. Modifying the URL parse function would require more extensive changes, so pre-encoding is a simpler solution.

Test

import { pathToFileURL } from 'url';

const filename = `test-${String.fromCharCode(0)}`;
console.log(pathToFileURL(filename).toString()); // file:///[...]/test-%00

Previously, this would output test-, but now it correctly outputs file:///[...]/test-%00.

Benchmark

                                                                                                                 confidence improvement accuracy (*)   (**)  (***)
url/whatwg-url-to-and-from-path.js n=5000000 method='fileURLToPath' input='file:///dev/null?key=param&bool'                      0.53 %       ±0.89% ±1.20% ±1.58%
url/whatwg-url-to-and-from-path.js n=5000000 method='fileURLToPath' input='file:///dev/null?key=param&bool#hash'                -0.23 %       ±0.86% ±1.16% ±1.53%
url/whatwg-url-to-and-from-path.js n=5000000 method='fileURLToPath' input='file:///dev/null'                                    -0.56 %       ±1.33% ±1.76% ±2.30%
url/whatwg-url-to-and-from-path.js n=5000000 method='pathToFileURL' input='file:///dev/null?key=param&bool'                      0.42 %       ±1.12% ±1.50% ±1.98%
url/whatwg-url-to-and-from-path.js n=5000000 method='pathToFileURL' input='file:///dev/null?key=param&bool#hash'         **     -0.87 %       ±0.60% ±0.79% ±1.03%
url/whatwg-url-to-and-from-path.js n=5000000 method='pathToFileURL' input='file:///dev/null'                                    -0.50 %       ±1.50% ±1.99% ±2.60%

Be aware that when doing many comparisons the risk of a false-positive result increases.
In this case, there are 6 comparisons, you can thus expect the following amount of false-positive results:
  0.30 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.06 false positives, when considering a   1% risk acceptance (**, ***),
  0.01 false positives, when considering a 0.1% risk acceptance (***)

The local benchmark doesn't show a statistically significant performance degradation

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/url

@nodejs-github-bot 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 Jul 27, 2024
@EarlyRiser42
Copy link
Contributor Author

@RedYetiDev
Hello sir,

If you don't mind, could you please review my PR? I saw that you have reviewed PRs associated with URL files before, so I thought you might be able to help. If this is a mistake, I apologize for the inconvenience.

@RedYetiDev
Copy link
Member

Hi! I'm a member of the triage team, so I help to perform basic reviews and labelings of issues and PRs. I don't know too much of the context of null character encoding, but seeing as no tests fail, and little perf impact, this does look good to me. Keep in mind that my opinion doesn't hold much "weight" in the actual review process.

@EarlyRiser42
Copy link
Contributor Author

Thank you for your kind review, sir. I don't want to bother the Node.js team, but it's been more than a week since I submitted this PR, and I was concerned it might have been overlooked. That's why I mentioned you. I will wait a bit longer and then find someone who can approve my PR. Thank you so much for your attention and help. I truly appreciate it.

@RedYetiDev RedYetiDev added the url Issues and PRs related to the legacy built-in url module. label Aug 5, 2024
@EarlyRiser42 EarlyRiser42 deleted the issue-51167 branch August 21, 2024 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run. url Issues and PRs related to the legacy built-in url module. whatwg-url Issues and PRs related to the WHATWG URL implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

v18.18.2 -> v18.19.0, v20.5.1 -> v20.6.0 regression: pathToFileURL trims whitespace.
3 participants