MSSQL removeColumn method fix for primaryKey column#7081
MSSQL removeColumn method fix for primaryKey column#7081sushantdhiman merged 4 commits intosequelize:masterfrom
Conversation
…fore dropping primarykey column
Current coverage is 94.82% (diff: 100%)
|
ace2fc6 to
f1a159e
Compare
|
@janmeier @sushantdhiman @felixfbecker Request review |
| return sql; | ||
| }, | ||
|
|
||
| getPrimaryKeyConstraint(table, attributeName) { |
There was a problem hiding this comment.
This method only generates SQL, lets call it getPrimaryKeyConstraintQuery
|
|
||
| getPrimaryKeyConstraint(table, attributeName) { | ||
| const tableName = wrapSingleQuote(table.tableName || table); | ||
| const sql = [ |
There was a problem hiding this comment.
You can directly return query without using sql variable :)
There was a problem hiding this comment.
@sushantdhiman I wanted to keep the query readable yet copy pastable when logged so went for this. Using backticks keeps the same formatting as defined when logged.
There was a problem hiding this comment.
I have no issue with Array based query generation and joining it, I would mostly encourage it for query generation.
My point was you put join result in a variable and return that variable, why don't directly return output of join directly
There was a problem hiding this comment.
Ok sure, I'll make the mentioned changes :)
| }); | ||
| }); | ||
|
|
||
| it('Should be able to remove a column with primaryKey', function () { |
| }); | ||
| }); | ||
|
|
||
| it('Should be able to remove a column with primaryKey', function () { |
Pull Request check-list
npm run testornpm run test-DIALECTpass with this change (including linting)?Futurein the changelog?Description of change
When the
removeColumnmethod is called on a primaryKey column, SQL Server errors as the primaryKey column has an associated constraint which should be dropped before dropping the primaryKey column.getPrimaryKeyConstraintmethod to retrieve primary key constraint based on tableName and columnName.