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.

Sign up for GitHub

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

Url::port_int implementation #707

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 55 additions & 13 deletions url/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
}

path_start: u32, // Before initial '/', if any
query_start: Option<u32>, // Before '?', unlike Position::QueryStart
fragment_start: Option<u32>, // Before '#', unlike Position::FragmentStart
Expand Down Expand Up @@ -502,10 +503,9 @@ impl Url {
} else {
assert_eq!(self.byte_at(self.host_end), b':');
let port_str = self.slice(self.host_end + 1..self.path_start);
assert_eq!(
self.port,
Some(port_str.parse::<u16>().expect("Couldn't parse port?"))
);
let port = port_str.parse::<u16>().expect("Couldn't parse port?");
assert_eq!(self.port, Some(port));
assert_eq!(self.port_int, Some(port));
}
assert!(
self.path_start as usize == self.serialization.len()
Expand All @@ -518,6 +518,7 @@ impl Url {
assert_eq!(self.host_end, self.scheme_end + 1);
assert_eq!(self.host, HostInternal::None);
assert_eq!(self.port, None);
assert_eq!(self.port_int, None);
assert_eq!(self.path_start, self.scheme_end + 1);
}
if let Some(start) = self.query_start {
Expand Down Expand Up @@ -545,6 +546,7 @@ impl Url {
(self.host_str(), other.host_str()) == (None, Some(""))
);
assert_eq!(self.port, other.port);
assert!(self.port == None || self.port_int == other.port_int);
assert_eq!(self.path_start, other.path_start);
assert_eq!(self.query_start, other.query_start);
assert_eq!(self.fragment_start, other.fragment_start);
Expand Down Expand Up @@ -915,8 +917,9 @@ impl Url {

/// Return the port number for this URL, if any.
///
/// Note that default port numbers are never reflected by the serialization,
/// use the `port_or_known_default()` method if you want a default port number returned.
/// Note that default port numbers are never reflected in `port()`,
/// use the `port_or_known_default()` method if you want a default port number returned
/// or `port_int()` to get the port number regardless of defaults.
///
/// # Examples
///
Expand All @@ -942,6 +945,34 @@ impl Url {
self.port
}

/// Return the port number for this URL, if any.
///
/// Note that this always returns the port number regardless of defaults.
///
/// # Examples
///
/// ```
/// use url::Url;
/// # use url::ParseError;
///
/// # fn run() -> Result<(), ParseError> {
/// let url = Url::parse("https://example.com")?;
/// assert_eq!(url.port_int(), None);
///
/// let url = Url::parse("https://example.com:443/")?;
/// assert_eq!(url.port_int(), Some(443));
///
/// let url = Url::parse("ssh://example.com:22")?;
/// assert_eq!(url.port_int(), Some(22));
/// # Ok(())
/// # }
/// # run().unwrap();
/// ```
#[inline]
pub fn port_int(&self) -> Option<u16> {
self.port_int
}

/// Return the port number for this URL, or the default port number if it is known.
///
/// This method only knows the default port number
Expand Down Expand Up @@ -1460,8 +1491,6 @@ impl Url {

/// Change this URL’s port number.
///
/// Note that default port numbers are not reflected in the serialization.
///
/// If this URL is cannot-be-a-base, does not have a host, or has the `file` scheme;
/// do nothing and return `Err`.
///
Expand All @@ -1484,7 +1513,7 @@ impl Url {
/// # run().unwrap();
/// ```
///
/// Known default port numbers are not reflected:
/// Known default port numbers are not reflected in `port()` but in `port_int()`:
///
/// ```rust
/// use url::Url;
Expand All @@ -1495,6 +1524,7 @@ impl Url {
///
/// url.set_port(Some(443)).map_err(|_| "cannot be base")?;
/// assert!(url.port().is_none());
/// assert_eq!(url.port_int(), Some(443));
/// # Ok(())
/// # }
/// # run().unwrap();
Expand Down Expand Up @@ -1524,14 +1554,15 @@ impl Url {
if !self.has_host() || self.host() == Some(Host::Domain("")) || self.scheme() == "file" {
return Err(());
}
let port_int = port;
if port.is_some() && port == parser::default_port(self.scheme()) {
port = None
}
self.set_port_internal(port);
self.set_port_internal(port, port_int);
Ok(())
}

fn set_port_internal(&mut self, port: Option<u16>) {
fn set_port_internal(&mut self, port: Option<u16>, port_int: Option<u16>) {
match (self.port, port) {
(None, None) => {}
(Some(_), None) => {
Expand Down Expand Up @@ -1568,6 +1599,7 @@ impl Url {
}
}
self.port = port;
self.port_int = port_int;
}

/// Change this URL’s host.
Expand Down Expand Up @@ -1710,7 +1742,11 @@ impl Url {
}

/// opt_new_port: None means leave unchanged, Some(None) means remove any port number.
fn set_host_internal(&mut self, host: Host<String>, opt_new_port: Option<Option<u16>>) {
fn set_host_internal(
&mut self,
host: Host<String>,
opt_new_port: Option<(Option<u16>, Option<u16>)>,
) {
let old_suffix_pos = if opt_new_port.is_some() {
self.path_start
} else {
Expand All @@ -1730,8 +1766,9 @@ impl Url {
self.host_end = to_u32(self.serialization.len()).unwrap();
self.host = host.into();

if let Some(new_port) = opt_new_port {
if let Some((new_port, new_port_int)) = opt_new_port {
self.port = new_port;
self.port_int = new_port_int;
if let Some(port) = new_port {
write!(&mut self.serialization, ":{}", port).unwrap();
}
Expand Down Expand Up @@ -2177,6 +2214,7 @@ impl Url {
host_end,
host,
port: None,
port_int: None,
path_start: host_end,
query_start: None,
fragment_start: None,
Expand Down Expand Up @@ -2233,6 +2271,7 @@ impl Url {
ref host_end,
ref host,
ref port,
ref port_int,
ref path_start,
ref query_start,
ref fragment_start,
Expand All @@ -2245,6 +2284,7 @@ impl Url {
host_end,
host,
port,
port_int,
path_start,
query_start,
fragment_start,
Expand Down Expand Up @@ -2273,6 +2313,7 @@ impl Url {
host_end,
host,
port,
port_int,
path_start,
query_start,
fragment_start,
Expand All @@ -2285,6 +2326,7 @@ impl Url {
host_end,
host,
port,
port_int,
path_start,
query_start,
fragment_start,
Expand Down
35 changes: 27 additions & 8 deletions url/src/parser.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -485,6 +485,7 @@ impl<'a> Parser<'a> {
let host_end = path_start;
let host = HostInternal::None;
let port = None;
let port_int = None;
let remaining = if let Some(input) = input.split_prefix('/') {
let path_start = self.serialization.len();
self.serialization.push('/');
Expand All @@ -500,6 +501,7 @@ impl<'a> Parser<'a> {
host_end,
host,
port,
port_int,
path_start,
remaining,
)
Expand Down Expand Up @@ -555,6 +557,7 @@ impl<'a> Parser<'a> {
host_end,
host,
port: None,
port_int: None,
path_start: host_end,
query_start,
fragment_start,
Expand Down Expand Up @@ -606,6 +609,7 @@ impl<'a> Parser<'a> {
host_end,
host,
port: None,
port_int: None,
path_start: host_end,
query_start,
fragment_start,
Expand Down Expand Up @@ -666,6 +670,7 @@ impl<'a> Parser<'a> {
base_url.host_end,
base_url.host,
base_url.port,
base_url.port_int,
base_url.path_start,
remaining,
)
Expand All @@ -686,6 +691,7 @@ impl<'a> Parser<'a> {
host_end: path_start,
host: HostInternal::None,
port: None,
port_int: None,
path_start,
query_start,
fragment_start,
Expand All @@ -709,6 +715,7 @@ impl<'a> Parser<'a> {
host_end: path_start,
host: HostInternal::None,
port: None,
port_int: None,
path_start,
query_start,
fragment_start,
Expand Down Expand Up @@ -792,6 +799,7 @@ impl<'a> Parser<'a> {
base_url.host_end,
base_url.host,
base_url.port,
base_url.port_int,
base_url.path_start,
remaining,
)
Expand Down Expand Up @@ -830,6 +838,7 @@ impl<'a> Parser<'a> {
base_url.host_end,
base_url.host,
base_url.port,
base_url.port_int,
base_url.path_start,
remaining,
)
Expand All @@ -851,7 +860,7 @@ impl<'a> Parser<'a> {
let has_authority = before_authority != self.serialization.len();
// host state
let host_start = to_u32(self.serialization.len())?;
let (host_end, host, port, remaining) =
let (host_end, host, port, port_int, remaining) =
self.parse_host_and_port(remaining, scheme_end, scheme_type)?;
if host == HostInternal::None && has_authority {
return Err(ParseError::EmptyHost);
Expand All @@ -867,6 +876,7 @@ impl<'a> Parser<'a> {
host_end,
host,
port,
port_int,
path_start,
remaining,
)
Expand Down Expand Up @@ -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.

fn parse_host_and_port<'i>(
&mut self,
input: Input<'i>,
scheme_end: u32,
scheme_type: SchemeType,
) -> ParseResult<(u32, HostInternal, Option<u16>, Input<'i>)> {
) -> ParseResult<(u32, HostInternal, Option<u16>, Option<u16>, Input<'i>)> {
let (host, remaining) = Parser::parse_host(input, scheme_type)?;
write!(&mut self.serialization, "{}", host).unwrap();
let host_end = to_u32(self.serialization.len())?;
Expand All @@ -968,16 +979,16 @@ impl<'a> Parser<'a> {
}
};

let (port, remaining) = if let Some(remaining) = remaining.split_prefix(':') {
let (port, port_int, remaining) = if let Some(remaining) = remaining.split_prefix(':') {
let scheme = || default_port(&self.serialization[..scheme_end as usize]);
Parser::parse_port(remaining, scheme, self.context)?
} else {
(None, remaining)
(None, None, remaining)
};
if let Some(port) = port {
write!(&mut self.serialization, ":{}", port).unwrap()
}
Ok((host_end, host.into(), port, remaining))
Ok((host_end, host.into(), port, port_int, remaining))
}

pub fn parse_host(
Expand Down Expand Up @@ -1109,7 +1120,7 @@ impl<'a> Parser<'a> {
mut input: Input<'_>,
default_port: P,
context: Context,
) -> ParseResult<(Option<u16>, Input<'_>)>
) -> ParseResult<(Option<u16>, Option<u16>, Input<'_>)>
where
P: Fn() -> Option<u16>,
{
Expand All @@ -1130,10 +1141,16 @@ impl<'a> Parser<'a> {
input = remaining;
}
let mut opt_port = Some(port as u16);
if !has_any_digit || opt_port == default_port() {
let mut opt_port_int = opt_port;
if has_any_digit {
if opt_port == default_port() {
opt_port = None;
}
} else {
opt_port = None;
opt_port_int = None;
}
Ok((opt_port, input))
Ok((opt_port, opt_port_int, input))
}

pub fn parse_path_start<'i>(
Expand Down Expand Up @@ -1368,6 +1385,7 @@ impl<'a> Parser<'a> {
host_end: u32,
host: HostInternal,
port: Option<u16>,
port_int: Option<u16>,
path_start: u32,
remaining: Input<'_>,
) -> ParseResult<Url> {
Expand All @@ -1381,6 +1399,7 @@ impl<'a> Parser<'a> {
host_end,
host,
port,
port_int,
path_start,
query_start,
fragment_start,
Expand Down
8 changes: 4 additions & 4 deletions url/src/quirks.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ pub fn set_host(url: &mut Url, new_host: &str) -> Result<(), ()> {
} else {
Parser::parse_port(remaining, || default_port(scheme), Context::Setter)
.ok()
.map(|(port, _remaining)| port)
.map(|(port, port_int, _remaining)| (port, port_int))
}
} else {
None
Expand All @@ -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.

return Err(());
} else if url.port().is_some() {
return Err(());
Expand Down Expand Up @@ -215,8 +215,8 @@ pub fn set_port(url: &mut Url, new_port: &str) -> Result<(), ()> {
Context::Setter,
)
}
if let Ok((new_port, _remaining)) = result {
url.set_port_internal(new_port);
if let Ok((new_port, new_port_int, _remaining)) = result {
url.set_port_internal(new_port, new_port_int);
Ok(())
} else {
Err(())
Expand Down
Loading