set_port, set_username & set_password should check for a value before returning Err(())

[chanced]

The methods set_port, set_username and set_password all fail early if called when the url cannot have a port/username/password:

    pub fn set_port(&mut self, mut port: Option<u16>) -> Result<(), ()> {
        // has_host implies !cannot_be_a_base
        if !self.has_host() || self.host() == Some(Host::Domain("")) || self.scheme() == "file" {
            return Err(());
        }

    pub fn set_username(&mut self, username: &str) -> Result<(), ()> {
        // has_host implies !cannot_be_a_base
        if !self.has_host() || self.host() == Some(Host::Domain("")) || self.scheme() == "file" {
            return Err(());
        }

    pub fn set_password(&mut self, password: Option<&str>) -> Result<(), ()> {
        // has_host implies !cannot_be_a_base
        if !self.has_host() || self.host() == Some(Host::Domain("")) || self.scheme() == "file" {
            return Err(());
        }

I believe they should first check for the existence of a value (or empty string, in the case of set_username) before returning an error.

    pub fn set_username(&mut self, username: &str) -> Result<(), ()> {
        // has_host implies !cannot_be_a_base
        if !self.has_host() || self.host() == Some(Host::Domain("")) || self.scheme() == "file" {
+           if username.is_empty() {
+                return Ok(());
+           }
            return Err(());
        }

    pub fn set_password(&mut self, password: Option<&str>) -> Result<(), ()> {
        // has_host implies !cannot_be_a_base
        if !self.has_host() || self.host() == Some(Host::Domain("")) || self.scheme() == "file" {
+           if password.is_none() || password == Some("") {
+                return Ok(());
+           }
            return Err(());
        }

[lucacasonato]

Why do you expect this?

[chanced]

I need to wrap Url for a Uri type. To do so, I need a method that allows for setting the authority. It would be nice not to have to ignore errors when attempting to set an empty value when it should not be applicable.

My workaround:

if u.set_username(authority.username().unwrap_or_default())
    .is_err()
{
    // the url crate doesn't check for empty values before returning `Err(())`
    // https://github.com/servo/rust-url/issues/844
    let username = authority.username().unwrap_or_default();
    if !username.is_empty() {
        return Err(AuthorityError::UsernameNotAllowed(username.to_string()).into());
    }
}
if u.set_password(authority.password()).is_err() {
    let password = authority.password().unwrap_or_default();
    if !password.is_empty() {
        return Err(AuthorityError::PasswordNotAllowed(password.to_string()).into());
    }
}
u.set_host(authority.host())?;
if u.set_port(authority.port()).is_err() {
    if let Some(port) = authority.port() {
        return Err(AuthorityError::PortNotAllowed(port).into());
    }
}

[lucacasonato]

Or you check for the empty string before calling set_username?

[chanced]

Sure, but that may be a valid value (as in, unsetting the username). Arguably, I could perform the same checks (!url.has_host() || url.host() == Some(Host::Domain("")) || url.scheme() == "file") but that leaves my side of things brittle. If additional checks are added to the url crate, I need to mirror those.

[lucacasonato]

We're bound to whatever the URL specification says. I'd have to check whether we can change this

[chanced]

I think you should be able to.

This only changes whether an error is returned in the event that someone attempts to set an empty port/username/password when the port/username/password cannot be set.

On the happy path, for example when an empty string is attempted to be set for the username of a file:// url, the Url would not be altered anyhow. The same goes for password. The call effectively becomes a no-op.

If a non-empty value is provided for either and they are not allowed, the current logic of returning an Err(()) is maintained.

[chanced]

This also applies to set_port