maxmind / MaxMind-DB-Reader-dotnet

.NET Reader for the MaxMind DB Database Format
https://www.nuget.org/packages/MaxMind.Db
Apache License 2.0
102 stars 32 forks source link

Returning parent block of an IP #22

Closed sarus closed 8 years ago

sarus commented 8 years ago

Hello,

I posted a related issue (https://github.com/maxmind/GeoIP2-dotnet/issues/54) over on the GeoIP2-dotnet github issue tracker and it was suggested to submit a PR to this repo to add the ability to get the network block for an IP.

I'm looking over the code now and wanted to see how you'd like to expose this functionality. I could see adding a method like this

public T FindWithBlock<T>(IPAddress ipAddress, InjectableValues injectables = null) where T : class

Which would inject a new block attribute into the returned data so usage would be something like:

    public class GeoIP2Block
    {
        public string Block;

        [Constructor]
        public GeoIP2Block([InjectAttribute("block")] string block)
        {
            Block = block;
        }
    }

    internal class Program
    {
        private static void Main(string[] args)
        {
            using (var reader = new Reader("GeoLite2-City.mmdb", FileAccessMode.Memory))
            {
                var result = reader.FindWithBlock<GeoIP2Block>(IPAddress.Parse("1.188.0.56"));
                Console.WriteLine(result.Block);
                // result.Block in this example would be "1.188.0.0/14"
            }
        }
}

The other option would be to create a method specific to returning the block data

public string GetIpBlock(IPAddress ip);

Which would just return the block. Rather than return just a string representation of the block I would probably include other information. I could see returning the IP address and the routing prefix separately and maybe the range as well:

public IPGeoBlock GetIpBlock(IPAddress ip);
//IpGeoBlock would contain:
block: 1.188.0.0/14
ip: 1.188.0.0
routingPrefix: 14
start: 29097984
end: 29360127

Maybe instead of start and end integers use a byte array?

Thanks for your feedback! Happy to submit a PR once I know which direction to go in.

oschwald commented 8 years ago

What about just providing the prefix length, e.g., 14 in your example. From this, it should be easy to calculate any of the other data. I don't know if injecting the data into the model is necessary. It seems like an out param would probably be sufficient.

sarus commented 8 years ago

I like that. Much simpler. How about overloading Find with a version that has an out parameter for the routingPrefix:

public T Find<T>(IPAddress ipAddress, out int routeingPrefix, InjectableValues injectables = null) where T : class

If that works for you i can put together the PR.

oschwald commented 8 years ago

That sounds good to me.

sarus commented 8 years ago

Would you object to updating nunit to 3.x? The ExpectedException syntax is no longer supported but I could update those tests to use Assert.Throws.

sarus commented 8 years ago

On second thought that should just be in a separate pull requesst

oschwald commented 8 years ago

NUnit 3 sounds great, but I agree it might be better as a separate PR.

oschwald commented 8 years ago

Thanks! :+1: I merged #23.