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

Bug: the parse::url::port() does not work on https url #4065

Closed
2 tasks done
LucasSovre opened this issue May 18, 2024 · 3 comments · Fixed by #4087
Closed
2 tasks done

Bug: the parse::url::port() does not work on https url #4065

LucasSovre opened this issue May 18, 2024 · 3 comments · Fixed by #4087
Labels
bug Something isn't working good first issue Good for newcomers topic:surrealql This is related to the SurrealQL query language

Comments

@LucasSovre
Copy link

Describe the bug

the function format does not find the port when url is https

Steps to reproduce

surreal version -> 1.2.1 for macos on aarch64

BEGIN TRANSACTION;
LET $https = "https://www.google.com:443/path?search=hello#world";
LET $http = "http://www.google.com:443/path?search=hello#world";

RETURN [
    parse::url::port($http),
    parse::url::port($https),
];

COMMIT TRANSACTION;

return :

[
{
"result": [
443,
null
],
"status": "OK",
"time": "20.458µs"
}
]

Expected behaviour

I expect the function to work with https

SurrealDB version

1.2.1 for macos on aarch64

Contact Details

lucas.sovre@proton.me

Is there an existing issue for this?

  • I have searched the existing issues

Code of Conduct

  • I agree to follow this project's Code of Conduct
@LucasSovre LucasSovre added bug Something isn't working triage This issue is new labels May 18, 2024
@LucasSovre LucasSovre changed the title Bug: Bug: the parse::url::port() does not work on https url May 18, 2024
@phughk phughk added good first issue Good for newcomers topic:surrealql This is related to the SurrealQL query language and removed triage This issue is new labels May 20, 2024
@phughk
Copy link
Contributor

phughk commented May 21, 2024

This is being worked on by @LivingLimes

@LivingLimes
Copy link
Contributor

Problem details

From what I can tell, parsing https urls will work. This issue arises from specifying the default port when using a special scheme such as https. If a valid port other than 443 is specified when using https, then this should function as expected. It follows that you will get a similar problem when trying to parse http://www.google.com:80.

We are currently using url crate, based on the url standard. The standard stipulates:

If url’s port is url’s scheme’s default port, then set url’s port to null.

which sounds like the issue we're facing here.

Next steps

I wouldn't call this a bug and I do not think we need to update any documentation or code in Surreal as the url crate conforms to an existing standard. Users who have this problem can find this issue to work around it.

@LucasSovre @phughk

@phughk
Copy link
Contributor

phughk commented May 23, 2024

Great work @LivingLimes thank you!
servo/rust-url#214
As per the ticket, the solution would be to rely on port_or_known_default instead of port. So it is still a bug and that would be the solution. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers topic:surrealql This is related to the SurrealQL query language
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants