-
Notifications
You must be signed in to change notification settings - Fork 35
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
Add IPAddr#unmasked #26
base: master
Are you sure you want to change the base?
Conversation
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 breaks when used on an instance initiated with a non-string:
a = IPAddr.new(2130706433, Socket::AF_INET)
a.to_s == "127.0.0.1"
a.to_unmasked_string
# NoMethodError: undefined method `>>' for nil:NilClass
I've submitted a PR to your fork to amend this: composerinteralia#1
b06488d
to
fb337bc
Compare
Nice catch @CvX. While reviewing your changes I also spotted some other cases where we call |
Good one! LGTM 🙂 |
Do you think I agree with the functionality. Just want to make sure this is the right name. Is this a How does this work with IPv6? |
fb337bc
to
c4472f7
Compare
@ioquatix thanks for the review.
Yeah, I think your suggestion of
I have a couple IPv6 test cases. Are there additional cases that I am missing? |
c4472f7
to
d8e5562
Compare
Prior to this commit there was no way to retrieve the unmasked address for an `IPAddr`. For example, given the `IPAddr` below there is no method or instance variable that would return `"1.2.3.4"`. ```rb IPAddr.new("1.2.3.4/8") ``` This poses a problem for Rails in supporting the PostgreSQL inet type. Rails uses `IPAddr` for both the cidr and inet types, but at the moment cannot fully support the inet type, which "accepts values with nonzero bits to the right of the netmask". This came up originally in rails/rails#14857, and more recently in rails/rails#40138. The change in this commit would allow us to support the inet type without moving to another library. This commit holds onto the address before the mask is applied, in a new instance variable `@unmasked_addr`. It then exposes this via `unmasked`. This was originally implemented back in ruby/ruby#599, but it got stale and was eventually closed. There are also a few differences with that PR: - That PR set up the instance variable inside of `mask!`, which meant that the new method was broken in the case of an `IPAddr` with no mask. - That PR introduced a method that returned a `to_s`-like value, whereas this one returns a new, unmasked IPAddr object. - As a result of the above, this version doesn't need to change the signature of `to_s` - This PR also sets @unmasked_addr within `set`, which covers a few additional edge cases with non-string arguments and with methods like `&` or `succ` that clone the IpAdrr and call `set` Co-authored-by: Jarek Radosz <[email protected]>
d8e5562
to
d5e866d
Compare
LGTM. |
The maintainer of ipaddr is @knu |
Hello, I have a question about this. It actually took me quite a while to figure out what's wrong, as also Postgres' inet data type is mapped to IPAddr.
Athough it's a behavior change, normally I would expect IPAddr to keep the host bits. So wouldn't it make sense to adjust the code, that the example above returns '1.2.3.4'? and to introduce a function that returns the network address (discards the host bits)?
Also, #succ would be more useful if you could initialize a new object with non-zero host bits.. On the proposed function If the goal is to not break the (strange) behavior, maybe an
Both would work fine, as IPAddr already now is able to cope with non-zero host bits:
Seb |
@composerinteralia do you have some time to check the above feedback? It would be awesome to merge this in time for Ruby 3.1 cc @knu |
I’m on vacation for the next two weeks away from a computer, but I can take a look when I get back. Anyone is also welcome to take over in the meantime. |
I can revise this PR if that's the direction folks want to go, but it's a potentially disruptive breaking change for anyone relying on the existing behavior (I believe we're relying on it for the cidr type in Rails, for example). I assume that would need to be a major release, and ideally there would be some sort of deprecation cycle.
I believe that's what this PR does. (Although it's been a while, so please correct me if I am mistaken.) |
Prior to this commit there was no way to retrieve the unmasked address
for an
IPAddr
. For example, given theIPAddr
below there is nomethod or instance variable that would return
"1.2.3.4"
.This poses a problem for Rails in supporting the PostgreSQL inet type.
Rails uses
IPAddr
for both the cidr and inet types, but at the momentcannot fully support the inet type, which "accepts values with nonzero
bits to the right of the netmask". This came up originally in
rails/rails#14857, and more recently in
rails/rails#40138. The change in this commit
would allow us to support the inet type without moving to another
library.
This commit holds onto the address before the mask is applied, in a new
instance variable
@unmasked_addr
. It then exposes this viaunmasked
.This was originally implemented back in
ruby/ruby#599, but it got stale and was
eventually closed. There are also a few differences with that PR:
mask!
, which meantthat the new method was broken in the case of an
IPAddr
with nomask.
to_s
-like value, whereasthis one returns a new, unmasked IPAddr object.
signature of
to_s
set
, which covers a fewadditional edge cases with non-string arguments and with methods like
&
orsucc
that clone the IpAdrr and callset