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 SQLite foreign key constraints when altering a table #4189
Fix SQLite foreign key constraints when altering a table #4189
Conversation
It's preferable to avoid doing that (although we have cases of this, but we intend to refactor that out). The reason for that - knex aims to be a querybuilder, and certain projects use it that way, simply generating SQL statements that they execute on their own. Ideally that should be preserved.
See #3033
Why not? That sounds confusing for the user.
Why should it be default? Isn't it something that user should opt-in on their own? I'm open to exposing this config as an option param, though. |
@nickrum A heads-up - I'm currently reworking foreign key creation to output proper SQL, you should be able to reuse same solution after it is done. |
But in order to unblock you for now, since changes might take some time, feel free to execute changes directly on client for now if that makes things easier for you. |
@kibertoad Thank you for the info and the heads-up. In that case I will wait until that PR is merged as I don't think it would be possible to output proper SQL for the foreign key check only without also refactoring the whole DDL generation. I did find a way to essentially do what this PR currently does with generated SQL statements, but there is one catch: As, to my knowledge at least, it is not possible to enable/disable the foreign key check based on a previous query, the SQL statements have to be generated based on a query of "
I see, thanks. Is there a way to disable processing without a transaction? The function doesn't seem to exist on the client directly.
Because I didn't find the time to implement this yet. It is a bit more complicated as it requires modifying the
Mainly because of #453 (comment). I am not sure if users expect the check to be turned on by default because of the way other database vendors do it or the opposite because it is the default in SQLite. In any case, a config option sounds like a good idea. |
@nickrum PR was just merged!
That's not a problem. Check how that PR is implemented, there is an async function involved that generates query SQL based on results of previous queries, same approach should work for you.
Where do you need it? You'll have an instance of transaction inside ddl. |
c48ac42
to
e5e4216
Compare
@nickrum Awesome! Do you need help writing a test? |
Inside
I would appreciate any help regarding tests. I still have to port the foreign keys check to the new
|
@nickrum New style of integration tests are actually quite easy to run. Since you are only interested in SQLite, what you can do is mark your test suite with |
In order for the new method to work, make sure to update
|
e5e4216
to
d8b7f31
Compare
@kibertoad Thanks, I think I will be able to figure this out. |
@nickrum If you'll get stuck with writing tests, please let me know, would be happy to help. |
Friendly ping :). Happy to pick up any pieces of remaining work if you need help with anything. |
@kibertoad sorry for the delay :) The beginning of a year is always a little busy. I also had a few issues implementing the foreign keys check for generated SQL statements, but I think I got this now. If you could pick up writing some tests, that would help a lot. |
d8b7f31
to
1b5b2a5
Compare
@kibertoad I finally found the time to implement my idea regarding foreign keys check with generated SQL statements. Unfortunately it is not exactly intuitive. I had to create temporary tables and triggers to dynamically rollback the changes if a foreign key error occurs. There are two independent checks because the manual foreign keys check can report violations in two different ways. Either the check returns a list of violations for every row that violates a foreign key constraint, or the check command simply fails if a whole column referenced by some other column can not be found. The change to |
@nickrum Does the new logic only work when executed on client directly? Is it not possible to generate SQL for all of the operations that users could execute themselves, so that they could use knex purely as a query builder? |
It should work without the client as long as every generated SQL is executed regardless of any errors. This is definitely not ideal but, to my knowledge at least, there is no easy way around this as the foreign key constraint enforcement definitely has to be reenabled after the transaction. I can only think of two alternatives. One would be to basically execute the whole procedure in |
@kibertoad Let me know which approach you prefer or if you can think of another way to go about this. |
@nickrum I would say that with the query builder approach it is not possible to have a completely safe solution, so I would argue that if user goes for query generation and executes it manually, they should be responsible for wrapping it into transaction and rolling back properly should any part of it (such as executing the foreign check) fails. When we are executing it directly (using |
@kibertoad Good point. What about the problem that |
@nickrum Yeah, that is definitely a problem. I don't think that a good solution for that exists; considering that we don't know when and how user will execute generated SQL, we can't really provide him with an easy way to rollback their changes, so I would say that we should simply throw an error and let the user handle situation in this case. |
@nickrum Do you think that there is anything left, other than fixing tests? |
@kibertoad Sounds good to me. Regarding executing the queries directly inside After finding a solution for that, fixing the tests and writing new tests to verify the added functionality, this PR should be good to go. |
@nickrum Any chance you could create a preliminary PR changing signature of |
@kibertoad Sure! I feel like it is not clear which queries should be executed inside a transaction and which queries must not if {
pre: [],
sql: [],
post: []
} This way, if foreign key constraint enforcement is disabled, the user could simply ignore the |
@nickrum Sounds like a great idea! |
What is the status of this PR ? @nickrum |
1b5b2a5
to
970e2bb
Compare
@kibertoad @OlivierCavadenti Sorry this took so long! I made several attempts throughout the year at finishing this PR but there were a lot of show-stoppers preventing me from doing so. Initially I thought I could use the SQLite's |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
In just pushed a small commit that wraps the |
No problem, if I ask, it's to know if we can finish the work or not, I understand that everyone is busy ! |
@kibertoad hate to be that guy, but do you think we could sneak in a 0.95.15 before the big 1.0 release? 😬 Between this and #4923, a couple mission critical bugfixes for SQLite use have made it in to the project, but aren't available for use yet. Totally understand if v1 is the exclusive focus rn tho! ✌🏻 |
@rijkvanzanten Sure. can you give a laundry list of prs you want in it? |
@kibertoad This one is by far the most important (without, doing any table alterations drops foreign key columns completely, resulting in critical data loss). Other than that, #4923 would be nice to have, as it's a bug without a workaround. Anything else would be cherry on the cake 🙂 |
Released in 1.0.1. @rijkvanzanten Do you still need 0.95 backport, or 1.x is sufficient? |
@kibertoad |
Some notes/questions:
disableProcessing()
/enableProcessing()
ingetTableSql()
? It seems like this does not work when using the client directly instead of through a transaction.reinsertMapped()
should be treated like a critical section, so another invocation of the function can't interfere with the procedure of querying, disabling and reenabling foreign key constraint enforcement.Fixes #4155