Skip to content

Comments

Add support for database types other than MaxMind#3

Open
sblinch wants to merge 6 commits intoIncSW:masterfrom
sblinch:master
Open

Add support for database types other than MaxMind#3
sblinch wants to merge 6 commits intoIncSW:masterfrom
sblinch:master

Conversation

@sblinch
Copy link

@sblinch sblinch commented Dec 15, 2021

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.

@IncSW
Copy link
Owner

IncSW commented Dec 16, 2021

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.

@sblinch
Copy link
Author

sblinch commented Dec 16, 2021

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.

@IncSW
Copy link
Owner

IncSW commented Dec 17, 2021

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.

@sblinch
Copy link
Author

sblinch commented Dec 21, 2021

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

@IncSW
Copy link
Owner

IncSW commented Dec 21, 2021

Looks ok, conflicts should be resolved before merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants