#22873 closed Cleanup/optimization (fixed)

Rename BaseDatabaseWrapper.use_debug_cursor to force_debug_cursor

Reported by: john@… Owned by: anonymous
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

In Django 1.6, the logic for using the CursorDebugWrapper was:

  • If db.connection.use_debug_cursor is True, use it
  • If db.connection.use_debug_cursor is None, defer to settings.DEBUG
  • if db.connection.use_debug_cursor is False, don't use it.

This was changed in the fix for #20420. Commit [127218b92bb8] changed the logic to:

  • If db.connection.use_debug_cursor is True, use it
  • If db.connection.use_debug_cursor is False or None, defer to settings.DEBUG

I am using a management command to load lots of data into the database, and I was setting use_debug_cursor to False to disable query logging before my lengthy import. I'd rather not have to remember to set DEBUG=False every time I run a management command on my dev box. I'd like the 1.6 trinary behavior, as well as the better queries_logged property introduced in [40bfd8561d016]

Attachments (1)

trinary.patch (892 bytes) - added by john@… 15 months ago.
Proposed patch for django/db/backends/init.py

Download all attachments as: .zip

Change History (9)

Changed 15 months ago by john@…

Proposed patch for django/db/backends/init.py

comment:1 Changed 15 months ago by aaugustin

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Indeed, use_debug_cursor was a tri-valued boolean. That ranks quite high on my scale of programming anti-patterns. Furthermore, one of the three values was never used by Django.

So I changed it to a regular boolean. The new logic is:

  • If db.connection.use_debug_cursor is a true-ish value, use a debug cursor;
  • If db.connection.use_debug_cursor is a false-ish value, defer to settings.DEBUG.

None is just a false-ish value among others.

I should have changed the name to something like force_debug_cursor but I didn't find a short and explicit name so I left it as is. Suggestions?

I don't think the tri-valued boolean behaviour was introduced on purpose because it wasn't documented. It was a private API with no guarantee that its behaviour will be preserved across versions.

Accepting to fix this ticket would mean turning use_debug_cursor into a public API with its previous three-valued behaviour. I'm very close to -1 on this, but I'll let someone else confirm in case that behaviour was there for a good reason. If it was, it must be documented.

comment:2 Changed 15 months ago by john@…

This was added way back in 2010 in [5506653] by Alex Gaynor to support tests for assertNumQueries, fixing #5416. Previously, the code directly looked at settings.DEBUG. It may have been the intention to allow more control over the new debug cursor, but the most direct reason was to support the new assertion without forcing the test to run with DEBUG=True. assertNumQueries might now be broken when queries pass 9000, but that's probably rare and silly.

If use_debug_cursor it could initialized to settings.DEBUG in the __init__ method, then it could be set or cleared by code as needed. I'm not familiar with early settings loading, so it is possible that settings isn't initialized at this point.

If the current behavior is retained, force_debug_cursor is a better name, making it clearer that this is a private hook for testing.

If this undocumented API goes away, I could instead fail the management command when DEBUG=True.

comment:3 Changed 14 months ago by jwhitlock

It appears no one has anything to say about this bug. I'd support closing it w/o changes. For my own use case, I can detect and monkey patch queries_logged, and get the same desired behavior.

comment:4 Changed 14 months ago by timo

If the backwards compatibility concerns are minimal, renaming the attribute to force_debug_cursor seems like it could be a useful change to clarify the expected behavior.

comment:5 Changed 14 months ago by anonymous

  • Owner changed from nobody to anonymous
  • Status changed from new to assigned

comment:6 Changed 13 months ago by areski

I'm also in favor to rename to force_debug_cursor for clarity.
Here a pull request for this: https://github.com/django/django/pull/2968

comment:7 Changed 13 months ago by timo

  • Summary changed from Allow disabling debug cursor when DEBUG=True to Rename BaseDatabaseWrapper.use_debug_cursor to force_debug_cursor
  • Triage Stage changed from Unreviewed to Ready for checkin
  • Type changed from Bug to Cleanup/optimization

comment:8 Changed 13 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In 90faa196f67863370a4682ecfd053e189cd46705:

Fixed #22873 -- Renamed use_debug_cursor to force_debug_cursor to clarify the behavior.

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