Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refs #27064 -- Added RenameIndex migration operation. #15677

Merged
merged 1 commit into from
May 12, 2022

Conversation

David-Wobrock
Copy link
Member

@David-Wobrock David-Wobrock commented May 9, 2022

Adds for https://code.djangoproject.com/ticket/27064 the RenameIndex operation

Split from #15651. Here we implement the operation, in #15651 we use it in the autodetector.
This PR:

  • Added the RenameIndex operation, that nearly always has 2 behaviours for each method: 1/ renaming with an old_name or renaming an unnamed index with old_fields
  • With its state forward, that either modifies the indexes or both indexes + index_together
  • And the database forward/backward operations: which will either try to introspect the indexes to fetch the name, or simply fetch the Index
  • Added related unit tests
  • Added the supports_index_renaming feature flag on DB backends, to know if one can use a RENAME INDEX database statement or needs to drop/create the index
  • Adapted the documentation

Copy link
Member

@felixxm felixxm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@David-Wobrock Thanks 👍 I left comments.

django/db/migrations/operations/models.py Outdated Show resolved Hide resolved
django/db/migrations/operations/models.py Outdated Show resolved Hide resolved
django/db/migrations/operations/models.py Outdated Show resolved Hide resolved
django/db/migrations/state.py Show resolved Hide resolved
docs/ref/migration-operations.txt Outdated Show resolved Hide resolved
tests/migrations/test_operations.py Show resolved Hide resolved
tests/migrations/test_operations.py Outdated Show resolved Hide resolved
@David-Wobrock
Copy link
Member Author

Thanks a lot for the review @felixxm 🙏
I amended the commit, where I:

  • removed docstrings and added the default arguments for the RenameIndex docs
  • added tests for error messages and changed them to ValueErrors
  • added a reduce function and a unit test for it

@felixxm felixxm changed the title Refs #27064 -- Implemented RenameIndex operation Refs #27064 -- Added RenameIndex migration operation. May 12, 2022
Copy link
Member

@felixxm felixxm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@David-Wobrock Thanks for updates 👍 I rebased and pushed some edits.

@@ -27,6 +27,7 @@ class DatabaseSchemaEditor(BaseDatabaseSchemaEditor):
)
sql_delete_table = "DROP TABLE %(table)s CASCADE CONSTRAINTS"
sql_create_index = "CREATE INDEX %(name)s ON %(table)s (%(columns)s)%(extra)s"
sql_renamed_index = "ALTER INDEX %(old_name)s RENAME TO %(new_name)s"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo:

Suggested change
sql_renamed_index = "ALTER INDEX %(old_name)s RENAME TO %(new_name)s"
sql_rename_index = "ALTER INDEX %(old_name)s RENAME TO %(new_name)s"

)
if len(matching_index_name) != 1:
raise ValueError(
f"Found wrong number ({len(matching_index_name)}) of indexes for "
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

f-strings should not contain function calls. This guideline is from Python coding style.

if self.old_fields:
from_model = from_state.apps.get_model(app_label, self.model_name)
matching_index_name = schema_editor._constraint_names(
from_model, column_names=self.old_fields, index=True
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Users would use field names not column names 🤔 we must take this into account, e.g.

columns = [from_model._meta.get_field(field).column for field in self.old_fields]

Comment on lines 3025 to 3066
def test_rename_index_reduction(self):
operation_one = migrations.RenameIndex(
"Pony", new_name="mid_name", old_fields=("weight", "pink")
)
operation_two = migrations.RenameIndex(
"Pony", new_name="new_name", old_name="mid_name"
)
result_operations = operation_one.reduce(operation_two, [])
self.assertEqual(len(result_operations), 1)
self.assertEqual(
result_operations[0].deconstruct(),
migrations.RenameIndex(
"Pony",
new_name="new_name",
old_fields=("weight", "pink"),
).deconstruct(),
)

operation_one = migrations.RenameIndex(
"Pony", new_name="mid_name", old_name="old_name"
)
operation_two = migrations.RenameIndex(
"Pony", new_name="new_name", old_name="mid_name"
)
result_operations = operation_one.reduce(operation_two, [])
self.assertEqual(len(result_operations), 1)
self.assertEqual(
result_operations[0].deconstruct(),
migrations.RenameIndex(
"Pony", new_name="new_name", old_name="old_name"
).deconstruct(),
)

operation_one = migrations.RenameIndex(
"Pony", new_name="mid_name", old_name="old_name"
)
operation_two = migrations.RenameIndex(
"Pony", new_name="new_name", old_fields=("weight", "pink")
)
result_operations = operation_one.reduce(operation_two, [])
self.assertIs(result_operations, False)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to test_optimizer.

@David-Wobrock
Copy link
Member Author

Awesome, thanks a lot for the edits @felixxm 🚀

docs/releases/4.1.txt Outdated Show resolved Hide resolved
@felixxm felixxm merged commit eacd497 into django:main May 12, 2022
@felixxm felixxm temporarily deployed to schedules May 13, 2022 03:25 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants