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

mssql: schema builder - attempt to drop default constraints when changing default value on columns #4321

Merged
merged 3 commits into from
Feb 28, 2021
Merged

mssql: schema builder - attempt to drop default constraints when changing default value on columns #4321

merged 3 commits into from
Feb 28, 2021

Conversation

dhensby
Copy link
Contributor

@dhensby dhensby commented Feb 26, 2021

Depends on #4319

When changing a default value on a column in mssql, the previous default constraint needs to be dropped before a new one can be assigned. This PR attempts to drop the constraint and then adds the constraint via a separate query:

DROP CONSTRAINT [constraint_name]; //does some look up to find the constraint name
ALTER TABLE [table] ALTER COLUMN ...; // other altering of column
ALTER TABLE [table] ADD CONSTRAINT [constraint_name] DEFAULT [defaultVal] FOR [column_name]; // applies default constraint

Help required:

I've tried to get a grasp of how the internals of the builder work for this and implemented this in a way that I think makes the most sense.

Under some circumstances this change can result in a needless alter query being run when the only change to a column is its default value; in this case we should only be running the drop and add constraint queries, but I'm not clear on how best to skip the other query that is produced under these circumstances.

To do:

  • Create integration tests to cover this feature specifically

@kibertoad
Copy link
Collaborator

      -ALTER TABLE [users] ADD CONSTRAINT [users_foo_default] DEFAULT test FOR [foo]
      +ALTER TABLE [users] ADD CONSTRAINT [users_foo_default] DEFAULT 'test' FOR [foo]

Looks like escaping changed.

@kibertoad kibertoad merged commit d807832 into knex:master Feb 28, 2021
@kibertoad
Copy link
Collaborator

Thanks!

@kibertoad
Copy link
Collaborator

Released in 0.95.0

This was referenced Mar 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants