#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 <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To view this discussion visit
https://groups.google.com/d/msgid/django-updates/0107019d063dbac0-d4964483-cdec-426c-b00d-43891e98a9c3-000000%40eu-central-1.amazonses.com.
