jowilf / starlette-admin

Fast, beautiful and extensible administrative interface framework for Starlette & FastApi applications
https://jowilf.github.io/starlette-admin/
MIT License
622 stars 60 forks source link

Bug: SQLAlchemy Joined Inheritance raises error #369

Open hasansezertasan opened 1 year ago

hasansezertasan commented 1 year ago

Bug: SQLAlchemy Joined Inheritance raises error

Describe the bug

SQLAlchemy Joined Inheritance raises error.

Error:

File "...\Lib\site-packages\starlette_admin\contrib\sqla\converters.py", line 112, in convert_fields_list
    len(attr.columns) == 1
AssertionError: Multiple-column properties are not supported

To Reproduce

Example from: Mapping Class Inheritance Hierarchies — SQLAlchemy 2.0 Documentation

from sqlalchemy import ForeignKey
from sqlalchemy.orm import DeclarativeBase
from sqlalchemy.orm import Mapped
from sqlalchemy.orm import mapped_column

class Base(DeclarativeBase):
    pass

class Employee(Base):
    __tablename__ = "employee"
    id: Mapped[int] = mapped_column(primary_key=True)
    name: Mapped[str]
    type: Mapped[str]

    __mapper_args__ = {
        "polymorphic_identity": "employee",
        "polymorphic_on": "type",
    }

    def __repr__(self):
        return f"{self.__class__.__name__}({self.name!r})"

class Engineer(Employee):
    __tablename__ = "engineer"
    id: Mapped[int] = mapped_column(ForeignKey("employee.id"), primary_key=True)
    engineer_name: Mapped[str]

    __mapper_args__ = {
        "polymorphic_identity": "engineer",
    }

class Manager(Employee):
    __tablename__ = "manager"
    id: Mapped[int] = mapped_column(ForeignKey("employee.id"), primary_key=True)
    manager_name: Mapped[str]

    __mapper_args__ = {
        "polymorphic_identity": "manager",
    }

Environment (please complete the following information):

Additional context

As far as I know, Flask Admin supports Polymorphic Association. So I checked their source code and tried to fix the issue by myself, I inspected the models but I weren't able to solve it.

I thinks these resources could help you a bit to help me solve this issue:

jowilf commented 1 year ago

226

hasansezertasan commented 1 year ago

I saw that issue but my problem differs, I'm using polymorphic association.

Did you check the example?

from sqlalchemy import ForeignKey
from sqlalchemy.orm import DeclarativeBase
from sqlalchemy.orm import Mapped
from sqlalchemy.orm import mapped_column

class Base(DeclarativeBase):
    pass

class Employee(Base):
    __tablename__ = "employee"
    id: Mapped[int] = mapped_column(primary_key=True)
    name: Mapped[str]
    type: Mapped[str]

    __mapper_args__ = {
        "polymorphic_identity": "employee",
        "polymorphic_on": "type",
    }

    def __repr__(self):
        return f"{self.__class__.__name__}({self.name!r})"

class Engineer(Employee):
    __tablename__ = "engineer"
    id: Mapped[int] = mapped_column(ForeignKey("employee.id"), primary_key=True)
    engineer_name: Mapped[str]

    __mapper_args__ = {
        "polymorphic_identity": "engineer",
    }

class Manager(Employee):
    __tablename__ = "manager"
    id: Mapped[int] = mapped_column(ForeignKey("employee.id"), primary_key=True)
    manager_name: Mapped[str]

    __mapper_args__ = {
        "polymorphic_identity": "manager",
    }

Try to create one view for each model in the example and manually query all the models like db.query(model).all(). SQLalchemy will give you only the records created in that table or assosiation.

But admin panel will return the all the items in the employee.

226 doesn't help me. In my situation I can not add a primary key column because I already have in each of them.

hasansezertasan commented 1 year ago
from sqlalchemy import (
    ForeignKey,
    create_engine,
)
from sqlalchemy.orm import DeclarativeBase, Mapped, mapped_column, sessionmaker
from starlette.applications import Starlette
from starlette.responses import HTMLResponse
from starlette.routing import Route
from starlette_admin.contrib.sqla import Admin, ModelView
from starlette_admin.fields import (
    DateTimeField,
    IntegerField,
    StringField,
)

class Base(DeclarativeBase):
    pass

class Employee(Base):
    __tablename__ = "employee"
    id: Mapped[int] = mapped_column(primary_key=True)
    first_name: Mapped[str]
    last_name: Mapped[str]
    type: Mapped[str]  # This column is used for polymorphic identity to determine the type of the object.

    __mapper_args__ = {
        "polymorphic_identity": "employee",  # This is the default value for Employee.
        "polymorphic_on": "type",  # This is the column used for polymorphic identity.
    }

    def __repr__(self):
        return f"{self.__class__.__name__}({self.name!r})"

class Engineer(Employee):  # Inherits from Employee because every Engineer is an Employee.
    __tablename__ = "engineer"
    id: Mapped[int] = mapped_column(ForeignKey("employee.id"), primary_key=True)
    # Engineer specific data, could be so much more.
    engineer_related_data: Mapped[str]

    __mapper_args__ = {
        "polymorphic_identity": "engineer",  # This is the value used for Engineer.
    }

