DatabaseWrapper no longer hashable-> failure in test_connections_thread_local

[Vinay Sajip]

The resolution of #17258 leads to a failure under Python 3.x because the SQLite DatabaseWrapper is no longer hashable. I noticed that the BaseDatabaseWrapper defines __eq__ but not __hash__ - does this not prevent its instances from being keys in dictionaries / members of sets?

If you add __hash__ = object.__hash__ in BaseDatabaseWrapper, then the problem should go away.

[Anssi Kääriäinen]

Is the error that the DBWrapper is used in a dict/set somewhere and that errors, or just that if __eq__ is defined, then __hash__ must be defined, too. If it is used in a hash somewhere, could you provide a stack trace? There might be a bigger problem hidden...

It is a bit strange that changing BaseDatabaseWrapper from threading.local to object makes it non-hashable in Python 3.x. But that isn't the strangest thing about threading.local objects... :)

[Alex Gaynor]

Is it sane to use a DBWrapper as a dict key? Unless there's a sane use case I think __hash__ = None might be a saner choice.

[Vinay Sajip]

Replying to akaariai:

Is the error that the DBWrapper is used in a dict/set somewhere and that errors, or just that if __eq__ is defined, then __hash__ must be defined, too. If it is used in a hash somewhere, could you provide a stack trace? There might be a bigger problem hidden...

Here's a stack trace:

PYTHONPATH=.. python3.2 runtests.py --settings test_sqlite --noinput backends 2>&1 | tee ~/django3-sqlite-3.2.log
...s...s....s....E.....ssssss
======================================================================
ERROR: test_connections_thread_local (regressiontests.backends.tests.ThreadTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/vinay/projects/django3/tests/regressiontests/backends/tests.py", line 488, in test_connections_thread_local
    connections_set.add(conn)
TypeError: unhashable type: 'DatabaseWrapper'

----------------------------------------------------------------------
Ran 29 tests in 0.244s

FAILED (errors=1, skipped=9)

[Vinay Sajip]

Replying to Alex:

Is it sane to use a DBWrapper as a dict key? Unless there's a sane use case I think __hash__ = None might be a saner choice.

It seems to me that as an alternative to using a set, one could use the alias as a key and use a dict instead ...

[Anssi Kääriäinen]

I did a little investigation, and to me it seems the correct fix is to not define __eq__ or __hash__ at all, that is, they are inherited from object, and thus only same instance is equal.

The current equality test using alias isn't a sane test anymore. Previously you had to try extra-hard to get two different instances in one thread to point to the same alias. Now this is much easier to do. So, if you have two DBWrappers pointing to the same alias but using different connections, I don't see a reason why they should be equal. Other possibility would be to base the the equality on the underlying connection equality, that is self.connection == other.connection.

Previously if you transferred the DBWrapper in connections[some_alias] from thread 1 to thread 2, then the transferred DBWrapper and connections[some_alias] DBWrapper would have pointed to the same connection in thread 2 (because the DBWrapper instance was threading.local). So, if you tested self.alias == other.alias you would be testing that the connection is the same, too. This is no longer true. Now the transferred DBWrapper would have a different connection.

I hope the above explanation makes some sense... Making this change will require some minor changes to Django code. But the bigger deal is if this breaks user code in backwards incompatible way. User code relying on .alias equality is no longer correct, and instance equality will give the same result as long as you don't transfer connections between threads or anything like that. So, no problems should be caused to users by this.

[Aymeric Augustin]

Like Alex I'd prefer a rewrite of this test to avoid hashing connections.

Storing a set of id(connection) instead of a set of connection should do the job.

[Anssi Kääriäinen]

Yes, that is the obvious fix for the immediate problem.

I still think we should change the __eq__ because it is no longer doing the right thing. There are two cases where it fails:

• Send a connection to another thread, then test equality in the other thread. Alias equality doesn't produce correct results in this case.
• Create a new connection for some alias. Assign it to connections[alias]. If I am not mistaken, there is at least one place in the ORM where this will result in a check passing where it should not (subquery as_sql()).

Both of those are rare. Both of those are impossible to hit using public API. But I don't see the point of keeping the current __eq__ operator. I can't see a single case where alias equality is wanted instead of id equality. Maybe there are cases where alias equality will produce the correct result, but equality based on id doesn't. If that is the case, then by all means lets keep the current __eq__.

Both of the above conditions were impossible or at least very hard to hit before moving the threading.local from DBWrapper to django.db.connections.

[Aymeric Augustin <aymeric.augustin@…>]

In 58de495c54c203dfa838538fb6ca46187120f4e6:

Fixed #17427 -- Removed dubious definition of connections equality.