[Django] #36988: Limitation of supported GeoIP databases is too tight

[Django]

#36988: Limitation of supported GeoIP databases is too tight
----------------------+--------------------------------------
Reporter: rami | Type: Bug
Status: new | Component: GIS
Version: 6.0 | Severity: Normal
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
----------------------+--------------------------------------
Starting in Django 5.1, Django ships with a [list of allowed GeoIP
database
types](https://github.com/django/django/blame/main/django/contrib/gis/geoip2.py):

{{{
SUPPORTED_DATABASE_TYPES = {
"DBIP-City-Lite",
"DBIP-Country-Lite",
"GeoIP2-City",
"GeoIP2-Country",
"GeoLite2-City",
"GeoLite2-Country",
}
}}}

It seems weird that Django seems to be enforcing where I am getting my
databases from. We've always been using the database freely available from
[here](https://github.com/geoacumen/geoacumen-country), which has the type
"Geoacumen-Country".

Is it really intended that I need to monkeypatch Django to use this GeoIP
database from a source that is not known to Django? Should this list
extensible in some official way?
--
Ticket URL: <https://code.djangoproject.com/ticket/36988>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#36988: Limitation of supported GeoIP databases is too tight

------------------------+--------------------------------------
Reporter: rami | Owner: (none)

Type: Bug | Status: new
Component: GIS | Version: 6.0

Severity: Normal | Resolution:

Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

------------------------+--------------------------------------
Comment (by PhilS):

I have also just hit this, except I'm using the ''paid'' version of the
country database from DBIP which has a database type of `DBIP-Country` (no
`-Lite` suffix). I have difficulty believing the free version is supported
but the paid version is not.

For me personally, it would be ideal if a fix was back-ported to 5.2 LTS
as this feels like a regression from 4.2 LTS.
--
Ticket URL: <https://code.djangoproject.com/ticket/36988#comment:1>

[Django]

#36988: Limitation of supported GeoIP databases is too tight
------------------------+------------------------------------

Reporter: rami | Owner: (none)
Type: Bug | Status: new
Component: GIS | Version: 6.0
Severity: Normal | Resolution:

Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
------------------------+------------------------------------

Changes (by Tim Graham):

* stage: Unreviewed => Accepted

Comment:

The commit in question is 3fad712a91a8a8f6f6f904aff3d895e3b06b24c7. Feel
free to make a proposal, but unfortunately it's too late to backport a fix
to Django 5.2 since per our
[https://docs.djangoproject.com/en/dev/internals/release-process
/#supported-versions supported versions policy], it only receives security
fixes at this time.
--
Ticket URL: <https://code.djangoproject.com/ticket/36988#comment:2>

[Django]

#36988: Limitation of supported GeoIP databases is too tight

------------------------+---------------------------------------
Reporter: rami | Owner: SnippyCodes
Type: Bug | Status: assigned

Component: GIS | Version: 6.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

------------------------+---------------------------------------
Changes (by SnippyCodes):

* owner: (none) => SnippyCodes
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/36988#comment:3>

[Django]

#36988: Limitation of supported GeoIP databases is too tight
------------------------+---------------------------------------
Reporter: rami | Owner: SnippyCodes
Type: Bug | Status: assigned
Component: GIS | Version: 6.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
------------------------+---------------------------------------
Changes (by SnippyCodes):

* has_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/36988#comment:4>

[Django]

#36988: Limitation of supported GeoIP databases is too tight
------------------------+---------------------------------------
Reporter: rami | Owner: SnippyCodes
Type: Bug | Status: assigned
Component: GIS | Version: 6.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
------------------------+---------------------------------------

Changes (by David Smith):

* needs_better_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/36988#comment:5>

[Django]

#36988: Limitation of supported GeoIP databases is too tight
------------------------+---------------------------------------
Reporter: rami | Owner: SnippyCodes
Type: Bug | Status: assigned
Component: GIS | Version: 6.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
------------------------+---------------------------------------

Comment (by PhilS):

Looking at this more deeply (since a few suggestions in the
[https://github.com/django/django/pull/20937 PR] by @SnippyCodes have been
rejected), I'm wondering if the root cause is that the changes in proposed
in #35100 were misguided.

While that proposal simplified the code, it effectively broke the ability
to use the existing `GEOIP_PATH` with one of `GEOIP_COUNTRY` or
`GEOIP_CITY` to tell Django "this is a country DB" or "this is a city DB".
It was instead replaced with a system that tries to infer that information
from the "DB type" somehow, completely ignoring that the information as to
whether it is a city or country database may already be encoded in the
choice of those settings. See the Django 3.2 code
[https://github.com/django/django/blob/3591e1c1acbd7c13174275367c3fdf012cb0413b/django/contrib/gis/geoip2/base.py#L76-L114
here] for how it used to be handled. The repeated cycle of regressions and
fixes that introduce new regressions all seem to stem from trying to
simplify that Django 3.2 code in #35100.

Which brings me to the question of "what is the supported database list
actually for?". Because it seems like right now it's acting as a list of
"DB types" that satisfy the existing string matching being done to
determine if it is a city or country file (see
[https://github.com/django/django/blob/4b2b4bf0ac2707dc9c4d51cabfa72168eaea95fe/django/contrib/gis/geoip2.py#L136-L142
here]).

A proposed solution:

1. If the DB is provided via a `path` that points to a **file** (either
via a `path` keyword argument when instantiating `GeoIP2` or via the
`GEOIP_PATH` setting), then it must also meet the existing check via
`SUPPORTED_DATABASE_TYPES`. E.g. this maintains the current behaviour for
a path pointing directly to a file.
2. If the DB is provided via a combination of `path` that points to a
**directory** (either via a `path` argument when instantiating `GeoIP2` or
via the `GEOIP_PATH` setting), and one of `country` or `city` (either via
the `country`/`city` keyword argument when instantiating `GeoIP2` or via
the `GEOIP_COUNTRY`/`GEOIP_CITY` settings), then the
`SUPPORTED_DATABASE_TYPES` check is skipped (since as far as I can see it
doesn't serve a purpose).
3. The `GeoIP2.is_city`/`GeoIP2.is_country` properties return
`True`/`False` depending on whether the file was specified as a
country/city via the algorithm in (2) and only fall back to the existing
string matching behaviour on the "DB type" if the database was loaded via
the algorithm in (1)

This would allow users to, via instantiation arguments or the existing
global settings, provide a city or country database regardless of what the
specific format of the "DB type" is set as by the database authors, while
negating the need for monkey patching the `SUPPORTED_DATABASE_TYPES`. It
would also maintain the existing behaviour (e.g. it should be backwards
compatible)

Feedback on this idea is welcome - I may very well have missed something
--
Ticket URL: <https://code.djangoproject.com/ticket/36988#comment:6>

[Django]

#36988: Limitation of supported GeoIP databases is too tight
------------------------+---------------------------------------
Reporter: rami | Owner: SnippyCodes
Type: Bug | Status: assigned
Component: GIS | Version: 6.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
------------------------+---------------------------------------

Comment (by SnippyCodes):

Following up on the PR review, we need a way to allow users to utilize
valid paid/custom GeoIP databases (like DBIP-Country or Geoacumen-Country)
without relying on fragile substring matching or adding a new global
Django setting.
​Here are two potential API approaches for the community to consider:



**​Option A: Instance-Level Configuration**
Allow developers to pass a custom list of supported types directly when
initializing the GeoIP2 object.

​API: GeoIP2(supported_types=["DBIP-Country", "Geoacumen-Country"])
​Pros: Explicit, localized to where the object is used, doesn't pollute
global settings.
​Cons: Users might have to wrap the GeoIP2 instantiation if they use it in
multiple places across their app.



​**Option B: "Bless" the AppConfig Modification**
Keep SUPPORTED_DATABASE_TYPES as a module-level set (as it currently is),
but officially document the recommended way for users to extend it inside
their AppConfig.ready() method.

API: ```python
from django.contrib.gis.geoip2 import SUPPORTED_DATABASE_TYPES
​class MyGISAppConfig(AppConfig):
def ready(self):
SUPPORTED_DATABASE_TYPES.add("DBIP-Country")


I am happy to implement either of these, or another approach if the
community prefers. What are your thoughts?
--
Ticket URL: <https://code.djangoproject.com/ticket/36988#comment:7>

[Django]

#36988: Limitation of supported GeoIP databases is too tight
------------------------+---------------------------------------
Reporter: rami | Owner: SnippyCodes
Type: Bug | Status: assigned
Component: GIS | Version: 6.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
------------------------+---------------------------------------
Comment (by SnippyCodes):

Thank you for the incredible deep dive, @PhilS! Tracing this back to the
regressions introduced in #35100 makes perfect sense.
Your proposed solution is much cleaner than adding new settings. Trusting
the explicit country/city kwargs (or GEOIP_COUNTRY/GEOIP_CITY settings) to
bypass the strict SUPPORTED_DATABASE_TYPES check is an elegant, backward-
compatible approach. The underlying geoip2 library will still naturally
catch any invalid file formats.

I am going to pivot the PR to implement your exact algorithm. I will
update the GeoIP2 initialization logic to trust the explicit routing and
only use SUPPORTED_DATABASE_TYPES as a fallback. I'll push an updated
patch shortly!
--
Ticket URL: <https://code.djangoproject.com/ticket/36988#comment:8>

[Django]

#36988: Limitation of supported GeoIP databases is too tight
------------------------+---------------------------------------
Reporter: rami | Owner: SnippyCodes
Type: Bug | Status: assigned
Component: GIS | Version: 6.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
------------------------+---------------------------------------

Comment (by Cours granja):

Fone being rooted al wallets disabled connect to nest home on google
jetpack disabled and deprecatedReplying to [ticket:36988 rami]:

> Starting in Django 5.1, Django ships with a [list of allowed GeoIP
database
types](https://github.com/django/django/blame/main/django/contrib/gis/geoip2.py):
>
> {{{
> SUPPORTED_DATABASE_TYPES = {
> "DBIP-City-Lite",
> "DBIP-Country-Lite",
> "GeoIP2-City",
> "GeoIP2-Country",
> "GeoLite2-City",
> "GeoLite2-Country",
> }
> }}}
>
> It seems weird that Django seems to be enforcing where I am getting my
databases from. We've always been using the database freely available from
[here](https://github.com/geoacumen/geoacumen-country), which has the type
"Geoacumen-Country".
>
> Is it really intended that I need to monkeypatch Django to use this
GeoIP database from a source that is not known to Django? Should this list
extensible in some official way?

--
Ticket URL: <https://code.djangoproject.com/ticket/36988#comment:9>