class Manager(Employee):
    __tablename__ = "manager"
    id: Mapped[int] = mapped_column(ForeignKey("employee.id"), primary_key=True)
    # Manager specific data, could be so much more.
    manager_related_data: Mapped[str]

    __mapper_args__ = {
        "polymorphic_identity": "manager",  # This is the value used for Manager.
    }

class EmployeeView(ModelView):
    label = "Employees"
    name = "Employee"
    fields = [
        IntegerField(
            name="id",
            label="ID",
            help_text="ID of the record.",
            read_only=True,
        ),
        StringField(
            name="first_name",
            label="First Name",
            help_text="First name of the employee.",
        ),
        StringField(
            name="last_name",
            label="Last Name",
            help_text="Last name of the employee.",
        ),
    ]

class EngineerView(ModelView):  # Inherits from ModelView because we want to use the default ModelView.
    label = "Engineers"
    name = "Engineer"
    fields = [
        IntegerField(
            name="id",
            label="ID",
            help_text="ID of the record.",
            read_only=True,
        ),
        StringField(
            name="first_name",
            label="First Name",
            help_text="First name of the employee.",
        ),
        StringField(
            name="last_name",
            label="Last Name",
            help_text="Last name of the employee.",
        ),
        StringField(
            name="engineer_related_data",
            label="Engineer Related Data",
            help_text="Engineer related data.",
        ),
    ]

class ManagerView(ModelView):
    label = "Managers"
    name = "Manager"
    # I'm not including the `type` column because it doesn't meant to be edited. It will be set automatically by the ORM.
    fields = [
        IntegerField(
            name="id",
            label="ID",
            help_text="ID of the record.",
            read_only=True,
        ),
        StringField(
            name="first_name",
            label="First Name",
            help_text="First name of the employee.",
        ),
        StringField(
            name="last_name",
            label="Last Name",
            help_text="Last name of the employee.",
        ),
        StringField(
            name="manager_related_data",
            label="Manager Related Data",
            help_text="Manager related data.",
        ),
    ]

engine = create_engine(
    "sqlite:///db.sqlite3",
    connect_args={"check_same_thread": False},
    echo=True,
)
session = sessionmaker(bind=engine, autoflush=False)

def init_database() -> None:
    Base.metadata.create_all(engine)
    with session() as db:
        db.add_all(
            [
                Engineer(
                    first_name="Jon",
                    last_name="Snow",
                    engineer_related_data="Engineer related data",
                ),
                Engineer(
                    first_name="Maester",
                    last_name="Aemon",
                    engineer_related_data="Engineer related data",
                ),
                Manager(
                    first_name="Lord",
                    last_name="Commander",
                    manager_related_data="Manager related data",
                ),
                Manager(
                    first_name="Lord",
                    last_name="Steward",
                    manager_related_data="Manager related data",
                ),
            ]
        )
        db.commit()
        # Count of each table
        employees = db.query(Employee).all()
        employees = len(employees)
        engineers = db.query(Engineer).all()
        engineers = len(engineers)
        managers = db.query(Manager).all()
        managers = len(managers)
        print("All employees:", employees)
        print("All engineers:", engineers)
        print("All managers:", managers)

app = Starlette(
    routes=[
        Route(
            "/",
            lambda r: HTMLResponse('<a href="/admin/">Click me to get to Admin!</a>'),
        )
    ],
    on_startup=[init_database],
)

# Create admin
admin = Admin(engine, title="Example: Polymorphic Association")

# Add views
admin.add_view(EmployeeView(model=Employee))
admin.add_view(EngineerView(model=Engineer))
admin.add_view(ManagerView(model=Manager))

# Mount admin
admin.mount_to(app)

Check out this one, it's ready to run.

After the first attempt of running, it'll work properly and when you open the views there is something wrong.

EmployeeView will show "4" items and will count 4 items which is the correct one because every Engineer and Manager is still an Employee.

EngineerView and ManagerView will show 2 items and wil count 4 items. I did overcome the issue of Multiple-column properties are not supported by declaring all the fields by hand but it still doesn't work properly.

I believe we should fix it. It might be a problem in the future and I think this shouldn't be an issue to Starlette Admin with such a potential.

I can override the count method of the views but I'm not sure if it'll break at somewhere. I'm OK with overriding the count method because there won't be a create, edit, delete situation in my views.

If anyone reading this will ever encounter this issue, just declare the fields without passing columns, that will most probably solve the issue.

hasansezertasan commented 1 year ago

385 will only fix the problem here:

EngineerView and ManagerView will show 2 items and wil count 4 items. I did overcome the issue of Multiple-column properties are not supported by declaring all the fields by hand but it still doesn't work properly.

The issue persists with this example:

from sqlalchemy import (
    ForeignKey,
    create_engine,
)
from sqlalchemy.orm import DeclarativeBase, Mapped, mapped_column, sessionmaker
from starlette.applications import Starlette
from starlette.responses import HTMLResponse
from starlette.routing import Route
from starlette_admin.contrib.sqla import Admin, ModelView

