with ... as ... statement can make sesssion close,which will lead to DetachedInstanceError

ChengChe106 commented 1 month ago

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

sqladmin/models

def _run_query_sync(self, stmt: ClauseElement) -> Any:
    with self.session_maker(expire_on_commit=False) as session:
        result = session.execute(stmt)
        return result.scalars().unique().all()

In the code, the use of a with ... as ... statement to manage the session led to an unexpected closure of the session, which may caused the following error in some case: sqlalchemy.orm.exc.DetachedInstanceError:

https://docs.sqlalchemy.org/en/20/errors.html#parent-instance-x-is-not-bound-to-a-session-lazy-load-deferred-load-refresh-etc-operation-cannot-proceed

This error occurs because closing the session detaches instances that still require the session for lazy loading or other operations.

To resolve this, I modified the code to manage the session explicitly, avoiding its premature closure. This change prevents the DetachedInstanceError by keeping the session open until all required operations are complete.

def _run_query_sync(self, stmt: ClauseElement) -> Any:
    session = self.session_maker(expire_on_commit=False)
    result = session.execute(stmt)
    return result.scalars().unique().all()

With this modification, the error no longer occurs, and the instances remain attached to the session as needed.

Steps to reproduce the bug

No response

Expected behavior

No response

Actual behavior

No response

Debugging material

No response

Environment

Windows 10 / ptyhon 3.10.11 / sqladmin 0.16.1

Additional context

No response

aminalaee commented 1 month ago

Hey, thanks for reporting this.

First thing is that you can pass your session_maker to Admin(session_maker=...) with the expire_on_commit so SQLAdmin doesn't have to create one for you.
Second is that there's no code and information in your description to know how this is happening.

FaraSeer commented 4 weeks ago

I can confirm this. This can be easily reproduced if you try using any field from the relation in __repr__ method. When you open any admin pane where this representation is shown, you will get the following:

sqlalchemy.orm.exc.DetachedInstanceError: Parent instance <Release at 0x115b20690> is not bound to a Session; lazy load operation of attribute 'product' cannot proceed

ChengChe106 commented 4 weeks ago

Yes, I did reconstruct the method repr of models in my project. I was unaware that this was the issue.

aminalaee commented 4 weeks ago

if you try using any field from the relation in

That is expected. The SQLAlchemy session is closed at this point so you cannot load any relationship that was not loaded. For example if your model has 10 relationships and SQLAdmin has no idea which relationships to pre-load, it has to either load nothing or load all of them to avoid this problem. Loading all of them will be a huge database load.

I can think of two options for this:

If your model has a relationship called x, you can include x in column_list and column_details_list. This way the relationship is always loaded and should work in __repr__ too.
Another option is that we allow the user to control which relationships to load so something along these lines should be updated: https://github.com/aminalaee/sqladmin/blob/d062cfa845fdbd2f09cbf0a1335ea8cce6e285c9/sqladmin/models.py#L648

ChengChe106 commented 4 weeks ago

Pardon, I didn't understand the sentence "SQLAdmin should not relationships so they can be used in the repr or str methods." Could you please explain it?

aminalaee commented 4 weeks ago

@ChengChe106 Sorry I wrote that in a hurry. I have edited my comment, let me know if you have any questions.

ChengChe106 commented 4 weeks ago

if you try using any field from the relation in

That is expected. The SQLAlchemy session is closed at this point so you cannot load any relationship that was not loaded. For example if your model has 10 relationships and SQLAdmin has no idea which relationships to pre-load, it has to either load nothing or load all of them to avoid this problem. Loading all of them will be a huge database load.

I can think of two options for this:

If your model has a relationship called x, you can include x in column_list and column_details_list. This way the relationship is always loaded and should work in __repr__ too.

Another option is that we allow the user to control which relationships to load so something along these lines should be updated: https://github.com/aminalaee/sqladmin/blob/d062cfa845fdbd2f09cbf0a1335ea8cce6e285c9/sqladmin/models.py#L648

I try the first option. It cann't work.

If I add "User.permission_groups" to the "column_list" and "column_details_list" in here, like this:

class UserAdmin(ModelView, model=User):
    column_list = [
        User.id,
        User.username,
        User.is_active,
        User.is_superuser,
        User.permission_groups,
    ]
    column_column_details_list = [
        User.id,
        User.username,
        User.is_active,
        User.is_superuser,
        User.permission_groups,
    ]

