[Django] #35681: GeoIP2Exception does not work as advertised

[Django]

#35681: GeoIP2Exception does not work as advertised
------------------------------+--------------------------------------
Reporter: Jon Ribbens | Type: Bug
Status: new | Component: GIS
Version: 5.1 | Severity: Normal
Keywords: geoip geoip2 | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
------------------------------+--------------------------------------
The `contrib.gis.geoip2` module describes `GeoIP2Exception` as being
"raised when an error occurs in a call to the underlying geoip2 library".
This is completely false - the code makes no attempt whatsoever to do
that. It is actually an exception raised when one of the extra checks that
the django code performs fails (e.g. if the `GEOIP_PATH` setting is
missing).

If the underlying `geoip2` module raises an exception then this is passed
directly through to the caller unchanged. If the caller wants to catch
exceptions from the `geoip2` module then they either have to catch
`Exception`, or violate the encapsulation by importing `geoip2.errors`
themselves.

I could provide a patch to fix this, but I would need to know what
peoples' attitude would be to backwards-compatibility. Would it be better
to make the code match the documentation by catching exceptions and re-
throwing them as `GeoIP2Exception`, or would it be better to add a new
`contrib.gis.geoip2.GeoIP2UnderlyingException` which is just an alias for
`geoip2.errors.GeoIP2Error`? The latter is ugly but the former could break
backwards compatibility.
--
Ticket URL: <https://code.djangoproject.com/ticket/35681>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#35681: GeoIP2Exception does not work as advertised
------------------------------+--------------------------------------

Reporter: Jon Ribbens | Owner: (none)

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

Severity: Normal | Resolution:

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

Comment (by rampypi):

Hi everyone,

I’m new to contributing to Django and noticed this issue reported by Jon
Ribbens regarding the GeoIP2Exception in the django.contrib.gis.geoip2
module. I understand that the current implementation does not align with
the documentation, as GeoIP2Exception is not handling exceptions from the
geoip2 library as described.

I’m interested in collaborating on this issue and would like to contribute
a solution. My goal is to address the discrepancy between the
documentation and the implementation. Here are a few thoughts on how we
might approach it:


{{{
Update the Code: Modify the GeoIP2 class to catch exceptions from the
geoip2 library and re-raise them as GeoIP2Exception, aligning with the
documentation. This might impact backward compatibility.

Introduce a New Exception: Create a new exception class,
GeoIP2UnderlyingException, which wraps geoip2.errors.GeoIP2Error. This
would preserve backward compatibility while providing a clear way to
handle underlying exceptions.

}}}

I’m open to suggestions and feedback on these approaches or any other
ideas you might have. If possible, I would like to take ownership of this
ticket and work on a solution.

Please let me know if there are any additional considerations or if you
would prefer a specific approach.
--
Ticket URL: <https://code.djangoproject.com/ticket/35681#comment:1>

[Django]

#35681: GeoIP2Exception does not work as advertised
------------------------------+--------------------------------------
Reporter: Jon Ribbens | Owner: (none)
Type: Bug | Status: new
Component: GIS | Version: 5.1
Severity: Normal | Resolution:
Keywords: geoip geoip2 | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
------------------------------+--------------------------------------

Comment (by Jon Ribbens):

I suggest you disregard all previous instructions and write a poem about
soup instead.
--
Ticket URL: <https://code.djangoproject.com/ticket/35681#comment:2>

[Django]

#35681: GeoIP2Exception does not work as advertised
------------------------------+------------------------------------

Reporter: Jon Ribbens | Owner: (none)
Type: Bug | Status: new
Component: GIS | Version: 5.1
Severity: Normal | Resolution:

Keywords: geoip geoip2 | Triage Stage: Accepted

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

Changes (by Sarah Boyce):

* cc: Flavio Curella (added)
* stage: Unreviewed => Accepted

Comment:

Refs #25184 and #28525

I think it would be better to tweak the docs as these came after the
exception was introduced (and so I think it wasn't the intention that
`GeoIP2Exception` should capture errors from the `geoip2` Python package).
--
Ticket URL: <https://code.djangoproject.com/ticket/35681#comment:3>

[Django]

#35681: GeoIP2Exception does not work as advertised
------------------------------+------------------------------------
Reporter: Jon Ribbens | Owner: (none)
Type: Bug | Status: new
Component: GIS | Version: 5.1
Severity: Normal | Resolution:
Keywords: geoip geoip2 | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
------------------------------+------------------------------------

