jowilf / starlette-admin

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

Bug: EnumField options displayed incorrectly in list/detail templates [2] #123

Closed sshghoult closed 1 year ago

sshghoult commented 1 year ago

The issue is the same as reported in #95. I'm terribly sorry about leaving it hanging for 3 weeks, but that problem was in my backlog all this time. Opening a new issue so you don't miss a comment on the old one. However, this once I bring you reproducable example.

I cloned and started up starlette-admin-demo, set up the sqla admin as per readme.md and applied the following changes:

app/sqla/models.py: updated Gender enum content

class Gender(str, enum.Enum):
    MALE = "Mâle"
    FEMALE = "Femme"
    UNKNOWN = 'ignoré'
    STORED = 'LaBeL'

app/sqla/views.py: added sex field to the model View via clean EnumField, not inferred from a model; added a choice list, copied from the Gender enum. added EnumFields based on both enum and choices_list (one of these is always commented out, of course)

sex_choices = [('MALE', 'Mâle'), ('FEMALE', 'Femme'), ('UNKNOWN', 'ignoré'), ('STORED', 'LaBeL')]

class UserView(ModelView):
    page_size_options = [5, 10, 25, -1]
    fields = [
        "id",
        "full_name",
        EmailField("username"),
        "avatar",
        "posts",
        EnumField('sex', choices=sex_choices),
        # EnumField('sex', enum=Gender),
        CommentCounterField("comments_counter", label="Number of Comments"),
        "comments",
    ]
    ...

If Enum-based EnumField is used, then all the views (list, detail, create, edit) always display names of the enumeration members, and never their values. image image

if choices-list-based EnumField is used, then create/edit views display values (per Django terminology) and list/detail views display labels. image image image

It's hard to notice when values and names of members a similar to each other and frontend styles format the received value. However, it's rather clear when values are written in another language.

I believe, the template shenenigans I mentioned in #95 are still the key to the core of the problem. The choices-list-based field is displayed correctly in create/edit forms, because options' values and labels are explicitly set in parts of generated html in _starletteadmin/templates/forms/enum.html. EnumField.serialize_value doesn't seem to be actually used during rendering (though I might be wrong, as I don't know this codebase this thorougly).

Sidenote: when choices-list-based EnumField is used, current value is erased on edit. It's not a case when the Enum-based field is used. image image

jowilf commented 1 year ago

The Enum name is used as label inside EnumField. It's the same behavior with wtform-sqlalchemy

It better to use choices-list based EnumField when you want to customize the label

For example:

EnumField(
    "status",
    choices=[("d", _("Draft")), ("p", _("Published")), ("w", _("Withdrawn"))],
),
jowilf commented 1 year ago

Sidenote: when choices-list-based EnumField is used, current value is erased on edit. It's not a case when the Enum-based field is used.

if choices-list-based EnumField is used, then create/edit views display values (per Django terminology) and list/detail views display labels.

I can't seem to reproduce these issues, can you provide a minimum example ? just a simple model with 2-3 columns

sshghoult commented 1 year ago

As per description, I can reproduce the issue with the model "User" at app/sqla/models.py in your starlette-admin-demo and aforementioned updates to the view

jowilf commented 1 year ago

Your error is related to your choices list -> sex_choices = [('MALE', 'Mâle'), ('FEMALE', 'Femme'), ('UNKNOWN', 'ignoré'), ('STORED', 'LaBeL')]

As you can see in the documentation (EnumField), the choices parameters is a list of (value, label) pairs.

The valid list is ->

sex_choices = [('Mâle', 'MALE'), ('Femme', 'FEMALE'), ('ignoré', 'UNKNOWN'), ('LaBeL', 'STORED')]

Another example :

if you have an enum class like this:

class Counter(str, enum.Enum):
    ONE = 1
    TWO = 2

The equivalent EnumField is:

EnumField("choice", choices=((1, "One"), (2, "Two")), coerce=int),
sshghoult commented 1 year ago

I'm sorry, but your first example contradicts the example in the documentation for the (EnumField)

class MyModel:
    language: str

class MyModelView(ModelView):
    fields = [
        EnumField(
            "language",
            choices=[("cpp", "C++"), ("py", "Python"), ("text", "Plain Text")],
        )
    ]

Clearly, the choices list here comprises (value, label) tuples.

I believe one of us must be misinterpreting the difference between the value and label terms. I use them in the way defined in the Django, where value is a "thing" stored in the database, and label is a human-readable serialization of the "thing" to display to the user. This notion seems to comply with the example in the documentation, however, contradicts the first example you've just provided (though complies with the second).

I must've caused that confusion by initially mixing up the value and label entities in

if choices-list-based EnumField is used, then create/edit views display values (per Django terminology) and list/detail views display labels.

The correct version of the problem description that is actually in accordance with Django terminology is " if choices-list-based EnumField is used, then create/edit views display labels (per Django terminology) and list/detail views display values." Sorry about that.

Moreover, with whichever ordering of value and label being correct, there is still inconsistency in the display of the field in the different views, which is a problem.

jowilf commented 1 year ago

