Skip to content

Conversation

@WhatmoreCB
Copy link

is_hybrid_property method failing when called by scaffold_filters method on ModelView class when adding a grandchild column name to column_filters e.g column_filters = ('children.grandchild.name')

Added additional if statement to convert the last_model class back to a sqlalchemy model class

Noticed that this issue was raised on another pull request but poster hasn't responded: #2390

maintainer asks to add a test that fails without the changes and that passes with the changes, upon writing a test I found that it's due to the backref arg when defining relationships in the model, not sure where to put the test file so will add as an additional comment

is_hybrid_property method failing when called by scaffold_filters method on ModelView class when adding a grandchild column name to column_filters e.g column_filters = ('children.grandchild.name')

Added aditional if statement to convert the last_model class back to a sqlalchemy model class
@WhatmoreCB
Copy link
Author

from sqlalchemy import Column, Integer, String, ForeignKey
from sqlalchemy.ext.declarative import as_declarative
from sqlalchemy.orm import relationship, backref, configure_mappers
from sqlalchemy.ext.hybrid import hybrid_property
import flask_admin.contrib.sqla.tools as tools

import types

try:
# Attempt _class_resolver import from SQLALchemy 1.4/2.0 module architecture.
from sqlalchemy.orm.clsregistry import _class_resolver
except ImportError:
# If 1.4/2.0 module import fails, fall back to <1.3.x architecture.
from sqlalchemy.ext.declarative.clsregistry import _class_resolver # type: ignore

from flask_admin._compat import string_types

@as_declarative()
class Base:
pass

class GrandChild(Base):
tablename = 'GrandChild'
id = Column(Integer, primary_key=True)
name = Column(String)

child_id = Column(Integer, ForeignKey('Child.id'))
child = relationship('Child', foreign_keys=[child_id], backref=backref('grandchild'))

@hybrid_property
def upper_name(self):
    return self.name.upper() if self.name else None

class Child(Base):
tablename = 'Child'
id = Column(Integer, primary_key=True)
parent_id = Column(Integer, ForeignKey('Parent.id'))
parent = relationship('Parent', foreign_keys=[parent_id], backref=backref('children'))

class Parent(Base):
tablename = 'Parent'
id = Column(Integer, primary_key=True)

def test_is_hybrid_property_deep_relationship():

# Existing method
try:
    result = tools.is_hybrid_property(Parent, "children.grandchild.upper_name")
except AttributeError:
    result = False

assert result is False

def is_hybrid_property(model, attr_name):
    if isinstance(attr_name, string_types):
        names = attr_name.split('.')
        last_model = model
        for i in range(len(names) - 1):
            attr = getattr(last_model, names[i])
            if tools.is_association_proxy(attr):
                attr = attr.remote_attr
            last_model = attr.property.argument
            if isinstance(last_model, string_types):
                last_model = attr.property._clsregistry_resolve_name(last_model)()
            elif isinstance(last_model, _class_resolver):
                last_model = model._decl_class_registry[last_model.arg]
            elif isinstance(last_model, (types.FunctionType, types.MethodType)):
                last_model = last_model()
            elif getattr(last_model, 'is_mapper'):
                last_model = last_model.class_
        last_name = names[-1]
        return last_name in tools.get_hybrid_properties(last_model)
    else:
        return attr_name.name in tools.get_hybrid_properties(model)
    
# This should return True if the function correctly resolves deep relationships
try:
    result = is_hybrid_property(Parent, "children.grandchild.upper_name")
except AttributeError:
    result = False

assert result is True

if name == 'main':
configure_mappers()
test_is_hybrid_property_deep_relationship()

@WhatmoreCB WhatmoreCB marked this pull request as ready for review October 29, 2025 12:04
@ElLorans
Copy link
Contributor

ElLorans commented Oct 29, 2025

Thanks for your PR.

  1. Can you run tox -e style and commit the changes? If you do pre-commit install you won't need to do that anymore.
  2. Can you try fixing the formatting in your example?

EDIT: I also see your example is directly on flask-admin internals, which is great for the tests. Do you have by any chance a test also showing how this would work on a flask-admin app?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants