-
Notifications
You must be signed in to change notification settings - Fork 28
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?
INTPYTHON-698 Allow defining embedded model indexes on the top-level model #376
Conversation
998a080
to
5dd116b
Compare
django_mongodb_backend/schema.py
Outdated
# Remove the top level indexes. | ||
# TODO: Find a workaround | ||
for index in model._meta.indexes: | ||
if any( | ||
field_name.startswith(f"{field.column}{LOOKUP_SEP}") | ||
for field_name in index.fields | ||
): | ||
self.remove_index(model, index) | ||
for constraint in model._meta.constraints: | ||
if any( | ||
field_name.startswith(f"{field.column}{LOOKUP_SEP}") | ||
for field_name in constraint.fields | ||
): | ||
self.get_collection(model._meta.db_table).drop_index(constraint.name) |
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 would think this is unnecessary since the Index/Constraint would have to be removed from the model's Meta.indexes/constraints
before removing the field.
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.
🤔 Oki, I will try to move the logic into _remove_model_indexes
and _remove_field_unique
. The downfall is I have to pass all the indexes from its parents. Like concatenating all way down the recursion. The index could have been created in anywhere of the hierarchy.
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 think it's simpler than you describe. Example:
class Book(models.Model):
author = EmbeddedModelField(Author)
class Meta:
app_label = "schema_"
indexes = [
models.Index(fields=["author__indexed_two"]),
models.Index(fields=["author__address__indexed_one"]),
]
I removing the an index, the generated migration operation is:
migrations.RemoveIndex(
model_name='book',
name='embed_index_author._f84329_idx',
),
The logic you describe isn't needed.
django_mongodb_backend/lookups.py
Outdated
class Options(base.Options): | ||
def get_field(self, field_name): | ||
if LOOKUP_SEP in field_name: | ||
previous = self | ||
keys = field_name.split(LOOKUP_SEP) | ||
path = [] | ||
for field in keys: | ||
field = base.Options.get_field(previous, field) | ||
if isinstance(field, EmbeddedModelField): | ||
previous = field.embedded_model._meta | ||
else: | ||
previous = field | ||
path.append(field.column) | ||
column = ".".join(path) | ||
embedded_column = field.clone() | ||
embedded_column.column = column | ||
return embedded_column | ||
return super().get_field(field_name) |
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.
Monkey patching at such a low level seems risky, though I haven't much about what could go wrong. Instead I imagined an Index
subclass with this sort of logic. Did you consider it?
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.
🤔 Maybe is feasible if we create something like EMFIndex. I didn't considered yet. I could take a look.
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.
mmh this way (monkey patching) will not work, I forgot that foreign field, lookups and many other things use this. So isn't very straightforward. I have to think another way to solve this
3e722d1
to
2010776
Compare
369e6b5
to
d5b415d
Compare
d5b415d
to
a775af8
Compare
f70d0f6
to
32a1712
Compare
Index are defined with __ for EMF