[Django] #29706: Issue with RenameContentType _rename method transaction wrapping
Django
-------------------------------------+-------------------------------------
Reporter: Tyler | Owner: nobody
Morgan |
Type: Bug | Status: new
Component: | Version: master
contrib.contenttypes | Keywords: RenameContentType
Severity: Normal | transaction atomic content_type
Triage Stage: | update_fields using
Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 1
UI/UX: 0 |
-------------------------------------+-------------------------------------
**The commit in question:**
https://github.com/django/django/commit/f179113e6cbc8ba0a8d4e87e1d4410fb61d63e75
**The specific lines in question:**
{{{
with transaction.atomic(using=db):
content_type.save(update_fields={'model'})
}}}
**The issue:**
For some background, we run a dynamic database router and have no "real"
databases configured in the settings file, just a default sqlite3 backend
which is never actually generated or used. We forked the migrate.py
management command and modified it to accept a dictionary containing
database connection parameters as the --database argument.
The dynamic database router is based on, and very similar to this:
https://github.com/ambitioninc/django-dynamic-db-
router/blob/master/dynamic_db_router/router.py
This has worked beautifully for all migrations up until this point.
The issue we're running into is that when attempting to run a migration
which contains a call to `migrations.RenameModel`, and while specifying
the database parameters to the migrate command, the migration fails with
an `OperationalError`, stating that `no such table: django_content_types
exists`.
After having exhaustively stepped through the traceback, it appears that
even though the `content_type.save` call is wrapped in the `with
transaction.atomic(using=db)` context manager, the actual database
operation is being attempted on the default database (which in our case
does not exist) rather than the database specified via
schema_editor.connection.alias (on line 15 of the same file) and thus
fails loudly.
So, I believe that:
{{{
content_type.save(update_fields={'model'})
}}}
should be
{{{
content_type.save(using=db, update_fields={'model'})
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29706>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: | Version: master
contrib.contenttypes |
Keywords: RenameContentType | Triage Stage:
transaction atomic content_type | Unreviewed
update_fields using |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Comment:
Added a pull request with the fix.
https://github.com/django/django/pull/10332
--
Ticket URL: <https://code.djangoproject.com/ticket/29706#comment:1>
Django
database
-------------------------------------+-------------------------------------
Reporter: Tyler Morgan | Owner: nobody
Component: | Version: master
contrib.contenttypes |
Keywords: RenameContentType | Triage Stage: Accepted
transaction atomic content_type |
update_fields using |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Tim Graham):
* stage: Unreviewed => Accepted
* needs_tests: 0 => 1
* easy: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/29706#comment:2>
Django
database
-------------------------------------+-------------------------------------
Reporter: Tyler Morgan | Owner: nobody
Component: | Version: master
contrib.contenttypes |
Keywords: RenameContentType | Triage Stage: Accepted
transaction atomic content_type |
update_fields using |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Herbert Fortes (added)
Comment:
Hi,
I spent all afternoon into this ticket. As I am a newbie I failed to
make a test for it. And the fix can really be 'using=db' in the .save()
method.
But I found a StackOverflow answer about transaction.atomic(using=db)
that is interesting. It does not work (Django 1.10.5).
[https://stackoverflow.com/questions/41866795/multi-db-transactions,
multi-db-transactions]
The reason given is that the router is responsible for directions.
Tyler Morgan probably read the docs but I am putting it here in case
for a steps review. It has an
[https://docs.djangoproject.com/en/2.1/topics/db/multi-db/#topics-db-
multi-db-routing, example purpose only]
--
Ticket URL: <https://code.djangoproject.com/ticket/29706#comment:3>
Django
database
-------------------------------------+-------------------------------------
Reporter: Tyler Morgan | Owner: nobody
Component: | Version: master
contrib.contenttypes |
Keywords: RenameContentType | Triage Stage: Accepted
transaction atomic content_type |
update_fields using |
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Simon Charette):
Hebert, the solution proposed by Tyler on the PR looks appropriate to me.
The `using=db` kwarg has to be passed to `save()` for the same reason
explained in the SO page you linked to; `transaction.atomic(using)`
doesn't route any query and `save(using)` skips query routing.
--
Ticket URL: <https://code.djangoproject.com/ticket/29706#comment:4>
Django
database
-------------------------------------+-------------------------------------
Reporter: Tyler Morgan | Owner: nobody
Component: | Version: master
contrib.contenttypes |
Keywords: RenameContentType | Triage Stage: Accepted
transaction atomic content_type |
update_fields using |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Herbert Fortes):
Hi,
First, I shoud went to mentors instead of comment here.
I did not make the test. Back to the ticket: " a default sqlite3
backend which is never actually generated or used. ". If it is
not generated I thought: How get info from there? Then I did
a search.
There is no '.save(using)' in the StackOverflow I linked. If no 'using' is
set
it fails. Then the guy tried two 'transaction.atomic(using='db1')' - and
'db2'
togheter. It worked. First, the decorator (worked). Second, the 'with'
(did
nothing).
{{{
@transaction.atomic(using='db1')
def edit(self, context):
"""Edit
:param dict context: Context
:return: None
"""
# Check if employee exists
try:
result = Passport.objects.get(pk=self.user.employee_id)
except Passport.DoesNotExist:
return False
result.name = context.get('name')
with transaction.atomic(using='db2'):
result.save()
}}}
"In the above example I even made another transaction inside
of the initial transaction but this time using='db2' where the model
doesn't even exist. I figured that would have failed, but it didn't and
the data was written to the proper database."
It is ok to '.save(using=db)'. I used the 'can', probably not the right
term.
Next time I go to mentors. To not make confusion in the ticket.
Regards,
Herbert
--
Ticket URL: <https://code.djangoproject.com/ticket/29706#comment:5>
Django
database
-------------------------------------+-------------------------------------
| Ramezani
Type: Bug | Status: assigned
Component: | Version: master
contrib.contenttypes |
Severity: Normal | Resolution:
Keywords: RenameContentType | Triage Stage: Accepted
transaction atomic content_type |
update_fields using |
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* owner: nobody => Hasan Ramezani
* status: new => assigned
* has_patch: 1 => 0
* needs_tests: 1 => 0
Comment:
PR[https://github.com/django/django/pull/11066]
--
Ticket URL: <https://code.djangoproject.com/ticket/29706#comment:6>
Django
database
-------------------------------------+-------------------------------------
Reporter: Tyler Morgan | Owner: Hasan
| Ramezani
Type: Bug | Status: assigned
Component: | Version: master
contrib.contenttypes |
Severity: Normal | Resolution:
Keywords: RenameContentType | Triage Stage: Accepted
transaction atomic content_type |
update_fields using |
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/29706#comment:7>
Django
database
-------------------------------------+-------------------------------------
Reporter: Tyler Morgan | Owner: Hasan
| Ramezani
Component: | Version: master
contrib.contenttypes |
Severity: Normal | Resolution: fixed
Keywords: RenameContentType | Triage Stage: Accepted
transaction atomic content_type |
update_fields using |
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"661e6cc2c97d9bcb45198be787409488e1825c90" 661e6cc]:
{{{
#!CommitTicketReference repository=""
revision="661e6cc2c97d9bcb45198be787409488e1825c90"
Fixed #29706 -- Made RenameContentType._rename() save to the correct
database.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29706#comment:8>