class Base(DeclarativeBase):
    pass

class Employee(Base):
    __tablename__ = "employee"
    id: Mapped[int] = mapped_column(primary_key=True)
    first_name: Mapped[str]
    last_name: Mapped[str]
    type: Mapped[str]  # This column is used for polymorphic identity to determine the type of the object.

    __mapper_args__ = {
        "polymorphic_identity": "employee",  # This is the default value for Employee.
        "polymorphic_on": "type",  # This is the column used for polymorphic identity.
    }

    def __repr__(self):
        return f"{self.__class__.__name__}({self.name!r})"

class Engineer(Employee):  # Inherits from Employee because every Engineer is an Employee.
    __tablename__ = "engineer"
    id: Mapped[int] = mapped_column(ForeignKey("employee.id"), primary_key=True)
    # Engineer specific data, could be so much more.
    engineer_related_data: Mapped[str]

    __mapper_args__ = {
        "polymorphic_identity": "engineer",  # This is the value used for Engineer.
    }

class Manager(Employee):
    __tablename__ = "manager"
    id: Mapped[int] = mapped_column(ForeignKey("employee.id"), primary_key=True)
    # Manager specific data, could be so much more.
    manager_related_data: Mapped[str]

    __mapper_args__ = {
        "polymorphic_identity": "manager",  # This is the value used for Manager.
    }

class EmployeeView(ModelView):
    label = "Employees"
    name = "Employee"
    fields = [
        Employee.id,
        Employee.first_name,
        Employee.last_name,
    ]

class EngineerView(ModelView):  # Inherits from ModelView because we want to use the default ModelView.
    label = "Engineers"
    name = "Engineer"
    fields = [
        Engineer.id,
        Engineer.first_name,
        Engineer.last_name,
        Engineer.engineer_related_data,
    ]

class ManagerView(ModelView):
    label = "Managers"
    name = "Manager"
    # I'm not including the `type` column because it doesn't meant to be edited. It will be set automatically by the ORM.
    fields = [
        Manager.id,
        Manager.first_name,
        Manager.last_name,
        Manager.manager_related_data,
    ]

engine = create_engine(
    "sqlite:///db.sqlite3",
    connect_args={"check_same_thread": False},
    echo=True,
)
session = sessionmaker(bind=engine, autoflush=False)

def init_database() -> None:
    Base.metadata.drop_all(engine)
    Base.metadata.create_all(engine)
    with session() as db:
        db.add_all(
            [
                Engineer(
                    first_name="Jon",
                    last_name="Snow",
                    engineer_related_data="Engineer related data",
                ),
                Engineer(
                    first_name="Maester",
                    last_name="Aemon",
                    engineer_related_data="Engineer related data",
                ),
                Manager(
                    first_name="Lord",
                    last_name="Commander",
                    manager_related_data="Manager related data",
                ),
                Manager(
                    first_name="Lord",
                    last_name="Steward",
                    manager_related_data="Manager related data",
                ),
            ]
        )
        db.commit()
        # Count of each table
        employees = db.query(Employee).all()
        employees = len(employees)
        engineers = db.query(Engineer).all()
        engineers = len(engineers)
        managers = db.query(Manager).all()
        managers = len(managers)
        print("All employees:", employees)
        print("All engineers:", engineers)
        print("All managers:", managers)

app = Starlette(
    routes=[
        Route(
            "/",
            lambda r: HTMLResponse('<a href="/admin/">Click me to get to Admin!</a>'),
        )
    ],
    on_startup=[init_database],
)

# Create admin
admin = Admin(engine, title="Example: Polymorphic Association")

# Add views
admin.add_view(EmployeeView(model=Employee))
admin.add_view(EngineerView(model=Engineer))
admin.add_view(ManagerView(model=Manager))

# Mount admin
admin.mount_to(app)

I ran the tests with this block, no errors accured. @jowilf What do you think about this? I don't know if there is another situation that attr.columns has more than 1 column except Polymorphic Association.

We can also implement something like this: https://github.com/flask-admin/flask-admin/blob/14e24c970f0ee3a29add830612eee9a0b0ba5dcc/flask_admin/contrib/sqla/view.py#L427

Edit: This also may resolve #338 but I'm not sure about this.

jowilf commented 1 year ago

The issue has been automatically closed due to your comment here.

I ran the tests with this block, no errors accured. @jowilf What do you think about this?

The code is great, but we can also filter the foreign keys as Flask does. This way, it will always work for Polymorphic Association because the other columns are all foreign keys.

I don't know if there is another situation that attr.columns has more than 1 column except Polymorphic Association.

I don't know either. Let's focus on supporting Polymorphic Association first.

Edit: This also may resolve https://github.com/jowilf/starlette-admin/issues/338 but I'm not sure about this.

As you can see in the code, the foreign keys are intentionally removed with the following line of code:

if not column.foreign_keys:

However, you can still provide your own field and this step will be skipped.