Skip to content

Url::port_int implementation #707

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

Closed
wants to merge 1 commit into from
Closed

Conversation

daxpedda
Copy link

Some questions arose up while I was doing this implementation.

  • I implemented this by adding a field port_int to Url, my initial implementation actually handled it by storing the actual port always in Url::port and when using the method Url::port() it would filter based on scheme defaults. In addition the Url::serialization would always contain the correctly removed default port. This implementation caused a lot of edge cases to arise, so I avoided it by doing it in a separate field.
  • Currently when changing a scheme with Url::set_scheme, Url::port_int is removed if it was the scheme default, see Changing the scheme can break things #61. Probably it would be more intuitive that if Url::port_int is empty, ergo the user didn't define a port (default or not), then having the new port be the default port for that scheme makes sense. But if the user deliberately defined the default port on the URL, changing the scheme shouldn't change the port.
    // this makes sense
    let mut url = Url::parse("http://example.net").unwrap();
    url.set_scheme("https").unwrap();
    assert_eq!(url.port(), None);
    assert_eq!(url.port_int(), None);
    assert_eq!(url.port_or_known_default(), Some(443));
    
    // this is not intuitive
    let mut url = Url::parse("http://example.net:80").unwrap();
    assert_eq!(url.port(), None);
    assert_eq!(url.port_int(), Some(80));
    assert_eq!(url.port_or_known_default(), Some(80));
    url.set_scheme("https").unwrap();
    assert_eq!(url.port(), None);
    assert_eq!(url.port_int(), None); // why does this vanish, probably expected to stay 80
    assert_eq!(url.port_or_known_default(), Some(443)); // probably expected to stay 80
    
    // even weirder, but you shouldn't use a assigned port to a custom protocol anyway
    let mut url = Url::parse("custom-protocol://example.net:80").unwrap();
    assert_eq!(url.port(), Some(80));
    assert_eq!(url.port_int(), Some(80));
    assert_eq!(url.port_or_known_default(), Some(80));
    url.set_scheme("http").unwrap();
    assert_eq!(url.port(), None);
    assert_eq!(url.port_int(), None);
    assert_eq!(url.port_or_known_default(), Some(80));
    url.set_scheme("https").unwrap();
    assert_eq!(url.port(), None);
    assert_eq!(url.port_int(), None);
    assert_eq!(url.port_or_known_default(), Some(443)); // port changed completely, I guess this is potentially an issue with the current implementation too
    I don't think this is a big deal, I would argue that this is a pretty bad use-case the url crate might not want to support.
  • Comparing two Urls with different Url::port_int might turn out to be equal, because it compares Url::serialization, and default ports for scheme aren't reflected in the serialization.

Personally I'm fine with last two points, I consider them edge-cases that are fine for the url crate to ignore.

Fixes #706.

@@ -182,6 +182,7 @@ pub struct Url {
host_end: u32,
host: HostInternal,
port: Option<u16>,
port_int: Option<u16>,
Copy link
Author

Choose a reason for hiding this comment

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

Potentially I could make an enum like this:

enum Port {
    None,
    Default(u16),
    Custom(u16),
}

@@ -947,12 +957,13 @@ impl<'a> Parser<'a> {
Ok((username_end, remaining))
}

#[allow(clippy::type_complexity)]
Copy link
Author

Choose a reason for hiding this comment

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

If use the enum from above I could potentially pass it down from here, reducing the complexity.

@@ -141,7 +141,7 @@ pub fn set_host(url: &mut Url, new_host: &str) -> Result<(), ()> {
if host == Host::Domain("".to_string()) {
if !username(&url).is_empty() {
return Err(());
} else if let Some(Some(_)) = opt_port {
} else if let Some((_, Some(_))) = opt_port {
Copy link
Author

Choose a reason for hiding this comment

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

This might be considered a breaking change? I consider checking for user input regardless of defaults "more" correct, this might have been considered a bug until now.
This can easily be removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Method to get port regardless of defaults
2 participants