This error is raised:

ERROR:    Exception in ASGI application
Traceback (most recent call last):
  File "F:\PythonProjects\FastAPI\FastAPISystem\venv\lib\site-packages\uvicorn\protocols\http\httptools_impl.py", line 426, in run_asgi
    result = await app(  # type: ignore[func-returns-value]
  File "F:\PythonProjects\FastAPI\FastAPISystem\venv\lib\site-packages\uvicorn\middleware\proxy_headers.py", line 84, in __call__    return await self.app(scope, receive, send)
  File "F:\PythonProjects\FastAPI\FastAPISystem\venv\lib\site-packages\fastapi\applications.py", line 1054, in __call__
    await super().__call__(scope, receive, send)
  File "F:\PythonProjects\FastAPI\FastAPISystem\venv\lib\site-packages\starlette\applications.py", line 116, in __call__
    await self.middleware_stack(scope, receive, send)
  File "F:\PythonProjects\FastAPI\FastAPISystem\venv\lib\site-packages\starlette\middleware\errors.py", line 186, in __call__    
    raise exc
  File "F:\PythonProjects\FastAPI\FastAPISystem\venv\lib\site-packages\starlette\middleware\errors.py", line 164, in __call__    
    await self.app(scope, receive, _send)
  File "F:\PythonProjects\FastAPI\FastAPISystem\venv\lib\site-packages\starlette\middleware\exceptions.py", line 62, in __call__ 
    await wrap_app_handling_exceptions(self.app, conn)(scope, receive, send)
  File "F:\PythonProjects\FastAPI\FastAPISystem\venv\lib\site-packages\starlette\_exception_handler.py", line 55, in wrapped_app 
    raise exc
  File "F:\PythonProjects\FastAPI\FastAPISystem\venv\lib\site-packages\starlette\_exception_handler.py", line 44, in wrapped_app 
    await app(scope, receive, sender)
  File "F:\PythonProjects\FastAPI\FastAPISystem\venv\lib\site-packages\starlette\routing.py", line 746, in __call__
    await route.handle(scope, receive, send)
  File "F:\PythonProjects\FastAPI\FastAPISystem\venv\lib\site-packages\starlette\routing.py", line 459, in handle
    await self.app(scope, receive, send)
  File "F:\PythonProjects\FastAPI\FastAPISystem\venv\lib\site-packages\starlette\applications.py", line 116, in __call__
    await self.middleware_stack(scope, receive, send)
  File "F:\PythonProjects\FastAPI\FastAPISystem\venv\lib\site-packages\starlette\middleware\errors.py", line 186, in __call__    
    raise exc
  File "F:\PythonProjects\FastAPI\FastAPISystem\venv\lib\site-packages\starlette\middleware\errors.py", line 164, in __call__    
    await self.app(scope, receive, _send)
  File "F:\PythonProjects\FastAPI\FastAPISystem\venv\lib\site-packages\starlette\middleware\sessions.py", line 83, in __call__   
    await self.app(scope, receive, send_wrapper)
  File "F:\PythonProjects\FastAPI\FastAPISystem\venv\lib\site-packages\starlette\middleware\exceptions.py", line 62, in __call__ 
    await wrap_app_handling_exceptions(self.app, conn)(scope, receive, send)
  File "F:\PythonProjects\FastAPI\FastAPISystem\venv\lib\site-packages\starlette\_exception_handler.py", line 55, in wrapped_app 
    raise exc
  File "F:\PythonProjects\FastAPI\FastAPISystem\venv\lib\site-packages\starlette\_exception_handler.py", line 44, in wrapped_app 
    await app(scope, receive, sender)
  File "F:\PythonProjects\FastAPI\FastAPISystem\venv\lib\site-packages\starlette\routing.py", line 746, in __call__
    await route.handle(scope, receive, send)
  File "F:\PythonProjects\FastAPI\FastAPISystem\venv\lib\site-packages\starlette\routing.py", line 288, in handle
    await self.app(scope, receive, send)
  File "F:\PythonProjects\FastAPI\FastAPISystem\venv\lib\site-packages\starlette\routing.py", line 75, in app
    await wrap_app_handling_exceptions(app, request)(scope, receive, send)
  File "F:\PythonProjects\FastAPI\FastAPISystem\venv\lib\site-packages\starlette\_exception_handler.py", line 55, in wrapped_app 
    raise exc
  File "F:\PythonProjects\FastAPI\FastAPISystem\venv\lib\site-packages\starlette\_exception_handler.py", line 44, in wrapped_app 
    await app(scope, receive, sender)
  File "F:\PythonProjects\FastAPI\FastAPISystem\venv\lib\site-packages\starlette\routing.py", line 70, in app
    response = await func(request)
  File "F:\PythonProjects\FastAPI\FastAPISystem\venv\lib\site-packages\sqladmin\authentication.py", line 66, in wrapper_decorator    return await func(*args, **kwargs)
  File "F:\PythonProjects\FastAPI\FastAPISystem\venv\lib\site-packages\sqladmin\application.py", line 444, in list
    return await self.templates.TemplateResponse(
  File "F:\PythonProjects\FastAPI\FastAPISystem\venv\lib\site-packages\sqladmin\templating.py", line 63, in TemplateResponse     
    content = await template.render_async(context)
  File "F:\PythonProjects\FastAPI\FastAPISystem\venv\lib\site-packages\jinja2\environment.py", line 1324, in render_async        
    return self.environment.handle_exception()
  File "F:\PythonProjects\FastAPI\FastAPISystem\venv\lib\site-packages\jinja2\environment.py", line 936, in handle_exception     
    raise rewrite_traceback_stack(source=source)
  File "F:\PythonProjects\FastAPI\FastAPISystem\venv\lib\site-packages\jinja2\environment.py", line 1321, in <listcomp>
    [n async for n in self.root_render_func(ctx)]  # type: ignore
  File "F:\PythonProjects\FastAPI\FastAPISystem\venv\lib\site-packages\sqladmin\templates\list.html", line 1, in top-level template code
    {% extends "layout.html" %}
  File "F:\PythonProjects\FastAPI\FastAPISystem\venv\lib\site-packages\sqladmin\templates\layout.html", line 2, in top-level template code
    {% from '_macros.html' import display_menu %}
  File "F:\PythonProjects\FastAPI\FastAPISystem\venv\lib\site-packages\sqladmin\templates\base.html", line 18, in top-level template code
    {% block body %}
  File "F:\PythonProjects\FastAPI\FastAPISystem\venv\lib\site-packages\sqladmin\templates\layout.html", line 49, in block 'body' 
    {% block content %} {% endblock %}
  File "F:\PythonProjects\FastAPI\FastAPISystem\venv\lib\site-packages\sqladmin\templates\list.html", line 147, in block 'content'
    <a href="{{ model_view._build_url_for('admin:details', request, elem) }}">({{ formatted_elem }})</a>
  File "F:\Python310\lib\dataclasses.py", line 239, in wrapper
    result = user_function(self)
  File "<string>", line 3, in __repr__
  File "F:\PythonProjects\FastAPI\FastAPISystem\venv\lib\site-packages\sqlalchemy\orm\attributes.py", line 566, in __get__       
    return self.impl.get(state, dict_)  # type: ignore[no-any-return]
  File "F:\PythonProjects\FastAPI\FastAPISystem\venv\lib\site-packages\sqlalchemy\orm\attributes.py", line 1086, in get
    value = self._fire_loader_callables(state, key, passive)
  File "F:\PythonProjects\FastAPI\FastAPISystem\venv\lib\site-packages\sqlalchemy\orm\attributes.py", line 1121, in _fire_loader_callables
    return self.callable_(state, passive)
  File "F:\PythonProjects\FastAPI\FastAPISystem\venv\lib\site-packages\sqlalchemy\orm\strategies.py", line 918, in _load_for_state
    raise orm_exc.DetachedInstanceError(
sqlalchemy.orm.exc.DetachedInstanceError: Parent instance <PermissionGroup at 0x2450fee7e50> is not bound to a Session; lazy load operation of attribute 'users' cannot proceed (Background on this error at: https://sqlalche.me/e/20/bhk3)

If I don't add "User.permission_groups" to column_list, users list page is good. But, no matter how, visiting users detail page will raise the error mentioned above.

aminalaee commented 4 weeks ago

Please provide a minimum code to reproduce the issue. I see that you are using dataclasses but don't see how.

aminalaee / sqladmin