| msg212550 - (view) |
Author: Michel Albert (exhuma) * |
Date: 2014-03-02 13:18 |
| The ipaddress module always returns ``False`` when testing if a network is contained in another network. However, I feel this should be a valid test. No? Is there any reason why this is fixed to ``False``? In case not, here's a patch which implements this test. Note that by design, IP addresses networks can never overlap "half-way". In cases where this should return ``False``, you either have a network that lies completely "to the left", or completely "to the right". In the case it should return ``True`` the smaller network is always completely bounded by the larger network's network- and broadcast address. I needed to change two containment tests as they were in conflict with this change. These tests were ``self.v6net not in self.v6net`` and ``self.v4net not in self.v4net``. The reason these two failed is that the new containment test checks the target range *including* broadcast and network address. So ``a in a`` always returns true. This could be changed by excluding one of the two boundaries, and by that forcing the "containee" to be smaller than the "container". But it would make the check a bit more complex, as you would need to add an exception for the case that both are identical. Backwards compatibility is a good question. Strictly put, this would break it. However, I can't think of any reason why anyone would expect ``a in a`` to be false in the case of IP-Addresses. Just as a side-note, I currently work at our national network provider and am currently implementing a tool dealing with a lot of IP-Addresses. We have run into the need to test ``net in net`` a couple of times and ran into bugs because the stdlib returns ``False`` where you technically expect it to be ``True``. |
|
|
| msg212551 - (view) |
Author: Antoine Pitrou (pitrou) *  |
Date: 2014-03-02 13:56 |
| I don't think "in" is the right operator for this. You can draw an analogy with sets: >>> a = {1,2} >>> b = {1,2,3} >>> a <= b True >>> a in b False In mathematical terms, there is a difference between inclusion (being a subset of) and containment (being an element of). |
|
|
| msg212552 - (view) |
Author: Michel Albert (exhuma) * |
Date: 2014-03-02 14:07 |
| Hmm... after thinking about this, I kind of agree. I was about to state something about the fact that you could consider networks like an "ordered set". And use that to justify my addition :) But the more I think about it, the more I am okay with your point. I quickly tested the following: >>> a = ip_network('10.0.0.0/24') >>> b = ip_network('10.0.0.0/30') >>> a <= b True >>> b <= a False Which is wrong when considering "containement". What about an instance-method? Something like ``b.contained_in(a)``? At least that would be explicit and avoids confusion. Because the existing ``__lt__`` implementation makes sense in the way it's already used. |
|
|
| msg212558 - (view) |
Author: R. David Murray (r.david.murray) * |
Date: 2014-03-02 15:15 |
| It seems from the ipaddress documentation that 'a.contains(b)' would appropriate. But to avoid confusion with 'in', you could say a.covers(b). However, 'in' is already used for in , and you can already get subnets out of a network (via 'subnets'), so it isn't completely crazy to consider the network a container of subnets (of varying sizes, depending on the arguments to subnet). |
|
|
| msg212560 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2014-03-02 15:23 |
| > However, 'in' is already used for in , and you can > already get subnets out of a network (via 'subnets'), so it isn't > completely crazy to consider the network a container of subnets (of > varying sizes, depending on the arguments to subnet). So how about subnet_of(other)? (and the reciprocal supernet_of(other)) |
|
|
| msg212695 - (view) |
Author: Michel Albert (exhuma) * |
Date: 2014-03-04 08:19 |
| I second "supernet_of" and "subnet_of". I'll implement it as soon as I get around it. I have been thinking about using ``in`` and ``<=`` and, while I initially liked the idea for tests, I find both operators too ambiguous. With ``in`` there's the already mentioned ambiguity of containment/inclusion. And ``<=`` could mean is a smaller size (has less individual hosts), but could also mean that it is a subnet, or even that it is "located to the left". Naming it ``subnet_of`` makes it 100% clear what it does. Currently, ``a <= b`` returns ``True`` if a comes before/lies on the left of b. |
|
|
| msg212792 - (view) |
Author: Eric V. Smith (eric.smith) * |
Date: 2014-03-06 03:10 |
| There is some history for using "in" for containment. I'm porting some code from IPy (https://pypi.python.org/pypi/IPy/), and it uses "in". It would make my life easier if "in" worked in ipaddress, but then again it would have to be a previously release version of ipaddress. So I'm open to any names. It's definitely a useful feature. |
|
|
| msg212805 - (view) |
Author: Michel Albert (exhuma) * |
Date: 2014-03-06 11:50 |
| Here's a new patch implementing both ``subnet_of`` and ``supernet_of``. It also contains the relevant docs and unit-tests. |
|
|
| msg213167 - (view) |
Author: pmoody (pmoody) * |
Date: 2014-03-11 19:31 |
| subnet_of and supernet_of also avoid confusion with the IP?Interface classes (which incidentally can be used in 'a in b' containment tests). Michael, have you signed the contributor license agreement? I don't think I have anyway of seeing if you have or not and I think you need to sign it for me to apply you patch. http://www.python.org/psf/contrib/contrib-form/ |
|
|
| msg213169 - (view) |
Author: Michel Albert (exhuma) * |
Date: 2014-03-11 19:38 |
| Yes. I signed it last Friday if I recall correctly. As I understood it, the way for you to tell if it's done, is that my username will be followed by an asterisk. But I'm not in a hurry. Once I get the confirmation, I can just ping you again via a comment here, so you don't need to monitor it yourself. |
|
|
| msg213173 - (view) |
Author: pmoody (pmoody) * |
Date: 2014-03-11 20:22 |
| Thanks! |
|
|
| msg214561 - (view) |
Author: Michel Albert (exhuma) * |
Date: 2014-03-23 10:54 |
| Hi again, The contribution agreement has been processed, and the patch still cleanly applies to the latest revision of branch `default`. |
|
|
| msg214572 - (view) |
Author: R. David Murray (r.david.murray) * |
Date: 2014-03-23 12:49 |
| The patch needs .. versionadded:: 3.5 tags for the two new methods, and adding it to the skeleton whatsnew would be a good idea. The committer can do these, but if you feel like updating the patch that would be great. |
|
|
| msg214591 - (view) |
Author: Michel Albert (exhuma) * |
Date: 2014-03-23 15:31 |
| I made the changes mentioned by r.david.murray I am not sure if the modifications in ``Doc/whatsnew/3.5.rst`` are correct. I tried to follow the notes at the top of the file, but it's not clear to me if it should have gone into ``News/Misc`` or into ``Doc/whatsnew/3.5.rst``. On another side-note: I attached this as an ``-r3`` file, but I could have replaced the existing patch as well. Which method is preferred? Replacing existing patches on the issue or adding new revisions? |
|
|
| msg246401 - (view) |
Author: James (JamesGuthrie) |
Date: 2015-07-07 11:24 |
| What is the status of these changes? Apparently they were slated for inclusion in 3.5 but it looks as though they haven't hit yet - is there a reason for this, or was it just forgotten? |
|
|
| msg266869 - (view) |
Author: James Schneider (James Schneider) |
Date: 2016-06-02 03:21 |
| I'd like to ask for a status on getting this merged? As a network administrator, these changes would have a magical effect on my code dealing with routing tables and ACL's. |
|
|
| msg269225 - (view) |
Author: Michel Albert (exhuma) * |
Date: 2016-06-25 10:30 |
| I just realised that the latest patch on this no longer applies properly. I have fixed the issue and I am currently in the process of running the unit-tests which takes a while. Once those pass, I'll update some metadata and resubmit. |
|
|
| msg269226 - (view) |
Author: Michel Albert (exhuma) * |
Date: 2016-06-25 10:47 |
| Test pass properly. Is there anything else left to do? Here's the fixed patch (net-in-net-r4.patch) |
|
|
| msg269228 - (view) |
Author: SilentGhost (SilentGhost) *  |
Date: 2016-06-25 10:58 |
| Have you seen my comments on rietveld re you previous patch? |
|
|
| msg269229 - (view) |
Author: Michel Albert (exhuma) * |
Date: 2016-06-25 11:33 |
| Updated patch, taking into account notes from the previous patch-reviews |
|
|
| msg269232 - (view) |
Author: Berker Peksag (berker.peksag) * |
Date: 2016-06-25 11:46 |
| Thanks for the updated patch. Some comments from a quick review: * We need tests for the TypeError branches in both methods * + 'of type %s' % type(other) type(other) -> type(other).__name__ * You can drop the XXX part in +XXX Instances of * Perhaps code duplication mentioned by SilentGhost can be eliminated by using the operator module |
|
|
| msg269238 - (view) |
Author: Michel Albert (exhuma) * |
Date: 2016-06-25 13:46 |
| I don't quite see how the operator module could help. I don't have much experience with it though, so I might be missing something... I don't see how I can relate one check to the other. The example I gave in the patch review was the following: With the existing implementation: '192.168.1.0/25' subnet of '192.168.1.128/25' -> False '192.168.1.0/25' supernet of '192.168.1.128/25' -> False With the proposal to simply return "not subnet_of(...)" it would become: '192.168.1.0/25' subnet of '192.168.1.128/25' -> False '192.168.1.0/25' supernet of '192.168.1.128/25' -> True which would be wrong. I have now added the new test-cases for the TypeError and removed the code-duplication by introducing a new "private" function. Let me know what you think. I am running all test cases again and I'll uploaded it once they finished. |
|
|
| msg269239 - (view) |
Author: Michel Albert (exhuma) * |
Date: 2016-06-25 14:35 |
| New patch with proposed changes. |
|
|
| msg276208 - (view) |
Author: Michel Albert (exhuma) * |
Date: 2016-09-13 08:05 |
| Are there any updates on this? Not sure if it's too late again to get it applied for the next Python (3.6) release? |
|
|
| msg279066 - (view) |
Author: James Schneider (James Schneider) |
Date: 2016-10-20 18:52 |
| Please consider for implementation in 3.6. I'd love it even more for 3.5 but I don't think that will happen. With the latest patch, I don't believe there are any backwards-incompatible changes, though. |
|
|
| msg294244 - (view) |
Author: Aleksandr Balezin (gescheit) * |
Date: 2017-05-23 11:45 |
| I've reviewed this patch and want to make some advices. - hasattr is unwanted here. There is no any similar usage hasattr in this module. Also before hasattr there is a call of _ipversion method. If other is not instance of BaseNetwork it will raise AttributeError exception before hasattr check. - It is not a good manner comparing thru "other.network_address >= self.network_address and other.broadcast_address <= self.broadcast_address"(see ). Networks must be compared thru "other._prefixlen >= self._prefixlen and other.network._ip & self.netmask._ip == self.network._ip" for performance reason. - _containment_check function is excessive. There is no common logic in supernet_of/subnet_of function except _ipversion and type checking. I think this two functions should be simple as: def subnet_of(self, other): if self._version != other._version: raise TypeError('%s and %s are not of the same version' % (self, other)) if other._prefixlen >= self._prefixlen and other.network._ip & self.netmask._ip == self.network._ip: return True return False |
|
|
| msg302798 - (view) |
Author: Cheryl Sabella (cheryl.sabella) * |
Date: 2017-09-23 19:06 |
| Hello Michel, Would you be able to convert your patch to a Github pull request? It seemed like there was interest in merging this at some point, so maybe a PR would get it moving towards that again. |
|
|
| msg304683 - (view) |
Author: Mark Ignacio (Mark.Ignacio) |
Date: 2017-10-20 21:12 |
| Hey Michel, Are you still interested in pushing this patch through? It's be awesome if this got committed. |
|
|
| msg304768 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2017-10-22 21:39 |
| New changeset 91dc64ba3f51100540b2ab6c6cd72c3bb18a6d49 by Antoine Pitrou (Cheryl Sabella) in branch 'master': bpo-20825: Containment test for ip_network in ip_network. https://github.com/python/cpython/commit/91dc64ba3f51100540b2ab6c6cd72c3bb18a6d49 |
|
|
| msg304769 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2017-10-22 21:40 |
| This is now in 3.7. Thanks to everyone who contributed! |
|
|