Opened 3 years ago

Closed 2 years ago

#20897 closed Cleanup/optimization (fixed)

Asymmetrical cursor creation in django.db.backeds.BaseDatabaseWrapper

Reported by: emil.filipov@… Owned by: nobody
Component: Database layer (models, ORM) Version: 1.5
Severity: Normal Keywords:
Cc: emil.filipov@…, timmartin Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

We currently have the following cursor creation code in BaseDatabaseWrapper:

    def cursor(self):
        self.validate_thread_sharing()
        if (self.use_debug_cursor or
            (self.use_debug_cursor is None and settings.DEBUG)):
            cursor = self.make_debug_cursor(self._cursor())
        else:
            cursor = util.CursorWrapper(self._cursor(), self)
        return cursor

The issue I have with this piece of code is that the cursor is created in a very different way, depending on whether we have a debug mode or not. The symmetrical way of doing it would be to call something like self.make_regular_cursor(self._cursor()) for the cursor creating in the regular case.

Use case:
Modules like Django Debug Toolbar wrap the .cursor() invocation on the base class, it's all working nicely for the standard backends, that do not override cursor(). The problem, however, becomes apparent when you write your own backend, that overrides the cursor() method. Then you have to call the parent method, to ensure compatibility with DDT, but that parent method will either return the generic Cursor class (for a regular cursor), or your own, custom Cursor class (for a debug cursor, where you have overridden make_debug_cursor()) - a situation requiring a decision based on the type of the returned argument.

Patch:
Fairly trivial - just add another member function and wrap util.CursorWrapper(self._cursor(), self) inside it. I can provide one if needed.

Change History (3)

comment:1 Changed 3 years ago by Anssi Kääriäinen

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Summary: Asymmetrical cursor creation in djnago.db.backeds.BaseDatabaseWrapperAsymmetrical cursor creation in django.db.backeds.BaseDatabaseWrapper
Triage Stage: UnreviewedAccepted

Why not... Please make a pull request.

comment:2 Changed 2 years ago by timmartin

Cc: timmartin added
Has patch: set

I've created a pull request at https://github.com/django/django/pull/2649

I opted to call self._cursor() in both branches and pass the result into make_regular_cursor. This isn't exactly what was suggested above, but I think it's more regular. I can't quite picture the use case described, so if there's something about this that won't satisfy the use case let me know.

comment:3 Changed 2 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: newclosed

In 27aa85246a218b0999c7c9d227eed2afb08ed510:

Fixed #20897 -- Added make_cursor() for consistent cursor creation

In django.db.backends.BaseDatabaseWrapper, pulled the creation of
cursors in the non-debug case into a separate method, in order to
make behavior more consistent when overriding the cursor creation
in derived classes.

Note: See TracTickets for help on using tickets.
Back to Top