-
Notifications
You must be signed in to change notification settings - Fork 990
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
Fixes #37651 - Fix error msg about invalid MAC appearing twice #10245
Conversation
9c0dcbe
to
bcea13d
Compare
record.errors.add(attribute, (options[:message] || _("is not a valid MAC address"))) | ||
err_msg = _("is not a valid MAC address") | ||
# error message can already be present from the Net::Validations::normalize_mac method | ||
if record.errors.full_messages.none? { |message| message.include?(err_msg) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be sufficient to check if for the given attribute there's already any error message? I'd like to avoid doing string comparisons on error messages because it can easily break on translations. For example, when one is missing a translation while the other is translated. Or in a different way so it's not a substring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the tip, it seems to do the trick.
bcea13d
to
c35c37d
Compare
c35c37d
to
d0fa1d0
Compare
@@ -6,7 +6,10 @@ def validate_each(record, attribute, value) | |||
end | |||
|
|||
def make_invalid(record, attribute) | |||
record.errors.add(attribute, (options[:message] || _("is not a valid MAC address"))) | |||
# error message can already be present from the Net::Validations::normalize_mac method | |||
if record.errors[:mac].empty? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be for the same attribute?
if record.errors[:mac].empty? | |
if record.errors[attribute].empty? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the double comment, but I just looked and in the other place it's also hardcoded:
foreman/app/services/ipam/base.rb
Lines 64 to 69 in 2d5bd87
def normalize_mac! | |
return unless mac.present? | |
self.mac = Net::Validations.normalize_mac(mac) | |
rescue Net::Validations::Error | |
errors.add(:mac, _('is not a valid MAC address')) | |
end |
That makes me question why we call normalize twice. I'm not sure the service should add an error in the first place but it was introduced in 990dee6 so I suspect this bug has been around for a long time.
What I suspect happens is that it calls before_validation
:
foreman/app/models/nic/base.rb
Line 14 in 2d5bd87
before_validation :normalize_mac |
foreman/app/models/nic/base.rb
Lines 282 to 287 in 2d5bd87
def normalize_mac | |
self.mac = Net::Validations.normalize_mac(mac) | |
true | |
rescue Net::Validations::Error => e | |
errors.add(:mac, e.message) | |
end |
And then it validates (which is the code you're editing now):
foreman/app/models/nic/base.rb
Line 24 in 2d5bd87
validates :mac, :mac_address => true, :allow_blank => true |
It makes me think your current patch is a correct short term fix (because it does make still ensure it's invalid), but we should really untangle the whole mess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the double comment, but I just looked and in the other place it's also hardcoded:
foreman/app/services/ipam/base.rb
Lines 64 to 69 in 2d5bd87
def normalize_mac! return unless mac.present? self.mac = Net::Validations.normalize_mac(mac) rescue Net::Validations::Error errors.add(:mac, _('is not a valid MAC address')) end That makes me question why we call normalize twice. I'm not sure the service should add an error in the first place but it was introduced in 990dee6 so I suspect this bug has been around for a long time.
I am not sure if normalize_mac and MacAddressValidator do the same thing, so I'd prefer not to touch that 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ekohl should I revert this back to record.errors.[:mac]
?
d0fa1d0
to
a4ab4bb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. Just one small note where I'm not entirely sure about the Rails API.
@@ -6,7 +6,10 @@ def validate_each(record, attribute, value) | |||
end | |||
|
|||
def make_invalid(record, attribute) | |||
record.errors.add(attribute, (options[:message] || _("is not a valid MAC address"))) | |||
# error message can already be present from the Net::Validations::normalize_mac method | |||
if record.errors[attribute].empty? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you happen to know if it's guaranteed to be non-nil? I'd be tempted to add the safe navigator.
if record.errors[attribute].empty? | |
if record.errors[attribute]&.empty? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should always be non-nil, but we can't do anything wrong by adding the safe navigator so I am accepting the suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I realized this inverts the logic should record.errors be nil. I will come up with something else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added check for nil.
800a4a6
to
ffce2b8
Compare
aa6c7c4
to
b4eccd2
Compare
Currently when entering an invalid MAC address while editing a host's interface, the message shown is: "'fa:16:3e:17:3e:19x' is not a valid MAC address and is not a valid MAC address" This is because before the validation itself, another method Net::Validations::normalize_mac is called, which also raises an error upon finding a MAC address invalid (foreman/app/models/nic/base.rb). Since I found the method normalize_mac being used at multiple places, I did not want to alter the behavior of this method. Validation through the MacAddressValidator seems to be used only once, so I deemed more proper to edit the method there. Still, I am not certain if the two methods are equivalent as far as validation goes, so I just added a condition which prevents duplicating the error message if normalize_mac already threw an error.
b4eccd2
to
abf0c87
Compare
Currently when entering an invalid MAC address while editing a host's interface, the message shown is:
"'fa:16:3e:17:3e:19x' is not a valid MAC address and is not a valid MAC address"
This is because before the validation itself, another method normalize_mac is called, which also raises an error upon finding a MAC address invalid (foreman/app/models/nic/base.rb).
Since I found the method normalize_mac being used at multiple places, I did not want to alter the behavior of this method. Validation through the MacAddressValidator seems to be used only once, so I deemed more proper to edit the method there. Still, I am not certain if the two methods are equivalent as far as validation goes, so I just added a condition which prevents duplicating the error message if normalize_mac already threw an error.