src: fix ThrowInvalidURL call in PathToFileURL

[danielmbrasil]

Fixes #57133.

Replaces nullptr with std::nullopt when calling ThrowInvalidURL in PathToFileURL. This ensures the function receives the correct argument type and aligns with the expected behavior.

PS: I'm not sure how to test this, any help would be appreciated.

[nodejs-github-bot]

Review requested:

• @nodejs/url

[nodejs-github-bot]

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

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 89.03%. Comparing base (f6ce486) to head (12b9054).
Report is 59 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_url.cc	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #57141      +/-   ##
==========================================
+ Coverage   88.54%   89.03%   +0.49%     
==========================================
  Files         665      665              
  Lines      193408   193408              
  Branches    36961    37278     +317     
==========================================
+ Hits       171250   172205     +955     
+ Misses      14795    13884     -911     
+ Partials     7363     7319      -44     

Files with missing lines	Coverage Δ
src/node_url.cc	79.53% <0.00%> (+0.29%)	⬆️

... and 93 files with indirect coverage changes

[nodejs-github-bot]

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

[jazelly]

should we also add a test?

[lemire]

Do we have a test with a malformed URL?

[juanarbol]

should we also add a test?

I would say so.

[juanarbol]

LGTM once the test case is added. Thanks!

[geeksilva97]

Do you guys have any clues about how to test this? I tried to reach such a condition, but I couldn't.

[anonrig]

Do you guys have any clues about how to test this? I tried to reach such a condition, but I couldn't.

Adding a CC_TEST would be the fastest

[danielmbrasil]

Adding a CC_TEST would be the fastest

@anonrig Do you have any examples of how to test this? As this is my first contribution I'm a bit lost on how to write a CC_TEST.

[lemire]

Let me elaborate on my comment above where I wrote "Do we have a test with a malformed URL?".

Here is the code:

  Utf8Value input(isolate, args[0]);
  auto input_str = input.ToStringView();
  CHECK(!input_str.empty());
  auto out =
       ada::parse<ada::url_aggregator>(EncodePathChars(input_str, os), nullptr);

   if (!out) {
     return ThrowInvalidURL(realm->env(), input.ToStringView(), somethinghere);
   }

The question is whether we have a test that triggers the if(!out) check.

[lemire]

@juanarbol @anonrig @jazelly @geeksilva97

Would you object to this fix getting merged without a test. I think we have no test that would trigger this issue, but by reading the code, what I expect happens when the problematic code path is triggered... is that you get a hard crash.

This is a confirmed bug, I think that there is no disagreement about that. The fix looks correct to all of us. I think we should not block the fix due to a lack of good test.

[geeksilva97]

@juanarbol @anonrig @jazelly @geeksilva97

Would you object to this fix getting merged without a test. I think we have no test that would trigger this issue, but by reading the code, what I expect happens when the problematic code path is triggered... is that you get a hard crash.

This is a confirmed bug, I think that there is no disagreement about that. The fix looks correct to all of us. I think we should not block the fix due to a lack of good test.

No objection from my side.

[nodejs-github-bot]

Landed in fd45383