Enhancement: ModelView.find_all and ModelView.count still don't take into account the Request for the stmt - needed for multitenant support

[sglebs]

Describe the bug

As already suggested by me in https://github.com/jowilf/starlette-admin/issues/274 , views.py.ModelView.find_all and views.py.ModelView.count still do not take into account the Request when building the stmt. This forces people to subclass this class, and copy/paste code every time a startlette-admin version upgrade comes out.

To Reproduce

Try to filter records based on the logged user, properly supporting multi tenancy.

The problem is in the line stmt = self.get_list_query().offset(skip) , in both methods.

Environment (please complete the following information):

• Starlette-Admin version: 0.13.1
• ORM/ODMs: SQLAlchemy, psycopg2-binary==2.9.9
• SQLAlchemy-serializer==1.4.1

Suggested fix

1. Add these 2 methods in views.py.ModelView:

    def get_list_query_for_request(self, request: Request):
        return self.get_list_query()    # backwards compatibility

    def get_count_query_for_request(self, request: Request):
        return self.get_count_query()   # backwards compatibility

2. In views.py.ModelView.find_all, replace this line:

stmt = self.get_list_query().offset(skip)

with this line:

stmt = self.get_list_query_for_request(request).offset(skip)

3. In views.py.ModelView.count, replace this line:

stmt = self.get_count_query()

with this line:

stmt = self.get_count_query_for_request(request)

This will make things work, and it will give people a hook to properly filter based on the logged user (their id). And, the best of all: it is backwards-compatible.

Additional context Without the support above, I am force to do this every time I upgrade starlet-admin:

1. Copy both methods into my own custom subclass called WorakaroundModelView, fixing imports and hoping things will still work

2. Find the spots and apply the changes again

[jowilf]

How about we wrap the calls to get_list_query and get_count functions inside a try-catch block? In the try block, we can pass the request and to maintain backward compatibility, we will catch the Too Many Arguments error and call the functions without the request, but with a deprecation warning.

[sglebs]

I once was in a talk by a famous software developer and he was covering a DO/DONT list. In the DONT list, was the usage of exceptions as a control flow mechanism. So, I never do.

To be honest, I prefer my solution because there is no magic involved, it is bread&butter OO, and backwards-compatible. And very K.I.S.S.

You can put the deprecation warnings in the 2 new methods in views.py.ModelView

But either way, I am looking forward to having this, thank you SO MUCH for listening! Super happy here.

[jowilf]

In the DONT list, was the usage of exceptions as a control flow mechanism.

I agree with you, but adding those two methods may cause confusion. Instead, we can introduce a breaking change in release 0.14.

[jowilf]

thank you SO MUCH for listening! Super happy here.

Thank you for your kind words. I'm glad to know that you are enjoying being here.

[GuilhermeGZanetti]

I agree with the OP, the first suggestion seems better, and very much needed for this kind of filter.