-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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: change sqla is_relationship for support sqla<=2.0.0 #2357
fix: change sqla is_relationship for support sqla<=2.0.0 #2357
Conversation
how about this PR? > @flask-admin maintainer |
Is it possible to merge this PR? Relationships are not fully usable in SQLA2 currently |
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.
In #2332 there is a commit aee829a that adds model.registry.configure()
. This is described in https://docs.sqlalchemy.org/en/20/orm/mapping_api.html#sqlalchemy.orm.registry.configure.
I'm no SQLAlchemy expert, only saying that this stopped the test exceptions and looks like it works with the example in the linked issue.
@@ -224,7 +224,8 @@ def is_hybrid_property(model, attr_name): | |||
|
|||
|
|||
def is_relationship(attr): | |||
return hasattr(attr, 'property') and hasattr(attr.property, 'direction') | |||
return (hasattr(attr, 'property') and hasattr(attr.property, 'direction') # sqla<2.0.0 | |||
or (hasattr(attr, '_is_relationship') and attr._is_relationship)) # sqla>=2.0.0 |
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.
Are the attributes starting with _
documented?
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 this is not documented.
Actually I just found _is_relationship when debugging. this is reason why I use _is_relationship.
if there is a solution that documented (model.registry.configure()), the solution is best!
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.
Safer I guess anyway, better or best? I found another way #2398. Replacing mapper.iterate_properties
with mapper.attrs
, which does a check to see if model.registry.configure() has been done. But that does seem to have been around for a while, why iterate_properties was used in the first place I do not know.
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'm also not familiar with SQLAlchemy, a just user of flask-admin, sqlalchemy. So I follow mantainer's decistion. ;)
@cjmayo do we still need this for sqlalchemy 1/2 compatibility? |
I think the merged #2398 has addressed this problem. |
close #2345