How Settings::BLANK_ATTRS are meant to work

Development
1

Hello,

I am looking into a bug when a particular (discovery) setting can be set to blank value. (https://bugzilla.redhat.com/show_bug.cgi?id=1470083)

I can’t get my head around how Setting:BLANK_ATTRS is designed. It’s an constant array defined in Setting model and pushed from other STI subclasses.

Now, the validators are the following:

validates :default, :presence => true, :unless => proc { |s| s.settings_type == "boolean" || BLANK_ATTRS.include?(s.name) }

validates :value, :presence => true, :if => proc { |s| s.settings_type == "array" && !BLANK_ATTRS.include?(s.name) }

Problem is, if I have a setting with a default value and it is not in the list of BLANK_ATTRS, it never gets invalidated. IIUC default value is only taken into account if value is nil. It does not make much sense to me, what exactly I am missing?

2

BLANK_ATTRS works as an array, so if we append any setting into the BLANK_ATTRS array then that setting can have an empty value.

Check this out: https://github.com/theforeman/foreman/blob/develop/app/models/setting.rb#L43
This validation indicates that if a setting has a setting_type set to array and is not a part of the BLANK_ATTRS array then that setting cannot be blank.

This PR: https://github.com/theforeman/foreman/pull/7934 makes sure that all settings that are of the setting_type array or string they must not be blank, unless they are a part of the BLANK_ATTRS array.

This is my understanding, let me know if I missed anything.

3

First validation states “only boolean or BLANK_ATTR can have nil default”, second “Validate array values not to be blank unless it’s a BLANK_ATTR”.

It is that way for a long time, I think it could be because there is not really clear and good API to add to BLANK_ATTRS.

If we change this, we should go over all the string settings over all the plugin (or make it backward compatible with warning) and make sure they are supposed to be filled up or not.

IMHO it’s not worth the effort, I’d rather make more clear setting API first.

2 Likes