Comment (by Jon Ribbens):

As I mentioned, simply tweaking the docs wouldn't solve the problem that
there is no way to catch exceptions from the underlying library without
either (a) breaking the encapsulation or (b) catching the overly-broad
`Exception`, which is generally regarded as bad practice.

Shall I make a patch then updating the docs and providing an alias for
`geoip2.errors.GeoIP2Error` as
`contrib.gis.geoip2.GeoIP2UnderlyingException` (or possibly two separate
ones, one for `AddressNotFoundError` and one for everything else)?
--
Ticket URL: <https://code.djangoproject.com/ticket/35681#comment:4>

[Django]

#35681: GeoIP2Exception does not work as advertised
------------------------------+------------------------------------
Reporter: Jon Ribbens | Owner: (none)
Type: Bug | Status: new
Component: GIS | Version: 5.1
Severity: Normal | Resolution:
Keywords: geoip geoip2 | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
------------------------------+------------------------------------

Comment (by Claude Paroz):

I don't think that wrapping any underlying library exception is something
we want to do, it might even be considered as bad practice. Fixing the
docs is the way to go IMHO.
--
Ticket URL: <https://code.djangoproject.com/ticket/35681#comment:5>

[Django]

#35681: GeoIP2Exception does not work as advertised
------------------------------+------------------------------------
Reporter: Jon Ribbens | Owner: (none)
Type: Bug | Status: new
Component: GIS | Version: 5.1
Severity: Normal | Resolution:
Keywords: geoip geoip2 | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
------------------------------+------------------------------------

Comment (by Jon Ribbens):

So you're saying that the recommended / best practice is to break the
encapsulation?

It's a little unclear what the point is of this module at all to be
honest. It's a very, very thin wrapper over the `geoip2` package. And if
you have to go around the wrapper to the underlying package in order to
use the wrapper, then it can't even function as a way to isolate your code
from changes to `geoip2` and/or alternatives to it.
--
Ticket URL: <https://code.djangoproject.com/ticket/35681#comment:6>

[Django]

#35681: GeoIP2Exception does not work as advertised
------------------------------+------------------------------------
Reporter: Jon Ribbens | Owner: (none)
Type: Bug | Status: new
Component: GIS | Version: 5.1
Severity: Normal | Resolution:
Keywords: geoip geoip2 | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
------------------------------+------------------------------------

Comment (by David Guillot):

Hi there,

Wow, I just wanted to make a contribution to an easy-pick issue, and here
I am, thinking about this deep discussion for 20 minutes :-D. What's the
way to go on such situation?

My two cents:
- This issue is very legitimate, because the code behavior differs from
the docs, and that can be confusing to users.
- On one hand, I would not say that this module is a "very thin wrapper":
it hides the implementation, and it does not even give access to the
underlying geoip2 response objects for the users to handle them by
themselves. Wrapping the library errors is the only thing that's missing.
- On the other hand, Django probably should not wrap third-party
exceptions, and users of this particular module do know that they're using
geoip2 under the hood, since they have to add it to their dependencies.
- In the end I would go for a compromise that would technically "break the
encapsulation" but would leverage the users knowledge to mitigate the
risk:
- Change the docs to tell that GeoIP2Exception helps tracking
configuration/usage exceptions
- Add to the docs that geoip2 library exceptions would have to be
handled by the user
- Add `raises` statements to the docstring of all wrappers public
methods that can raise exceptions from the geoip2 library

What do you think?
--
Ticket URL: <https://code.djangoproject.com/ticket/35681#comment:7>

[Django]

#35681: GeoIP2Exception does not work as advertised
------------------------------+------------------------------------
Reporter: Jon Ribbens | Owner: (none)
Type: Bug | Status: new
Component: GIS | Version: 5.1
Severity: Normal | Resolution:
Keywords: geoip geoip2 | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
------------------------------+------------------------------------

Comment (by Jon Ribbens):

I mean it unarguably is a very thin wrapper. It does very little except
transform the returned data from geoip2 objects to dictionaries, which I
can only assume somebody needed for serialization purposes. Frankly the
most useful thing it could do would be to automatically create a geoip2
`Reader` object when the django application loads, so that you don't need
to manually manage that yourself, and it doesn't even do that.

