Codecov Report

Merging #12660 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #12660      +/-   ##
==========================================
+ Coverage   96.32%   96.33%   +0.01%     
==========================================
  Files          95       95              
  Lines        9261     9262       +1     
  Branches       90       90              
==========================================
+ Hits         8921     8923       +2     
+ Misses        323      322       -1     
  Partials       17       17              

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4097aaa...961926c. Read the comment docs.

Hello! Thanks for catching this! Please fix the tests and I will merge, thanks!

MariaDb and MySQL tests are failing because of the foreign key, which require to truncate in cascade, so it seems to not have the cascade set properly ? is it. another issue because i didn't touch anything specific to an implementation

@bmeriaux Hmmm... Probably the fact that it was not being awaited caused the error to not be caught by CI. I agree this does not seem your fault.

Do you have any idea how to solve it, though?

MariaDb and MySQL tests are failing because of the foreign key, which require to truncate in cascade, so it seems to not have the cascade set properly ? is it. another issue because i didn't touch anything specific to an implementation

After reading the MySQL doc, it does not support truncate with cascade on foreign key, so how to proceed ? add a if on dialect for the option to do a bulkDelete for maria and mysql ?

for. what i see here: https://dev.mysql.com/doc/refman/8.0/en/truncate-table.html and here: https://docs.microsoft.com/en-us/sql/t-sql/statements/truncate-table-transact-sql?view=sql-server-ver15
there is no possibility to use truncate on a table with foreign key, I see 3 choices:

• disable my test for MySQL, Maria and MSSQL, to keep the actual behavior which work when no FK for maria and mysql but will throw an explicit error when a FK is on the table
• try to detect foreign key in query-interface.truncateTableQuery() for Mysql, MariaDB and MSSQLto do a delete instead, but it will be very slow on big table or disable contrainst but it needs to be only when truncate all tables
• disable truncate with cascade on Mysql, MariaDB, MSSQL dialect

Let me ping @jedwards1211, maybe he will have an idea of why this await was missing in the first place :)

Let me ping @jedwards1211, maybe he will have an idea of why this await was missing in the first place :)

without it, i have a lot of deadlock error with postgres when truncating models with foreign key in cascade, because multiple truncate try to cascade on the same reference table at the same time

Thanks for the investigation @bmeriaux, intuitively I think your first bullet is the best option, but can you please do an extra investigation: does the sequelize documentation claim anything about truncate working or not working in mysql/mariadb/mssql?

We must give users whatever our docs says (or, maybe, change the docs)

Let me ping @jedwards1211, maybe he will have an idea of why this await was missing in the first place :)

without it, i have a lot of deadlock error with postgres when truncating models with foreign key in cascade, because multiple truncate try to cascade on the same reference table at the same time

Yes, I am sure the await is necessary, I decided to ping him because he was the one who made the transition in our codebase from promises to async/await, maybe when seeing this he might think of other places a similar mistake may have happened (or maybe the issue already existed before using async/await)

i added an option in dialect support truncateCascade, and use it when there are a model with at least an association (FK) to use DELETE instead of truncate
but it will not solve individual Model truncate with cascade on Model with FK with MSSQL, MariaDB and MySQL dialect, an explicit DB error will still be thrown

Do you know where the original code went? I don't see any missing awaits in the diff...seems totally plausible that one was missing though because there were a few heisenbugs in the refactoring

explicit DB error will still be thrown

@bmeriaux Yeah, that's fine, if the underlying database does not support it, it is ok to have an error in Sequelize as well.

@jedwards1211 It's this line in src/sequelize.js (former lib/sequelize.js)

finally, i removed the "best effort" to support truncate with cascade option set to true for dialect which does not support it,
it is more explicit and closest to the reality

okay, yup I don't remember if I automated that case of refactoring or not but if so, what must have tripped it up is that the iteratee was declared as a separate variable instead of provided as an inline function. I looked over the rest of that commit for good measure and didn't manage to find any other mistakes like this at least

@jedwards1211 Hmm, I see. Can you give me the link to that commit? I'd like to take a look too

sure: https://github.com/sequelize/sequelize/pull/12074/files

@bmeriaux Thanks again for the great work!!

I want to make sure nothing here is a breaking change, I want to make more tests and take a closer look at the code (not only your changes but look at how things worked before), so I will not merge this PR yet.

However, since it is very obvious that this missing await is a fatal mistake, I would like to ask you a favor:

• Please open a new PR in which you do just a single line change, adding the missing await, no need to create any test
• Please do the same as the above bullet but targeting the v6 branch instead of the master branch

I will merge those two as long as they don't break any existing tests, since it is very obvious that the await is necessary.

This way your issue is fixed asap while I don't risk making breaking changes unexpectedly.

@bmeriaux Thanks again for the great work!!

I want to make sure nothing here is a breaking change, I want to make more tests and take a closer look at the code (not only your changes but look at how things worked before), so I will not merge this PR yet.

However, since it is very obvious that this missing await is a fatal mistake, I would like to ask you a favor:

* Please open a new PR in which you do just a single line change, adding the missing `await`, no need to create any test

* Please do the same as the above bullet but targeting the `v6` branch instead of the `master` branch

I will merge those two as long as they don't break any existing tests, since it is very obvious that the await is necessary.

This way your issue is fixed asap while I don't risk making breaking changes unexpectedly.

it is done with #12663 and #12664

I rebased this one on master

Hey there :)

First of all: Thanks a bunch for your contribution to Sequelize! Much appreciated!
Second: Unfortunately we haven't had the chance to look into this ever since you created it. Sorry for that!

A couple of months ago, we have switched from master to main branch as our primary development branch and hence this PR is now outdated :(

If you still think this change is making sense, please consider recreating the PR against main. Thanks in advance and sorry for the additional work.

✌️