sqladmin
sqladmin copied to clipboard
aminalaee
Incorrect URLs with multiple views on the same model
Checklist
- [X] The bug is reproducible against the latest release or
master. - [X] There are no similar issues or pull requests to fix it yet.
Describe the bug
The usage case is to provide two different views with list_query and slightly different templates.
Having two views on the same model requires to provide a different name and identity to the second view to avoid a crash.
However, both links in the menu point to the same ModelView, so only one of the two views is usable.
Side note: if a name is omitted in the second view, we get a TypeError in Menu.add (called from BaseAdmin._build_menu), which could probably be caught in advance to yield a more meaningful exception, such as "Multiple views on the same model must have different names".
Steps to reproduce the bug
MVE:
from sqlalchemy import Column, Integer, String, create_engine, select, Select, func
from sqlalchemy.orm import declarative_base, Session
from fastapi import FastAPI
from sqladmin import Admin, ModelView
from starlette.requests import Request
Base = declarative_base()
engine = create_engine(
"sqlite:///example.db",
connect_args={"check_same_thread": False},
)
class User(Base):
__tablename__ = "users"
id = Column(Integer, primary_key=True)
name = Column(String)
email = Column(String)
Base.metadata.create_all(engine)
with Session(engine) as session:
if session.query(User.id).count() == 0:
session.add(User(name='Demo1', email='[email protected]'))
session.add(User(name='Demo2', email='demo2@bad_domain.org'))
session.commit()
# ------------------------
app = FastAPI()
admin = Admin(app, engine)
class UserAdmin(ModelView, model=User):
column_list = [User.id, User.name]
class BadDomainUserAdmin(ModelView, model=User):
column_list = [User.id, User.name]
name = 'Bad user' # omitting this leads to a TypeError
identity = 'bad_user'
def list_query(self, request: Request) -> Select:
return select(User).where(User.email == 'demo2@bad_domain.org')
def count_query(self, request: Request) -> Select:
return select(func.count()).select_from(User).where(User.email == 'demo2@bad_domain.org')
admin.add_view(UserAdmin)
admin.add_view(BadDomainUserAdmin)
Expected behavior
There should be two distincts views with distincts URLs (or at least, I didn't find anything suggesting that this in not supported).
Actual behavior
Both menu entries lead to /admin/user/list, instead of /admin/bad_user/list.
Debugging material
This seems to be due to identity being overwritten by the slugified model name:
https://github.com/aminalaee/sqladmin/blob/751329c16960ce2257c66ad19509fc657af9e4ca/sqladmin/models.py#L89
Changing the line to
cls.identity = attrs.get("identity", slugify_class_name(model.__name__))
seems to fix it at least at first glance, however, identity is referenced both in the templates and in models.py as a parameter to build urls, so this is not sufficient. Indeed, the urls for user edititing and detailed display both point to user instead of bad_user.
Environment
- Linux Mint 21.1 Vera
- python 3.10.12
- SQLAdmin 0.16.0
Additional context
The intention was to provide a customized list view for certain items in the table (so, listing only, no detail/edit view) with a custom form widget to allow the user to perform an action on some of these items.
You are right in most of the assumptions, and yes this usage is not intended, so I think you have two options:
- create a custom view for this model, needs a bit of work
- right now SQLAdmin doesn't have filtering, but if you could do dynamic filter on the Users, then you wouldn't need to implement this custom view, right?
I see; I might try to create a custom view in the future.
The elements available in the default templates (like the list view) are quite powerful, it would be nice to be able to reuse them, and have some examples in the documentation on how to do so.
if you could do dynamic filter on the Users, then you wouldn't need to implement this custom view, right?
In the actual application I would still go with a different view, mostly because the UI displayed for the filtered items is quite different.
For context, the table would contain a list of items, some of them in a "pending" state, and most of them in a "processed". The few pending items would have a few actions available on them and the ability to modify them, while the many processed items are there for later double-check and consultation, and are read-only. There are many ways to go about this, including creating two different tables, however the first attempt I made was to produce two different filtered views of the same table.
I faced the same requirement. I can propose next solution:
in models.py::ModelViewMeta:
- cls.identity = slugify_class_name(model.__name__)
+ cls.identity = kwargs.get("identity", slugify_class_name(model.__name__))
So the usage is:
# for custom view we use identity in params
class AllMessagesView(ModelView, model=SomeMessage, identity="all_messages"):
...
# default view is the same
class UserMessagesView(ModelView, model=SomeMessage):
...
I had a ton of trouble with this. I finally found a workaround: create a new inherited class. E.g.:
class View1(ModelView, model=User): ...
class UserForNewView(User): ...
class View2(ModelView, model=UserForNewView): ...
Now all URLs will work correctly in view2 because it has a brand new __class__.__name__
No use of identity is necessary. identity always breaks URLs if you change it to anything other than class name.
Any suggestions how to implement this to avoid such problems? Open to ideas and PRs.
Any suggestions how to implement this to avoid such problems? Open to ideas and PRs.
If identity is specified, inherit from sqlalchemy model class using identity as new class name.
The example I wrote above can be written in one line:
class View(ModelView, model=type("banned_users", (User,), {})): ...
I want this to be the equivalent of:
class View(ModelView, model=User, identity="banned_users"): ...
And if no identity is specified it would take slugified class name of User ("user"), as usual.
If 2 views have the same identity, an exception should be raised.
Currently identity is broken so you shouldn't worry about changing the logic around it.
Not sure if this is what @MitPitt suggested, but could we not make the changes in models.py and fetch the identity from the request-object instead of the row?
def _build_url_for(self, name: str, request: Request, obj: Any) -> URL:
return request.url_for(
name,
- identity=slugify_class_name(obj.__class__.__name__),
+ identity=request.path_params.get("identity", slugify_class_name(obj.__class__.__name__)),
pk=get_object_identifier(obj),
)
I'm happy to make the PR if it makes sense.
I think you can give it a try, reproducing it should be simple with this example: https://github.com/aminalaee/sqladmin/issues/681#issue-2033198325
I just tested around and remembered my last attempt why I didn't implement this. The URL builder can't always rely on request.path_params because sometimes we build URL from one model to point to another one, not always pointing to the same model, like the following example:
The link to Parent can is to the details of a single parent so it might be that it doesn't matter which view we are pointing to, but it is clear that we can't figure out which view we should use for Parent by default.
Yes I noticed the same thing when running some tests (I myself does not use relationship-models). To solve this, I think we need to use different ways to generate the url in the different places.
To solve the problem with the identity being derived from the model-name, I think we need to have a similar solution to what @glarione suggested and in that case, you need to create the second view-class like :
class BadDomainUserAdmin(ModelView, model=User, identity="bad_user"):
column_list = [User.id, User.name]
name = "Bad user" # omitting this leads to a TypeError
def list_query(self, request: Request) -> Select:
return select(User).where(User.email == "demo2@bad_domain.org")
Otherwise the list_query() in the example will not be used for the listing either (as it will use the UserAdmin-view and not BadDomainUserAdmin.
The other way to do it (and what I have done so far) is to create a "baseview" with the model and basic-settings you want and then inherit that view like:
class UserAdmin(ModelView, model=User):
column_list = [User.id, User.name]
class InheritedUserAdmin(UserAdmin):
column_list = [User.name, User.email]
name = "inherited" # omitting this leads to a TypeError
name_plural = "inherited users" # to have a different name in the sidebar
identity = "inherited"
def list_query(self, request: Request) -> Select:
return select(User).where(User.email == "demo2@bad_domain.org")
I filed a PR #759 as a start with some changes that addresses these problems.
Thanks for the initiative, but I'm still not sure if this is the best way to tackle this.
Even with this PR, it's still needs some hacking to make it work and that definitely is not ideal. Django for example simply raises an error and doesn't allow registering a model twice. I'm interested to see what is the flask-admin behaviour in this case.
Maybe it's still better to implement filters so one can filter records instead of providing two views.
I agree the identity-hack probably should be solved some other way, removing it from PR.
As the PR is now, it works if you work with inheritance, if inheriting the view. This way (with inherited views like above) also works in flask-admin.
Flask-admin has the endpoint attribute that is used to generate urls.. But also note that in flask-admin we call the add_view with an instance of the view-class add_view(User()) and not the class as in sql-admin add_view(User)...
flask-admin:
admin.add_view(UserAdmin(User, db.session, name="List", endpoint="list"))
admin.add_view(InheritedUserAdmin(User, db.session, name="Bad User List", endpoint="baduser"))
vs in sql-admin:
admin.add_view(UserAdmin)
admin.add_view(InheritedUserAdmin)
Interesting, it is kind of similar to the identity we have, we just get it in a different way, but probably flask-admin is storing the endpoint:model mapping somewhere so it can do the mapping. I think the same could be achieved here, but we should also check how the URL building is done from one model to the other in flask admin.
For example we could do:
class UserAdmin(ModelView, model=User, identity="user"): ...
class AnotherUserAdmin(ModelView, model=User, identity="another_user"): ...
class AddressAdmin(ModelView, model=Address, identity="address"): ...
# register ...
How would the URL from Address know to which User view point to.
Interesting, it is kind of similar to the identity we have, we just get it in a different way, but probably flask-admin is storing the endpoint:model mapping somewhere so it can do the mapping. I think the same could be achieved here, but we should also check how the URL building is done from one model to the other in flask admin.
Flask Admin registers one blueprint per view. The model is stored in the view and each model view has its own endpoint functions, which they have access the the model, finally these endpoint functions are registered to the blueprint on the view.
For example we could do:
class UserAdmin(ModelView, model=User, identity="user"): ... class AnotherUserAdmin(ModelView, model=User, identity="another_user"): ... class AddressAdmin(ModelView, model=Address, identity="address"): ... # register ...How would the URL from Address know to which User view point to.
On the other hand, Flask AppBuilder builds the urls almost as is (view.__name__.lower()).
At #77 I proposed using Router per view and moving edit, create, detail, etc. endpoints to the ModelView class (similar to Flask Admin).
I believe using Flask Admin approach over endpoint mapping approach could be quite good for uses cases like this one.
I saw this endpoint mapping approach (mentioned above) used in the Starlette Admin library as well. I tried to implement router per view on Starlette Admin but failed (I'll look into it again tomorrow).
I would like to knock what is the motivation behind this approach?
