Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug: delete_where fail sanity check when a service is initialized with statment #286

Open
1 of 4 tasks
noamsto opened this issue Nov 12, 2024 · 0 comments
Open
1 of 4 tasks
Labels
bug Something isn't working lambda-stmt Related to `lambda_stmt` in SQLAlchemy. Not necessarily an `advanced-alchemy` bug.

Comments

@noamsto
Copy link
Contributor

noamsto commented Nov 12, 2024

Description

This is somewhat unexpected behavior,
Since delete is not using the base statement from the service, sanity checks may fail.

For example,
I have a service which limit the access by my tenant_id:

async def provide_document_access_received_service(
    request: "Request[UserAccount, Token, Any]", db_session: AsyncSession
) -> AsyncGenerator[ResourceAccessService, None]:
    """Provider for resource access service."""
    async with ResourceAccessService.new(
        statement=select(ResourceAccess)
        .where(ResourceAccess.tenant_id.in_(request.user.accessible_tenant_ids))
        .filter(ResourceAccess.resource.has(Resource.resource_type == ResourceType.DOCUMENT)),
        load=selectin_polymorphic(Resource, [Document]),
        session=db_session,
    ) as service:
        yield service

Now let's say I try to delete a resource which is not in my tenant, I expect to get NotFoundError, but in this case I'll get Sanity Check Failed exception.
Unless I'll provide the same filters I provided with the statement.

A possible solution, which I'm thinking to do and create a PR (if I'll have time)
Is have the new method get filters and build the statement internally, this way these "global" filters can be applied to the delete_where and the behavior will be more predictable.

URL to code causing the issue

No response

MCVE

No response

Steps to reproduce

No response

Screenshots

image

Logs

No response

Package Version

v0.23.1

Platform

  • Linux
  • Mac
  • Windows
  • Other (Please specify in the description above)
@noamsto noamsto added the bug Something isn't working label Nov 12, 2024
@cofin cofin added the lambda-stmt Related to `lambda_stmt` in SQLAlchemy. Not necessarily an `advanced-alchemy` bug. label Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working lambda-stmt Related to `lambda_stmt` in SQLAlchemy. Not necessarily an `advanced-alchemy` bug.
Projects
None yet
Development

No branches or pull requests

2 participants