-
Notifications
You must be signed in to change notification settings - Fork 27
fix incorrect GenericRelation joining #130
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
fix incorrect GenericRelation joining #130
Conversation
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.
Great work! Left commentary to get clarification on things
.github/workflows/test-python.yml
Outdated
repository: 'wavev/django' | ||
ref: 'add-support-join-extra-conditions' |
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 imagine this should be reverted
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 will.
django_mongodb/fields/object_id.py
Outdated
@@ -0,0 +1,30 @@ | |||
from bson import ObjectId, errors |
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.
is this file a required part of the change?
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.
This new field was used in the django library when a new model is created, not here. But we will handle those fields as string for now, so this field is not longer needed.
django_mongodb/query.py
Outdated
columns = [] | ||
for expr in extra.leaves(): | ||
if hasattr(expr, "lhs") and isinstance(expr.lhs, Col): | ||
columns.append((expr.lhs, len(lhs_fields))) |
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.
To confirm, we use len(lhs_fields) because expr.lhs will now be the newest column index, thus equal to len(lhs_fields)?
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.
Yes, it is.
django_mongodb/query.py
Outdated
columns.append((expr.lhs, len(lhs_fields))) | ||
lhs_fields.append(expr.lhs.as_mql(compiler, connection)) | ||
if hasattr(expr, "rhs") and isinstance(expr.rhs, Col): | ||
columns.append((expr.rhs, None)) |
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.
Why does rhs not get the same as lhs. Can it not be referenced by number still?
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.
You are right, it should be the same.
django_mongodb/query.py
Outdated
column_target.alias = compiler.collection_name | ||
else: | ||
column_target.target.db_column = f"{parent_template}{parent_pos}" | ||
column_target.target.set_attributes_from_name(f"{parent_template}{len(lhs_fields)}") |
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.
why do we need to use len(lhs_fields) instead of parent_pos
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.
Fixed
5bc356b
to
ac054a7
Compare
ac054a7
to
e1042f5
Compare
By adding support for Field.get_extra_restriction().
e52293a
to
78a8619
Compare
By adding support for Field.get_extra_restriction().