Rename BaseDatabaseWrapper.use_debug_cursor to force_debug_cursor

[john@…]

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]

[john@…]

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

[Aymeric Augustin]

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.

[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.

[John Whitlock]

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.

[Tim Graham]

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.

[Areski Belaid]

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

[Tim Graham <timograham@…>]

In 90faa196f67863370a4682ecfd053e189cd46705:

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