-
Notifications
You must be signed in to change notification settings - Fork 31
INTPYTHON-698 Allow defining embedded model indexes on the top-level model #376
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,7 +10,11 @@ Django MongoDB Backend 5.2.x | |
| New features | ||
| ------------ | ||
|
|
||
| - ... | ||
| - Added support for creating indexes from expressions. | ||
| Currently, only ``F()`` expressions are supported to reference top-level | ||
| model fields inside embedded models. | ||
|
Comment on lines
+13
to
+15
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should say something like "You can now index embedded model fields by adding an index to the parent model." and link to the new documentation.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤔 Can I use the 2nd person here? or the docs are in passive voice? |
||
|
|
||
|
|
||
|
|
||
| Bug fixes | ||
| --------- | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,3 +11,4 @@ know: | |
| embedded-models | ||
| transactions | ||
| known-issues | ||
| indexes | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| Indexes from Expressions | ||
| ======================== | ||
|
|
||
| Django MongoDB Backend now supports creating indexes from expressions. | ||
| Currently, only ``F()`` expressions are supported, which allows referencing | ||
| fields from the top-level model inside embedded fields. | ||
|
|
||
| Example:: | ||
|
|
||
| from django.db import models | ||
| from django.db.models import F | ||
|
|
||
| class Author(models.EmbeddedModel): | ||
| name = models.CharField() | ||
|
|
||
| class Book(models.Model): | ||
| author = models.EmbeddedField(Author) | ||
|
|
||
| class Meta: | ||
| indexes = [ | ||
| models.Index(F("author__name")), | ||
| ] | ||
|
Comment on lines
+1
to
+22
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of this document, probably "Indexing embedded models" should be part of that topic guide.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it could fix better. My only observation is: we are supporting expressions under (not very under) the hood. |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| import itertools | ||
|
|
||
| from django.db import connection, models | ||
| from django.db.models.expressions import F | ||
| from django.test import TransactionTestCase, skipUnlessDBFeature | ||
| from django.test.utils import isolate_apps | ||
|
|
||
|
|
@@ -519,6 +520,167 @@ class Meta: | |
| self.assertTableNotExists(Author) | ||
|
|
||
|
|
||
| class EmbeddedModelsTopLevelIndexTest(TestMixin, TransactionTestCase): | ||
| @isolate_apps("schema_") | ||
| def test_unique_together(self): | ||
| """Meta.unique_together defined at the top-level for embedded fields.""" | ||
|
|
||
| class Address(EmbeddedModel): | ||
| unique_together_one = models.CharField(max_length=10) | ||
| unique_together_two = models.CharField(max_length=10) | ||
|
|
||
| class Meta: | ||
| app_label = "schema_" | ||
|
|
||
| class Author(EmbeddedModel): | ||
| address = EmbeddedModelField(Address) | ||
| unique_together_three = models.CharField(max_length=10) | ||
| unique_together_four = models.CharField(max_length=10) | ||
|
|
||
| class Meta: | ||
| app_label = "schema_" | ||
|
|
||
| class Book(models.Model): | ||
| author = EmbeddedModelField(Author) | ||
|
|
||
| class Meta: | ||
| app_label = "schema_" | ||
| constraints = [ | ||
| models.UniqueConstraint( | ||
| F("author__unique_together_three").asc(), | ||
| F("author__unique_together_four").desc(), | ||
| name="unique_together_34", | ||
| ), | ||
| ( | ||
| models.UniqueConstraint( | ||
| F("author__address__unique_together_one"), | ||
| F("author__address__unique_together_two").asc(), | ||
| name="unique_together_12", | ||
| ) | ||
| ), | ||
| ] | ||
|
|
||
| with connection.schema_editor() as editor: | ||
| editor.create_model(Book) | ||
| self.assertTableExists(Book) | ||
| # Embedded uniques are created from top-level definition. | ||
| self.assertEqual( | ||
| self.get_constraints_for_columns( | ||
| Book, ["author.unique_together_three", "author.unique_together_four"] | ||
| ), | ||
| ["unique_together_34"], | ||
| ) | ||
| self.assertEqual( | ||
| self.get_constraints_for_columns( | ||
| Book, | ||
| ["author.address.unique_together_one", "author.address.unique_together_two"], | ||
| ), | ||
| ["unique_together_12"], | ||
| ) | ||
| editor.delete_model(Book) | ||
| self.assertTableNotExists(Book) | ||
|
|
||
| @isolate_apps("schema_") | ||
| def test_add_remove_field_indexes(self): | ||
| """AddField/RemoveField + EmbeddedModelField + Meta.indexes at top-level.""" | ||
|
|
||
| class Address(EmbeddedModel): | ||
| indexed_one = models.CharField(max_length=10) | ||
|
|
||
| class Meta: | ||
| app_label = "schema_" | ||
|
|
||
| class Author(EmbeddedModel): | ||
| address = EmbeddedModelField(Address) | ||
| indexed_two = models.CharField(max_length=10) | ||
|
|
||
| class Meta: | ||
| app_label = "schema_" | ||
|
|
||
| class Book(models.Model): | ||
| author = EmbeddedModelField(Author) | ||
|
|
||
| class Meta: | ||
| app_label = "schema_" | ||
| indexes = [ | ||
| models.Index(F("author__indexed_two").asc(), name="indexed_two"), | ||
| models.Index(F("author__address__indexed_one").asc(), name="indexed_one"), | ||
| ] | ||
|
|
||
| new_field = EmbeddedModelField(Author) | ||
| new_field.set_attributes_from_name("author") | ||
|
|
||
| with connection.schema_editor() as editor: | ||
| # Create the table and add the field. | ||
| editor.create_model(Book) | ||
| editor.add_field(Book, new_field) | ||
|
Comment on lines
+600
to
+616
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the test you copied this from needs to be fixed, but this logic isn't correct. You started with |
||
| # Embedded indexes are created. | ||
| self.assertEqual( | ||
| self.get_constraints_for_columns(Book, ["author.indexed_two"]), | ||
| ["indexed_two"], | ||
| ) | ||
| self.assertEqual( | ||
| self.get_constraints_for_columns( | ||
| Book, | ||
| ["author.address.indexed_one"], | ||
| ), | ||
| ["indexed_one"], | ||
| ) | ||
| editor.delete_model(Book) | ||
| self.assertTableNotExists(Book) | ||
|
|
||
| @isolate_apps("schema_") | ||
| def test_add_remove_field_constraints(self): | ||
| """AddField/RemoveField + EmbeddedModelField + Meta.constraints at top-level.""" | ||
|
|
||
| class Address(EmbeddedModel): | ||
| unique_constraint_one = models.CharField(max_length=10) | ||
|
|
||
| class Meta: | ||
| app_label = "schema_" | ||
|
|
||
| class Author(EmbeddedModel): | ||
| address = EmbeddedModelField(Address) | ||
| unique_constraint_two = models.CharField(max_length=10) | ||
|
|
||
| class Meta: | ||
| app_label = "schema_" | ||
|
|
||
| class Book(models.Model): | ||
| author = EmbeddedModelField(Author) | ||
|
|
||
| class Meta: | ||
| app_label = "schema_" | ||
| constraints = [ | ||
| models.UniqueConstraint(F("author__unique_constraint_two"), name="unique_two"), | ||
| models.UniqueConstraint( | ||
| F("author__address__unique_constraint_one"), name="unique_one" | ||
| ), | ||
| ] | ||
|
|
||
| new_field = EmbeddedModelField(Author) | ||
| new_field.set_attributes_from_name("author") | ||
|
|
||
| with connection.schema_editor() as editor: | ||
| # Create the table and add the field. | ||
| editor.create_model(Book) | ||
| editor.add_field(Book, new_field) | ||
| # Embedded constraints are created. | ||
| self.assertEqual( | ||
| self.get_constraints_for_columns(Book, ["author.unique_constraint_two"]), | ||
| ["unique_two"], | ||
| ) | ||
| self.assertEqual( | ||
| self.get_constraints_for_columns( | ||
| Book, | ||
| ["author.address.unique_constraint_one"], | ||
| ), | ||
| ["unique_one"], | ||
| ) | ||
| editor.delete_model(Book) | ||
| self.assertTableNotExists(Book) | ||
|
|
||
|
|
||
| class EmbeddedModelsIgnoredTests(TestMixin, TransactionTestCase): | ||
| def test_embedded_not_created(self): | ||
| """create_model() and delete_model() ignore EmbeddedModel.""" | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't analyze this complete, but it could be problematic to override this without also overriding
Index.check. This method will silently ignore unsupported indexes. The system check framework is what gives the user a warning that the index they declared isn't supported.