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

Query accessible records: SQLAlchemy 2.0 syntax #380

Merged
merged 3 commits into from
Feb 28, 2024

Conversation

Theodlz
Copy link
Collaborator

@Theodlz Theodlz commented Feb 23, 2024

I noticed that when using the sqlachemy 2.0 syntax to create CustomUserAccessControls on a table, I couldn't commit().

This is because commit() calls a method that only supported the sa.Query type of objects, and not the new sa.Select that you are expected to use in SQLAlchemy 2.0

@Theodlz Theodlz requested a review from stefanv February 23, 2024 21:01
@stefanv
Copy link
Contributor

stefanv commented Feb 23, 2024

Great detective work! Do you have a sense of how we can test this?

@Theodlz
Copy link
Collaborator Author

Theodlz commented Feb 25, 2024

Great detective work! Do you have a sense of how we can test this?

it’s a little tricky to test for sure. For that we would need one or 2 model(s) so that we can have 2 custom accessibility rules on update: one using DBSession.query() and one using sa.select().

In SP most of these rules are outdated and use DBSession and not sa.select(). So as long as what works still works and what is now written with sa.select() works too, we are covered.

But I 100% agree that some kind of dedicated test could be good.

Maybe we can actually test this without using these accessibility rules at all, just emulate whatever would be the input of these 2 methods I edited to take in both kind of inputs. Let me see if I can figure something out tomorrow.

@stefanv stefanv merged commit 5648054 into main Feb 28, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants