Is it even possible to submit an empty string as a config file/CLI-provided value?

[atc0005]

While preparing to duplicate an existing validation check like this one the thought hit me: Maybe it's not necessary to guard against empty strings as an input value from a user? It feels "wrong", but perhaps the config packages already have this responsibility?

// Verify that the user did not opt to set an empty string as the value,
// otherwise we fail the config validation by returning an error.
if c.IsSetIgnoredIPAddressesFile() && c.IgnoredIPAddressesFile() == "" {
    return fmt.Errorf("empty path to ignored ip addresses file provided")
}

For now I'm going ahead with duplicating the validation check, but it would be good to learn what the best practice for this is and apply it here (and elsewhere).

[atc0005]

Not sure if it makes the specific difference here, but in our case the validation check is performed specifically for filenames used by the application. The example method given previously alters the behavior of the application depending on its contents, so maybe more validation is required there vs what log file to write where.

However, even that setting makes a difference; fail2ban is configured to look for a log file in a very specific location which is intended to match the location that brick is configured to write to.

[atc0005]

Perhaps this is more a case of failing to properly document intentions than anything else.

This simpler empty string check made not far above the example seems to cover both cases:

if c.ReportedUsersLogFile() == "" {
    return fmt.Errorf("path to reported users log file not provided")
}

Because we have default values assigned, you would only ever see an empty string at this point if the user explicitly provided it (and any config-handling packages didn't object first). If we have a default that the Config.IgnoredUsersFile() method returns, we would only get an empty string if (again), the user explicitly provides it. I'm now thinking that providing an IsSetXYZ() type of validation helper method isn't necessary because we're testing the final results and ensuring that empty strings aren't passed on to the rest of the application.

[atc0005]

Relevant methods:

brick/config/getters.go

Lines 184 to 196 in c38c2e5

	// IsSetIgnoredUsersFile indicates whether a user-provided path to the file
	// containing a list of user accounts which should not be disabled and whose
	// associated IP should not be banned by this application was provided.
	func (c Config) IsSetIgnoredUsersFile() bool {
	switch {
	case c.cliConfig.IgnoredUsers.File != nil:
	return true
	case c.fileConfig.IgnoredUsers.File != nil:
	return true
	default:
	return false
	}
	}

brick/config/getters.go

Lines 212 to 224 in c38c2e5

	// IsSetIgnoredIPAddressesFile indicates whether a user-provided path to the
	// file containing a list of individual IP Addresses which should not be
	// banned by this application was provided.
	func (c Config) IsSetIgnoredIPAddressesFile() bool {
	switch {
	case c.cliConfig.IgnoredIPAddresses.File != nil:
	return true
	case c.fileConfig.IgnoredIPAddresses.File != nil:
	return true
	default:
	return false
	}
	}

I'm seriously leaning towards removing them, but will hold off while I do further research to confirm I'm thinking of all of the angles here.