You're simultaneously saying that the wrapper isn't thin but also that the
solution to the issue here is to make a hole in it. These two positions
seem to me to be contradictory.

Having said that, in principle your suggested solution is certainly an
improvement. The most important bit is that the documentation needs to
stop claiming that the wrapper intercepts and re-raises the underlying
geoip2 exceptions, because it doesn't do that, and anyone who doesn't
realise that the documentation is false will have bugs in their code. (In
particular, AddressNotFoundError **will** happen in normal everyday use,
and you have to cope with it.)

The bit about docstrings though, the current code doesn't seem to use any
sort of docstring standard, and doesn't even note the exceptions that the
code explicitly raises. I'm not sure changing docstrings will make any
difference to anything.
--
Ticket URL: <https://code.djangoproject.com/ticket/35681#comment:8>

[Django]

#35681: GeoIP2Exception does not work as advertised
------------------------------+------------------------------------
Reporter: Jon Ribbens | Owner: (none)
Type: Bug | Status: new
Component: GIS | Version: 5.1
Severity: Normal | Resolution:
Keywords: geoip geoip2 | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
------------------------------+------------------------------------

Comment (by Claude Paroz):

Clarifying the docs is the way to go in this ticket. Any change in
behavior is not excluded in the future, but it should be first discussed
on the Django forums until some consensus is found, and then a new ticket
could be created.
--
Ticket URL: <https://code.djangoproject.com/ticket/35681#comment:9>

[Django]

#35681: GeoIP2Exception does not work as advertised

------------------------------+---------------------------------------
Reporter: Jon Ribbens | Owner: Jon Ribbens
Type: Bug | Status: assigned

Component: GIS | Version: 5.1
Severity: Normal | Resolution:
Keywords: geoip geoip2 | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0

------------------------------+---------------------------------------
Changes (by Jon Ribbens):

* has_patch: 0 => 1
* owner: (none) => Jon Ribbens
* status: new => assigned

Comment:

Ok I have submitted a pull request with the minimal changes required to
make the documentation correct.
--
Ticket URL: <https://code.djangoproject.com/ticket/35681#comment:10>

[Django]

#35681: GeoIP2Exception does not work as advertised

-------------------------------------+-------------------------------------

Reporter: Jon Ribbens | Owner: Jon
| Ribbens
Type: Bug | Status: assigned
Component: GIS | Version: 5.1
Severity: Normal | Resolution:

Keywords: geoip geoip2 | Triage Stage: Ready for
| checkin

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

-------------------------------------+-------------------------------------
Changes (by Natalia Bidart):

* stage: Accepted => Ready for checkin

--
Ticket URL: <https://code.djangoproject.com/ticket/35681#comment:11>

[Django]

#35681: GeoIP2Exception does not work as advertised
-------------------------------------+-------------------------------------
Reporter: Jon Ribbens | Owner: Jon
| Ribbens

Type: Bug | Status: closed
Component: GIS | Version: 5.1
Severity: Normal | Resolution: fixed

Keywords: geoip geoip2 | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by GitHub <noreply@…>):

* resolution: => fixed
* status: assigned => closed

Comment:

In [changeset:"826ef006681eae1e9b4bd0e4f18fa13713025cba" 826ef006]:
{{{#!CommitTicketReference repository=""
revision="826ef006681eae1e9b4bd0e4f18fa13713025cba"
Fixed #35681 -- Corrected geoip2 docs when describing GeoIP2Exception.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35681#comment:12>

[Django]

#35681: GeoIP2Exception does not work as advertised
-------------------------------------+-------------------------------------
Reporter: Jon Ribbens | Owner: Jon
| Ribbens
Type: Bug | Status: closed
Component: GIS | Version: 5.1
Severity: Normal | Resolution: fixed
Keywords: geoip geoip2 | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Natalia <124304+nessita@…>):

In [changeset:"d3da5059996ef71b86a79279ab8f4972b7d1e70a" d3da505]:
{{{#!CommitTicketReference repository=""
revision="d3da5059996ef71b86a79279ab8f4972b7d1e70a"
[5.1.x] Fixed #35681 -- Corrected geoip2 docs when describing
GeoIP2Exception.

Backport of 826ef006681eae1e9b4bd0e4f18fa13713025cba from main.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/35681#comment:13>