loopbackio / loopback-connector-mssql

LoopBack connector for Microsoft SQL Server
http://loopback.io/doc/en/lb3/SQL-Server-connector.html
Other
52 stars 84 forks source link

Execute alterTable statements in order #227

Closed ataft closed 4 years ago

ataft commented 4 years ago

async.each() in alterTable executes the statements in parallel, allowing for things like creating an index before dropping it. Change async.each to async.eachSeries in order to execute the statements in proper order.

Fixes #180

slnode commented 4 years ago

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

dhmlau commented 4 years ago

@strongloop/sq-lb-apex , there's an existing PR https://github.com/strongloop/loopback-connector/issues/168. If it's fixed in loopback-connector, do we still need it for particular connector? Thanks.

ataft commented 4 years ago

Yes, we need it because the .each logic is in the alterTable function of this connector, which is called by autoupdate in loopback-connector.

Also, many of the connectors override automigrate/autoupdate from loopback-connector. In many cases, each vs. eachSeries isn't critical because it's just the order of the models. In this case, it's critical because it's the order of SQL statements. For example, an index has to be dropped before it's created.

dhmlau commented 4 years ago

@ataft, thanks for your contribution. Could you please add some tests to verify your changes? There's a similar PR: https://github.com/strongloop/loopback-connector/pull/170 which can be used as your reference.

Also, please fix the commit linter error as well:

**************************************************
**
**  Linting commit logs
**
**  1 problems found:
**    3daa474 - Execute alterTable statements in order: line 3 longer than 72 characters.
**
**************************************************

Thanks!

ataft commented 4 years ago

@dhmlau the current test covers this scenario because it does an automigrate (create the table) and then an autoupdate, which will call alterTable. BTW, this test should be failing intermittently, but given that the order is random it'd be hard to catch.

Regarding the commit linter error, what exactly is line 3? I only made one simple change to line 343.

dhmlau commented 4 years ago

@ataft, the error is about the commit message, not the actual change. It seems like there are 4 lines of commit message?

Execute alterTable statements in order

async.each() in alterTable executes the statements in parallel, allowing for things like creating an index before dropping it. Change async.each to async.eachSeries in order to execute the statements in proper order.
ataft commented 4 years ago

Yeah, not sure how to do that. There is something called word wrap that's pretty great, it wraps text so people can read it. Submitted new PR: https://github.com/strongloop/loopback-connector-mssql/pull/228

dhmlau commented 4 years ago

Thanks. As a future reference, you can also amend the commit message by following the instructions here: https://help.github.com/en/github/committing-changes-to-your-project/changing-a-commit-message