oschwald / maxminddb-golang

MaxMind DB Reader for Go
ISC License
597 stars 101 forks source link

When an IP Addresses is not found, no error is returned #41

Closed whitingj closed 5 years ago

whitingj commented 7 years ago

When the ip address is not found in the database the library doesn't return an error.

I was able to reproduce this problem using the latest GeoLite2-City.mmdb and the ip address 102.53.124.191. From what I can see at https://www.maxmind.com/en/geoip2-city-database-accuracy MaxMind doesn't promise to resolve all ip addresses so it seems useful to return an error when the ip address can not be found.

Here is code that shows the issue:

package main

import (
    "fmt"
    "log"
    "net"
    "os"

    "github.com/oschwald/maxminddb-golang"
)

type City struct {
    City struct {
        GeoNameID uint              `maxminddb:"geoname_id"`
        Names     map[string]string `maxminddb:"names"`
    } `maxminddb:"city"`
    Country struct {
        GeoNameID uint              `maxminddb:"geoname_id"`
        IsoCode   string            `maxminddb:"iso_code"`
        Names     map[string]string `maxminddb:"names"`
    } `maxminddb:"country"`
    Location struct {
        Latitude  float64 `maxminddb:"latitude"`
        Longitude float64 `maxminddb:"longitude"`
    } `maxminddb:"location"`
}

func main() {
    geodbfile := "GeoLite2-City.mmdb"
    if os.Getenv("GEODB") != "" {
        geodbfile = os.Getenv("GEODB")
    }
    db, err := maxminddb.Open(geodbfile)
    if err != nil {
        log.Fatal(err)
    }
    defer db.Close()

    ip := net.ParseIP("102.53.124.191")

    var city City
    err = db.Lookup(ip, &city)

    if err == nil {
        fmt.Println("Didn't get an error but expected one")
    }
    fmt.Println(city, err)
}
oschwald commented 7 years ago

Adding an error when the IP is not found to the existing Lookup method at this point would be a breaking API change as it would break some uses of the library. I am not prepared to do a breaking release at this time. However, I would consider an additional method that does return an error.

Currently, you can use LookupOffset to determine whether the IP has been found.

whitingj commented 7 years ago

I can see why you are hesitant to make a breaking API change. So I would need to do something like this?

ip := net.ParseIP("102.53.124.191")
offset, err := db.LookupOffset(ip)

if ptr != maxminddb.NotFound {
  err = db.Decode(offset, &city)
}

I'm somewhat new to the golang community so I'm not sure the best way to approach a potential breaking api change. If the best way to introduce an API change is to have a new method that makes sense to me.

oschwald commented 7 years ago

Yes, that should work.

I'd be open to a new method as long as the behavior of the old method stayed the same.

oschwald commented 5 years ago

This was resolved by #59 with the new LookupNetwork method.