geoip2 icon indicating copy to clipboard operation
geoip2 copied to clipboard

Add support for database types other than MaxMind

Open sblinch opened this issue 3 years ago • 5 comments

There are now a number of alternative providers of MMDB databases (including the free Geoacumen database generated from ASN data), but they are unusable with the geoip2 library because it explicitly checks for a MaxMind-specific reader.metadata.DatabaseType.

This pull request maintains the current behavior while adding the option to specify a custom database type when needed by calling NewXxxReaderType() instead of NewXxxReader(). It also accepts Geoacumen-Country as a valid database type for NewCountryReader() by default.

sblinch avatar Dec 15 '21 21:12 sblinch

Hi, thanks for your interest.

I think, you can just add Geoacumen-Country to supported types, without other changes. How I can see, Geoacumen contains only one field in country - iso_code, this field is supported in the library, but anyway test should be written for this database.

This library has hardcoded available fields, because of that it's not possible to permit user to use any types of database.

IncSW avatar Dec 16 '21 10:12 IncSW

Thanks for the response. My thought (and the reason I didn't create tests specifically for Geoacumen) is that there are actually a number of other providers of MMDB databases -- DB-IP is one, IP2Location is another, and there were a number of others that I did not make note of as they didn't fit my specific needs when I was compiling my list. There are also a number of tools available to create your own MMDB databases.

In my (admittedly subjective) experience, they tend to use the same field names as the MaxMind databases, they just don't use a DatabaseType that contains MaxMind's trademarked product names. So the existing DatabaseType checks become an artificial limitation on using third-party databases.

I'm not suggesting you aim to offer formal support for all database types; it would just be nice if we as developers had the option to swap in other databases on a "try it and see" basis without maintaining a fork just for that purpose.

sblinch avatar Dec 16 '21 19:12 sblinch

Looks reasonable. But I think it would be better to use names for functions with suffix like (By|With)Type and add documentation for this case, when developer takes responsible for correctness of database up to himself.

IncSW avatar Dec 17 '21 13:12 IncSW

Sounds good -- I've made the requested changes.

sblinch avatar Dec 21 '21 08:12 sblinch

Looks ok, conflicts should be resolved before merge.

IncSW avatar Dec 21 '21 15:12 IncSW