ensure single_ip is cast #to_s before comparison

[ryanbreed]

the include? method incorrectly initializes a local variable 'other' from its called argument single_ip

Description

Nexpose::IPRange#include? expects to be passed another Nexpose::IPRange instance for its comparison logic. It instantiates a local IPAddr object from this argument without first casting to a compatible type. This is probably incorrect behavior in some versions of core runtime.

This is a quick & dirty hack that doesn't affect any of the class internal representation, local initialization or comparison logic.

Motivation and Context

IPRange include? function error #240

How Has This Been Tested?

12:28 $ bundle exec rspec
..................................................
Finished in 0.35073 seconds (files took 0.48575 seconds to load)
50 examples, 0 failures

Types of changes

• Bug fix (non-breaking change which fixes an issue)

Checklist:

• I have updated the documentation accordingly (if changes are required).
• I have added tests to cover my changes.
• All new and existing tests passed.

[gschneider-r7]

I feel like it would be better to make this method more flexible and not require an IPRange as input in the first place. It should be able to accept IPRange, IPAddr, and String and handle each one just fine. @sgreen-r7 and I talked about this a few weeks ago and figured we'd end up with breaking changes as a result of our desired fix.

Maybe this can suffice for now.

[ryanbreed]

That was my first thought. I'm happy to take a run at it as an enhancement/feature branch and include the requisite test coverage. I think i can frame out the implementation in such a manner that won't create runtime exceptions in calling code expecting the older behavior (just make sure to always return a bool).

I'd take it a few steps further, up to and including accepting ranged IPAddrs or even ripping the guts out of IPRange altogether and delegating as much to corelib IPAddr as possible. I've done similar work a couple times already in other code.

[ryanbreed]

@gschneider-r7 here is what that more tolerant method is starting to look like. Since the logic is pretty switchy, I added a bunch of unit tests and test cases. I also took the liberty of extracting the IPRange class to its own file. I'll get around to renaming the spec if you think this is heading in the right direction. To preserve the old behavior, I raised some warnings for cases where strings cant properly be coerced into an IPAddr - there didn't seem to be an API logger available.

given the number of in-method reconversions to IPAddr, i'm still thinking that IPRange is worth refactoring to a different internal representation.

[gschneider-r7]

Nice! I like seeing the added test coverage.

given the number of in-method reconversions to IPAddr, i'm still thinking that IPRange is worth refactoring to a different internal representation.

What are you thinking that would look like? Should we just use IPAddr under the hood?

[ryanbreed]

yep, either a pair of IPAddrs or an IPaddr + int. It should be possible to maintain conformity with the xml/json/hash representations of the instance, as well as the string representation accessible via Nexpose::IPRange#from and Nexpose::IPRange#to.