The isssue is with sqlalchemy itself which store the enum names in database(https://docs.sqlalchemy.org/en/20/core/type_basics.html#sqlalchemy.types.Enum). The example in the documentation work well with MongoEngine, Odmantic

More info -> https://stackoverflow.com/questions/47206201/how-to-use-enum-with-sqlalchemy-and-alembic

sshghoult commented 1 year ago

It doesn't seem to be specific to SQLAlchemy, as I experience the same display inconsitency on custom backend. Allow me to highlight that create/edit views display everything correctly for both SQLA and custom backend.

Besides, on retrieval, SQLModel with a proper Enum as an attribute value is instantiated, so the pecularities of the sqla enum storage mechanism can't leak to the admin level.

sshghoult commented 1 year ago

Update: figured out the case on my custom backend, but it's applicable for SQLA as well, as per SO post you mentioned.

When model and raw_obj are passed to render, and raw_obj has a defined on the persistence level Enum as a value for a field, during the execution of EnumField._get_label that is called during serialization, the persistence-level Enum hijacks the control flow and forces it's name to be used in the display. Choices list defined on the EnumField as a part of a ModelView is therefore ignored. The inconsistency of the views and out-of-the-box correctness of the edit/create view is caused by the direct usage of the field.choices in the forms/enum.html.

As I understand, by mentioning the details of enum storage at sqla you implied that such forwarding and hijacking are obvious -- they really aren't, and I'd love to see it at the very least denoted in the docs, and much sooner fixed.

jowilf commented 1 year ago

To better understand each other, can you try and tell me what wrong with this code ?

import enum

import uvicorn
from sqlalchemy import Column, Integer, String, create_engine, Enum
from sqlalchemy.ext.declarative import declarative_base
from starlette.applications import Starlette

from starlette_admin.contrib.sqla import Admin, ModelView

Base = declarative_base()
engine = create_engine("sqlite:///test.db", connect_args={"check_same_thread": False})

class Gender(str, enum.Enum):
    MALE = 1
    FEMALE = 2
    UNKNOWN = 0

class User(Base):
    __tablename__ = "user"

    id = Column(Integer, primary_key=True)
    name = Column(String)
    sex = Column(Enum(Gender))
    # If you want to save values into your database, then
    # sex = Column(Enum(Gender, values_callable=lambda obj: [e.value for e in obj]))

Base.metadata.create_all(engine)

app = Starlette()  # FastAPI()

# Create admin
admin = Admin(engine, title="Example: SQLAlchemy", base_url="/")

# Add view
admin.add_view(ModelView(User))

# Mount admin to your app
admin.mount_to(app)

if __name__ == '__main__':
    uvicorn.run(app)
jowilf commented 1 year ago

When model and raw_obj are passed to render, and raw_obj has a defined on the persistence level Enum as a value for a field, during the execution of EnumField._get_label that is called during serialization, the persistence-level Enum hijacks the control flow and forces it's name to be used in the display. Choices list defined on the EnumField as a part of a ModelView is therefore ignored.

Are you trying to override a template? raw_obj is your database model instance which is not touch by starlette-admin. It can be useful in some case to access deep level values (e.g. https://github.com/jowilf/starlette-admin-demo/blob/9406c6908f6e89f953003bcafc4883dd732496c1/templates/admin/sqla/post_detail.html#L94). Otherwise, you should use obj which contains the serialized values.

sshghoult commented 1 year ago

https://github.com/jowilf/starlette-admin/issues/123#issuecomment-1476485332 There doesn't seem to be anything wrong with it.

https://github.com/jowilf/starlette-admin/issues/123#issuecomment-1476494167 Yes, raw_obj that is passed directly to the call of the TemplateResponse is not used in the templates, however, it serves as a parameter for a ModelView.serialize that defines the content of the view. The value stored in the raw_obj (which is an instance of Enum) is passed to the Field.serialize_value through the calls of ModelView.serialize and ModelView.serialize_field_value, where the type-check condition catches it and uses the name of the instance in the display. That is the default callstack for the rendering process, and that check in the Field.serialize_value is why the low-level enum leaks to the display.

class EnumFiels:
    ...

    def _get_label(self, value: Any) -> Any:
        if isinstance(value, Enum):
            return value.name
        for v, label in self.choices:
            if value == v:
                return label
        raise ValueError(f"Invalid choice value: {value}")
jowilf commented 1 year ago

As I said previously, raw_obj is your model instance returned by find_by_pk method, It's not serialized (ref). Only obj is serialized and should be used to access data.

Also, raw_obj is only available for model detail view. If you are trying to overriding a template for a field you should use data to access the field value -> https://jowilf.github.io/starlette-admin/advanced/custom-field/#form

But I think we are getting to far from your initial issue.

There doesn't seem to be anything wrong with it.

It still not clear for me the issue your are facing. can you provide a minimum example to reproduce it, and a clear description? Otherwise, sorry but I can't help you. Thank

sshghoult commented 1 year ago

It seems that we are completely unable to understand each other's side of the question. Thank you for the time spent trying to help me.