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

Fix truncate not actually truncating #909

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Ken-vdE
Copy link
Contributor

@Ken-vdE Ken-vdE commented Oct 26, 2024

This fixes an issue that causes postgres database to not actually truncate, so Id column auto-increment does not get reset to 1 and new models continue to use Id from existing increment.
This truncate query is actually faster and this fixes an issue where some tests fail (when using postgres db) because snapshot thinks new entities should have Id 1 when they do not.

Now, I'm very new to both Rust and Loco, so please inspect this code carefully and make sure I haven't done anything stupid.

This fixes an issue that causes postgres database to not actually truncate.
@jondot
Copy link
Contributor

jondot commented Oct 26, 2024

Thanks for this PR!
Your code looks good (building the statement probably needs some escaping for the table, or going through the SeaORM query builder to get that, but overall makes sense)
I think the semantics of truncate is to not restart identity (see rails: https://github.com/search?q=repo%3Arails%2Frails%20build_truncate_statement&type=code)
But the test errors I believe come from a change in how seeding is done (where seeding provides explicit ID), we're still trying to manage to get that ID to take effect in a different PR.
Either way moving to truncate is speedier, I'll leave that PR open until we locate the specific fix for seeding (in a different effort) and then see how to incorporate

@Ken-vdE
Copy link
Contributor Author

Ken-vdE commented Oct 26, 2024

Sounds good. Thanks for your quick response!
Yes, truncate is speedier. Furthermore, calling truncate on a database does something very different than running a delete-all query. So, if you're going to name the function 'truncate', then it should actually run truncate. Otherwise the function should probably just be named 'delete_all'. That also goes for the '_' case, in which I've left the delete_all statement. That should probably be replaced with an actual truncate equivalent too.

With regards to the 'restart identity'; yes, looking at your given link, Rails does not do that either. Looking at the postgres docs it's not the default behavior either. I come from MySQL, where that is the expected behavior, hence the confusion. That does not take away that there should probably be an easy way to truncate with an increment-reset and that it makes testing and asserting (or cleaning up before/after tests) a lot easier (if it does reset). It makes for better cleanup and feels like that should be expected behavior while running multiple tests.

@jondot
Copy link
Contributor

jondot commented Oct 27, 2024

you're right.
thinking ahead of incorporating this, I'm thinking about the topic of escaping and all kinds of edge case.
could you try using sea-query (which SeaORM uses under the hood) directly? looks like they have facilities to compose queries in a safe way: https://github.com/SeaQL/sea-query?tab=readme-ov-file#table-truncate

@Ken-vdE
Copy link
Contributor Author

Ken-vdE commented Oct 29, 2024

@jondot I've changed the hardcoded query string to a sea_query built string.
It's a little more verbose but safer. This escapes the table name.
Is this what you were looking for? If so, please triple check the code and make sure it's correct.

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.

3 participants