-
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 |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |
|
|
||
| from django.db.backends.base.schema import BaseDatabaseSchemaEditor | ||
| from django.db.models import Index, UniqueConstraint | ||
| from django.db.models.expressions import F, OrderBy | ||
| from pymongo.operations import SearchIndexModel | ||
|
|
||
| from django_mongodb_backend.indexes import SearchIndex | ||
|
|
@@ -351,6 +352,35 @@ def _remove_field_index(self, model, field, column_prefix=""): | |
| ) | ||
| collection.drop_index(index_names[0]) | ||
|
|
||
| def _check_expression_indexes_applicable(self, expressions): | ||
| return all( | ||
| isinstance(expression.expression if isinstance(expression, OrderBy) else expression, F) | ||
| for expression in expressions | ||
| ) | ||
|
|
||
| def _unique_supported( | ||
| self, | ||
| condition=None, | ||
| deferrable=None, | ||
| include=None, | ||
| expressions=None, | ||
| nulls_distinct=None, | ||
| ): | ||
|
Comment on lines
+361
to
+368
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 didn't analyze this complete, but it could be problematic to override this without also overriding |
||
| return ( | ||
| (not condition or self.connection.features.supports_partial_indexes) | ||
| and (not deferrable or self.connection.features.supports_deferrable_unique_constraints) | ||
| and (not include or self.connection.features.supports_covering_indexes) | ||
| and ( | ||
| not expressions | ||
| or self.connection.features.supports_expression_indexes | ||
| or self._check_expression_indexes_applicable(expressions) | ||
| ) | ||
| and ( | ||
| nulls_distinct is None | ||
| or self.connection.features.supports_nulls_distinct_unique_constraints | ||
| ) | ||
| ) | ||
|
Comment on lines
+361
to
+382
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 copies much from the base class. It needs a comment explaining the method is is overridden. I guess it's about |
||
|
|
||
| @ignore_embedded_models | ||
| def add_constraint(self, model, constraint, field=None, column_prefix="", parent_model=None): | ||
| if isinstance(constraint, UniqueConstraint) and self._unique_supported( | ||
|
|
@@ -361,6 +391,7 @@ def add_constraint(self, model, constraint, field=None, column_prefix="", parent | |
| nulls_distinct=constraint.nulls_distinct, | ||
| ): | ||
| idx = Index( | ||
| *constraint.expressions, | ||
| fields=constraint.fields, | ||
| name=constraint.name, | ||
| condition=constraint.condition, | ||
|
|
@@ -391,6 +422,7 @@ def remove_constraint(self, model, constraint): | |
| nulls_distinct=constraint.nulls_distinct, | ||
| ): | ||
| idx = Index( | ||
| *constraint.expressions, | ||
| fields=constraint.fields, | ||
| name=constraint.name, | ||
| condition=constraint.condition, | ||
|
|
||
| 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.
It seems to me this should be in
indexes.pyrather than the expressions module?