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

src: add a condition if the argument of DomainToUnicode is empty #49097

Merged
merged 1 commit into from
Aug 20, 2023

Conversation

pluris
Copy link
Contributor

@pluris pluris commented Aug 10, 2023

Add an empty check for the input, like DomainToASCII in the line below.

node/src/node_url.cc

Lines 79 to 82 in f1b3ade

std::string input = Utf8Value(env->isolate(), args[0]).ToString();
if (input.empty()) {
return args.GetReturnValue().Set(FIXED_ONE_BYTE_STRING(env->isolate(), ""));
}

Refs : #46410

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/url

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Aug 10, 2023
Copy link
Member

@anonrig anonrig left a comment

Choose a reason for hiding this comment

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

What's the goal of this pull request? Can you provide an example and update your PR with a test?

@pluris
Copy link
Contributor Author

pluris commented Aug 11, 2023

@anonrig Thank you for your comments.
I referred to the IDNA Mapping Table from the WhatWG URL.
I checked the handling of empty strings,
I saw that DomainToASCII and DomainToUnicode should be treated the same.
Therefore, in my opinion, it is in DomainToASCII, but it seems to be missing in DomainToUnicode, so I added it.

@anonrig
Copy link
Member

anonrig commented Aug 11, 2023

@pluris It already returns an empty string when an empty string is passed. It seems like this is an optimization for performance for the empty input to perform faster.

Welcome to Node.js v20.5.0.
Type ".help" for more information.
> require('url').domainToUnicode('')
''

Comment on lines +104 to +105
return args.GetReturnValue().Set(
String::NewFromUtf8(env->isolate(), "").ToLocalChecked());
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return args.GetReturnValue().Set(
String::NewFromUtf8(env->isolate(), "").ToLocalChecked());
return args.GetReturnValue().Set(FIXED_ONE_BYTE_STRING(env->isolate(), ""));

Copy link
Member

Choose a reason for hiding this comment

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

Can you apply the changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@anonrig Oh, I'm so sorry. I checked this comment too late. I thought I fixed it, but maybe I didn't add a commit...
It seems that the PR has been merged, but I will fix it.

@pluris
Copy link
Contributor Author

pluris commented Aug 11, 2023

@anonrig
If so, would it be better to do the same by removing the line below?

node/src/node_url.cc

Lines 79 to 82 in f1b3ade

std::string input = Utf8Value(env->isolate(), args[0]).ToString();
if (input.empty()) {
return args.GetReturnValue().Set(FIXED_ONE_BYTE_STRING(env->isolate(), ""));
}

@anonrig
Copy link
Member

anonrig commented Aug 11, 2023

@pluris I don't think removing it would make a difference since it is a single branch (in c++). So, personally I'm fine with adding it to domainToUnicode as well.

@pluris
Copy link
Contributor Author

pluris commented Aug 11, 2023

@anonrig Thank you for your opinion. It helped me a lot. 😄

@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 11, 2023
@pluris pluris closed this Aug 11, 2023
@pluris pluris reopened this Aug 11, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 11, 2023
@nodejs-github-bot
Copy link
Collaborator

@anonrig
Copy link
Member

anonrig commented Aug 11, 2023

@pluris Can you rebase from main and force-push?

@pluris pluris force-pushed the fix/check_empty_input branch from e2fd13c to 4e380fd Compare August 12, 2023 00:53
@pluris
Copy link
Contributor Author

pluris commented Aug 12, 2023

@anonrig Yes, I did rebase and force-push.

@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 12, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 12, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@pluris pluris force-pushed the fix/check_empty_input branch from 4e380fd to adf338b Compare August 18, 2023 04:18
@deokjinkim deokjinkim added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 18, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 18, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@deokjinkim deokjinkim added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Aug 20, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 20, 2023
@nodejs-github-bot nodejs-github-bot merged commit 45e5ec8 into nodejs:main Aug 20, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 45e5ec8

@pluris pluris deleted the fix/check_empty_input branch August 30, 2023 16:12
debadree25 pushed a commit that referenced this pull request Sep 1, 2023
PR-URL: #49336
Refs: #49097
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Debadree Chatterjee <[email protected]>
UlisesGascon pushed a commit that referenced this pull request Sep 10, 2023
PR-URL: #49097
Refs: #46410
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Deokjin Kim <[email protected]>
UlisesGascon pushed a commit that referenced this pull request Sep 10, 2023
PR-URL: #49336
Refs: #49097
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Debadree Chatterjee <[email protected]>
@UlisesGascon UlisesGascon mentioned this pull request Sep 10, 2023
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
PR-URL: nodejs#49336
Refs: nodejs#49097
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Debadree Chatterjee <[email protected]>
targos pushed a commit that referenced this pull request Nov 27, 2023
PR-URL: #49097
Refs: #46410
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Deokjin Kim <[email protected]>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
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++. needs-ci PRs that need a full CI run. whatwg-url Issues and PRs related to the WHATWG URL implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants