Fix assign ip address with invalid values raise exception #11574
Conversation
|
/cc @kennyj, @senny, @rafaelfranca |
|
In case of other fields that cannot parse, setting invalid value sets it to nil. record = SomeRecord.new
record.updated_at = 'asdf'
record.updated_at # => nil
record.updated_at_before_type_cast # => 'asdf'If saved, Should the same behavior be preserved for other parseable fields like IP address? |
|
cool idea, thanks, I thought about it. Have skipped it to be consistent with others casts for Postgresql. But, I think need to give it a chance. Will review more casts solutions. |
|
Changed fix with using as @egilburg mention So, it's ready for reviewing. Thanks in advance. P.S. Build failed because of: $ gem install bundler
ERROR: Could not find a valid gem 'bundler' (>= 0), here is why:
Unable to download data from https://rubygems.org/ - Errno::ETIMEDOUT: Connection timed out - connect(2) (https://rubygems.org/latest_specs.4.8.gz)
ERROR: Possible alternatives: bundler |
| @@ -100,7 +100,7 @@ def string_to_cidr(string) | |||
| if string.nil? | |||
| nil | |||
| elsif String === string | |||
| IPAddr.new(string) | |||
| IPAddr.new(string) rescue nil | |||
There was a problem hiding this comment.
Is there a specific Exception we can rescue to assign nil?
There was a problem hiding this comment.
It may raise: RuntimeError or ArgumentError
There was a problem hiding this comment.
In what circumstances does it raise a RuntimeError?
Sample irb session:
Loading development environment (Rails 4.0.0)
irb(main):001:0> IPAddr.new("akjsd")
IPAddr::InvalidAddressError: invalid address
There was a problem hiding this comment.
You checked 2.0.0 ruby version. But I found in 1.9.3 version only 2 exceptions. Not sure that #set invoked from #initialize
There was a problem hiding this comment.
Yes, we can get RuntimeError only from methods which are not invoked on initialization step.
There was a problem hiding this comment.
for ruby 1.9.3:
ArgumentError: invalid addressfor ruby 2.0.0:
IPAddr::InvalidAddressError: invalid addressThere was a problem hiding this comment.
I don't completely understand what you mean with:
Yes, we can get RuntimeError only from methods which are not invoked on initialization step.
As IPAddr::InvalidAddressError is a subclass of ArgumentError catching it would be perfect but I'm still not sure when you get a RuntimeError.
There was a problem hiding this comment.
IPAddr::InvalidAddressError is sub-class of ArgumentError
There was a problem hiding this comment.
I got your point, fixing it. Thanks.
|
this no longer applies cleanly, can you rebase? |
|
Thanks @senny for your comments. Updated with rescue only specific exception |
| * Assign inet/cidr attribute with `nil` value for invalid address. | ||
|
|
||
| `IPAddr` raise exception on parsing invalid address. Rescue exception | ||
| on type cast and return `nil`. |
There was a problem hiding this comment.
I think this paragraph should be replaced with an Example. It explains the situation better from a user perspective.
In order that set attribute should not be bang method
|
@senny, updated with example of the issue in Changelog. |
…et_assign Fix assign ip address with invalid values raise exception
|
Thank you 💛 |
|
Thanks @senny! |
|
👍 |
…et_assign Fix assign ip address with invalid values raise exception Conflicts: activerecord/CHANGELOG.md
|
Hey, since you merged this one for 4.0.4rc1, how about merging #11867 for consistency ? |
Closes #11552
In order that set attribute should not be bang method, rescue invalid ip address exceptions.
There is two options to resolve #11552 issue:
IPAddr.newvalue likeBigDecimaldoes:IPAddr.new 'invalid' #=> #<IPAddr: IPv6:0000:0000:0000:0000:0000:0000:0000:0000/ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff>What do you think about second solution?