Re: [Django] #17427: DatabaseWrapper no longer hashable-> failure in test_connections_thread_local

[Django]

#17427: DatabaseWrapper no longer hashable-> failure in
test_connections_thread_local
-+-
 Reporter:  vsajip   |Owner:  nobody
 Type:  Bug  |   Status:  closed
Component:  Database layer   |  Version:  master
  (models, ORM)  |   Resolution:  fixed
 Severity:  Normal   | Triage Stage:  Accepted
 Keywords:   |  Needs documentation:  0
Has patch:  0|  Patch needs improvement:  0
  Needs tests:  0|UI/UX:  0
Easy pickings:  0|
-+-
Changes (by Aymeric Augustin ):

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


Comment:

 In [changeset:"58de495c54c203dfa838538fb6ca46187120f4e6"]:
 {{{
 #!CommitTicketReference repository=""
 revision="58de495c54c203dfa838538fb6ca46187120f4e6"
 Fixed #17427 -- Removed dubious definition of connections equality.
 }}}

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/django-updates/064.6b633ff070f27b7d2ff735a398d941bf%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.

Re: [Django] #17427: DatabaseWrapper no longer hashable-> failure in test_connections_thread_local

[Django]

#17427: DatabaseWrapper no longer hashable-> failure in
test_connections_thread_local
-+-
 Reporter:  vsajip   |Owner:  nobody
 Type:  Bug  |   Status:  new
Component:  Database layer   |  Version:  SVN
  (models, ORM)  |   Resolution:
 Severity:  Normal   | Triage Stage:  Accepted
 Keywords:   |  Needs documentation:  0
Has patch:  0|  Patch needs improvement:  0
  Needs tests:  0|UI/UX:  0
Easy pickings:  0|
-+-

Comment (by akaariai):

 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.

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To post to this group, send email to django-updates@googlegroups.com.
To unsubscribe from this group, send email to 
django-updates+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-updates?hl=en.

Re: [Django] #17427: DatabaseWrapper no longer hashable-> failure in test_connections_thread_local

[Django]

#17427: DatabaseWrapper no longer hashable-> failure in
test_connections_thread_local
-+-
 Reporter:  vsajip   |Owner:  nobody
 Type:  Bug  |   Status:  new
Component:  Database layer   |  Version:  SVN
  (models, ORM)  |   Resolution:
 Severity:  Normal   | Triage Stage:  Accepted
 Keywords:   |  Needs documentation:  0
Has patch:  0|  Patch needs improvement:  0
  Needs tests:  0|UI/UX:  0
Easy pickings:  0|
-+-
Changes (by aaugustin):

 * type:  Uncategorized => Bug
 * stage:  Unreviewed => Accepted


Comment:

 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.

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To post to this group, send email to django-updates@googlegroups.com.
To unsubscribe from this group, send email to 
django-updates+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-updates?hl=en.

Re: [Django] #17427: DatabaseWrapper no longer hashable-> failure in test_connections_thread_local

[Django]

#17427: DatabaseWrapper no longer hashable-> failure in
test_connections_thread_local
-+-
 Reporter:  vsajip   |Owner:  nobody
 Type:  Uncategorized|   Status:  new
Component:  Database layer   |  Version:  SVN
  (models, ORM)  |   Resolution:
 Severity:  Normal   | Triage Stage:
 Keywords:   |  Unreviewed
Has patch:  0|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-

Comment (by akaariai):

 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, the transferred 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.

 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.

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To post to this group, send email to django-updates@googlegroups.com.
To unsubscribe from this group, send email to 
django-updates+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-updates?hl=en.

Re: [Django] #17427: DatabaseWrapper no longer hashable-> failure in test_connections_thread_local

[Django]

#17427: DatabaseWrapper no longer hashable-> failure in
test_connections_thread_local
-+-
 Reporter:  vsajip   |Owner:  nobody
 Type:  Uncategorized|   Status:  new
Component:  Database layer   |  Version:  SVN
  (models, ORM)  |   Resolution:
 Severity:  Normal   | Triage Stage:
 Keywords:   |  Unreviewed
Has patch:  0|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-

Comment (by vsajip):

 Replying to [comment:2 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 ...

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To post to this group, send email to django-updates@googlegroups.com.
To unsubscribe from this group, send email to 
django-updates+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-updates?hl=en.

Re: [Django] #17427: DatabaseWrapper no longer hashable-> failure in test_connections_thread_local

[Django]

#17427: DatabaseWrapper no longer hashable-> failure in
test_connections_thread_local
-+-
 Reporter:  vsajip   |Owner:  nobody
 Type:  Uncategorized|   Status:  new
Component:  Database layer   |  Version:  SVN
  (models, ORM)  |   Resolution:
 Severity:  Normal   | Triage Stage:
 Keywords:   |  Unreviewed
Has patch:  0|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-

Comment (by vsajip):

 Replying to [comment:1 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...ssE.ss
 ==
 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)
 }}}

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To post to this group, send email to django-updates@googlegroups.com.
To unsubscribe from this group, send email to 
django-updates+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-updates?hl=en.

Re: [Django] #17427: DatabaseWrapper no longer hashable-> failure in test_connections_thread_local

[Django]

#17427: DatabaseWrapper no longer hashable-> failure in
test_connections_thread_local
-+-
 Reporter:  vsajip   |Owner:  nobody
 Type:  Uncategorized|   Status:  new
Component:  Database layer   |  Version:  SVN
  (models, ORM)  |   Resolution:
 Severity:  Normal   | Triage Stage:
 Keywords:   |  Unreviewed
Has patch:  0|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-

Comment (by 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.

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To post to this group, send email to django-updates@googlegroups.com.
To unsubscribe from this group, send email to 
django-updates+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-updates?hl=en.

Re: [Django] #17427: DatabaseWrapper no longer hashable-> failure in test_connections_thread_local

[Django]

#17427: DatabaseWrapper no longer hashable-> failure in
test_connections_thread_local
-+-
 Reporter:  vsajip   |Owner:  nobody
 Type:  Uncategorized|   Status:  new
Component:  Database layer   |  Version:  SVN
  (models, ORM)  |   Resolution:
 Severity:  Normal   | Triage Stage:
 Keywords:   |  Unreviewed
Has patch:  0|  Needs documentation:  0
  Needs tests:  0|  Patch needs improvement:  0
Easy pickings:  0|UI/UX:  0
-+-
Changes (by akaariai):

 * cc: anssi.kaariainen@… (added)
 * needs_better_patch:   => 0
 * needs_tests:   => 0
 * needs_docs:   => 0


Comment:

 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... :)

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To post to this group, send email to django-updates@googlegroups.com.
To unsubscribe from this group, send email to 
django-updates+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-updates?hl=en.

[Django] #17427: DatabaseWrapper no longer hashable-> failure in test_connections_thread_local

[Django]

#17427: DatabaseWrapper no longer hashable-> failure in
test_connections_thread_local
--+
 Reporter:  vsajip|  Owner:  nobody
 Type:  Uncategorized | Status:  new
Component:  Database layer (models, ORM)  |Version:  SVN
 Severity:  Normal|   Keywords:
 Triage Stage:  Unreviewed|  Has patch:  0
Easy pickings:  0 |  UI/UX:  0
--+
 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.

-- 
Ticket URL: 
Django 
The Web framework for perfectionists with deadlines.

-- 
You received this message because you are subscribed to the Google Groups 
"Django updates" group.
To post to this group, send email to django-updates@googlegroups.com.
To unsubscribe from this group, send email to 
django-updates+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/django-updates?hl=en.