Opened 4 years ago
Last modified 4 years ago
#31730 assigned Bug
manage.py sqlsequencereset not implemented for sqlite3
Reported by: | axil | Owned by: | axil |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Normal | Keywords: | sql sequence reset sqlite3 |
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
Currently ./manage.py sqlsequencereset
(docs) never prints anything for the sqlite3 backend.
It happens because the function sequence_reset_sql
defined in django/django/db/backends/base/operations.py
is not implemented for the sqlite3 backend in django/django/db/backends/sqlite3/operations.py
django/django/contrib/sites/management.py
makes use of this function
# We set an explicit pk instead of relying on auto-incrementation, # so we need to reset the database sequence. See #17415. sequence_sql = connections[using].ops.sequence_reset_sql(no_style(), [Site]) if sequence_sql: if verbosity >= 2: print("Resetting sequence")
Here sequence_reset_sql
also does nothing for sqlite3, but sqlite3 is pretty lenient to
the sequences, so it didn't result in an exception.
Same applies to its usage in manage.py loaddata
: sqlite3 fixes the sequences automatically.
Leniency of sqlite is also the explanation of how django/tests/backends/tests.py:SequenceResetTest.test_generic_relation
test contrives to pass with sequence_reset_sql
being noop for sqlite3.
What it cannot do automatically is reset the sequence to zero as demonstrated in the new test
django/tests/backends/tests.py:SequenceResetTest.test_reset_sequence
def test_reset_sequence(self): Post.objects.create(name='1st post', text='hello world') Post.objects.all().delete() # Reset the sequences for the database commands = connections[DEFAULT_DB_ALIAS].ops.sequence_reset_sql(no_style(), [Post]) with connection.cursor() as cursor: for sql in commands: cursor.execute(sql) # If we create a new object now, it should have a PK greater # than the PK we specified manually. obj = Post.objects.create(name='New post', text='goodbye world') self.assertEqual(obj.pk, 1)
Change History (11)
comment:1 by , 4 years ago
comment:2 by , 4 years ago
The fix is related and partly based on the code from the recent ticket #31479 as well as on the corresponding functions for postgres and oracle.
comment:3 by , 4 years ago
Description: | modified (diff) |
---|
follow-up: 9 comment:4 by , 4 years ago
Patch needs improvement: | set |
---|---|
Triage Stage: | Unreviewed → Accepted |
TIL about the sqlsequencereset
command!
It seems to be barely tested
- https://github.com/django/django/blob/3152146e3abd029be2457d2e780599d399db0fe2/tests/empty_models/test_commands.py#L20
- https://github.com/django/django/blob/b9cf764be62e77b4777b3a75ec256f6209a57671/tests/bash_completion/tests.py#L81
And not implemented on both SQLite and MySQL!
Before any code is added here I think that we should add backend agnostics tests for the command that execute the provided SQL and assert it has the desired effects. This will likely require skipping the tests on SQLite and MySQL at first.
In a following commit the logic that identifies the sequences that need to be reset should be moved to the command itself to avoid the huge amount of boilerplate repeated in the current PostgreSQL and Oracle DatabaseOperations
implementations. That will also avoid leaking db.models
abstraction to the db.backends
ones. We should probably use connection.introspection.sequence_list()
to JOIN its return value to the specified model subset tables. In the end I think the signature of the of the sequence_reset_sql
method should be something like (style, sequences)
.
Then we should be able to add support for both SQLite and MySQL.
comment:5 by , 4 years ago
Before any code is added here
It might have come unnoticed but there is the code already ;)
https://github.com/django/django/blob/6f33f7dcf3ef20d0585bece747887c7054cf2189/django/db/backends/sqlite3/operations.py#L232-L257
I think that we should add backend agnostics tests for the command that execute the provided SQL and assert it has the desired effects.
One of the tests that I've added (django/tests/backends/tests.py:SequenceResetTest.test_reset_sequence) looks pretty backend agnostic to me:
https://github.com/axil/django/blob/7de27842c038d087257ebdd8e964f8921cd68295/tests/backends/tests.py#L186-L203
I've improved it a bit now.
In a following commit the logic that identifies the sequences that need to be reset should be moved to the command itself
Actually of the logic identifying the sequences has already been moved to the django.core.commands.sqlsequencerest:
https://github.com/django/django/blob/6f33f7dcf3ef20d0585bece747887c7054cf2189/django/core/management/commands/sqlsequencereset.py#L17-L25
or to be precise to app_config.get_models(include_auto_created=True)
. By that reason
to avoid the huge amount of boilerplate repeated in the current PostgreSQL and Oracle DatabaseOperations implementations.
this boilerplate can be cut in half as I suggest in #31731. In what is left after that yes, postgresql and oracle share the same logic and can be refactored, but sqlite needs a somewhat different SQL statement so I see little room for refactoring here.
That will also avoid leaking db.models abstraction to the db.backends ones
Yes, my patch doesn't do that.
comment:6 by , 4 years ago
Should sequence_reset_sql
automatically cascade to the autocreated many-to-many tables?
comment:7 by , 4 years ago
In the end I think the signature of the of the sequence_reset_sql method should be something like (style, sequences).
Yes, it looks strange that two similar named functions have different signatures and do very different things:
def sequence_reset_by_name_sql(self, style, sequences):
resets sequence to 1
def sequence_reset_sql(self, style, model_list):
fixes sequences to max(id)
comment:8 by , 4 years ago
Version: | 3.0 → master |
---|
comment:9 by , 4 years ago
Replying to Simon Charette:
In a following commit the logic that identifies the sequences that need to be reset should be moved to the command itself to avoid the huge amount of boilerplate repeated in the current PostgreSQL and Oracle
DatabaseOperations
implementations. That will also avoid leakingdb.models
abstraction to thedb.backends
ones. We should probably useconnection.introspection.sequence_list()
to JOIN its return value to the specified model subset tables. In the end I think the signature of the of thesequence_reset_sql
method should be something like(style, sequences)
.
I've done this part in a separate PR: https://github.com/django/django/pull/13103
So far for postgres only. Please have a look.
comment:10 by , 4 years ago
Added a deprecation warning to the sequence_reset_sql
method. Switched the tests to the new sql_refresh_sequences
function. (changeset)
comment:11 by , 4 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
Implemented and tested the sql_sequence_reset refactoring for oracle backend (changeset)
https://github.com/django/django/pull/13093