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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change UniqueConstraint for SQL Server (and Postgres) database support #6607

Closed
wants to merge 1 commit into from

Conversation

davidjb
Copy link
Contributor

@davidjb davidjb commented Dec 3, 2020

This fixes #6393 by modifying the constraint to use an IN condition which supports both Postgres and SQL Server. Previously, the | (OR) condition was only supported by Postgres because SQL Server only supports AND conditions, meaning that migrations would crash mid-way through on the latter.

The implementation follows suggestions from @gasman in #6393 (comment):

  • Migration 0050 is modified to not break on SQL Server
  • Added migration 0060 to add or replace the constraint in a manner that supports both of these databases

Additionally, this allows for and documents a DATABASE_DRIVER env variable to be set for testing, to allow a different SQL Server driver (e.g. FreeTDS on Mac/Linux); and adds the specific host_is_server option for FreeTDS (won't affect SQL Server Native Client on CI).

Note: 8 other database-related tests are failing (see https://gist.github.com/davidjb/be2849b5bbb5d4c3e3db1e317d644a17 for log) when tested against SQL Server; this could be a FreeTDS driver issue or something else but they don't seem related to this PR.

Other database backends (SQLite, MySQL) see their tests pass.

This PR depends on changes in #6585, hence why it has 2 commits present. Once that other is merged, it's just one.


Thanks for contributing to Wagtail! 馃帀

Before submitting, please review the contributor guidelines https://docs.wagtail.io/en/latest/contributing/index.html and check the following:

  • Do the tests still pass? (https://docs.wagtail.io/en/latest/contributing/developing.html#testing) - yes existing tests do on other database engines; please see comments
  • Does the code comply with the style guide? (Run make lint from the Wagtail root)
  • For Python changes: Have you added tests to cover the new/fixed behaviour? - the fact the tests run?
  • For front-end changes: Did you test on all of Wagtail鈥檚 supported browsers? Please list the exact versions you tested.
  • For new features: Has the documentation been updated accordingly?

@squash-labs
Copy link

squash-labs bot commented Dec 3, 2020

Manage this branch in Squash

Test this branch here: https://davidjbfix-mssql-migration-b1acg.squash.io

@davidjb davidjb changed the title Change UniqueConstraint for wider database support Change UniqueConstraint for SQL Server (and Postgres) database support Dec 3, 2020
@tomdyson
Copy link
Contributor

tomdyson commented Dec 3, 2020

Thanks @davidjb! This looks really good. Can you give us any advice on how to test against SQL Server, either locally or in CI? I don't think any of the core team have much SQL Server expertise.

@davidjb
Copy link
Contributor Author

davidjb commented Dec 3, 2020

@tomdyson Thanks for the feedback.

For sure. Here鈥檚 my cheat sheet for running wagtail tests on all the various db backends: https://gist.github.com/davidjb/643272824f469386f933e7a4e081a81e - SQL Server is down the bottom.

I spin up a Docker container for each server - thankfully SQL Server was released for Linux, install the requisite engine and client and run the tests. In this case, the install instructions are for Mac but adaptable for Linux - not sure about Windows. For CI, you could run the Docker container and then the tests; I鈥檝e done this style of thing on Travis. If that doc is helpful, feel free to take it for the wagtail docs or I can submit a PR.

You鈥檒l notice things are a little messy at the moment on the latest version of Django 3.1 because of the engine needing fixes for db internal changes. I鈥檝e got pull requests open to the engine repo at ESSolutions/django-mssql-backend#86 which will be merged at some point (code is broken in 3.1+ without it), hence the temporary use of a branch in my fork. Otherwise you could use the backend鈥檚 current master branch with Django 3.0 in tests

Edit: ESSolutions/django-mssql-backend#86 was now merged; I've updated my gist accordingly. The change hasn't been released yet, still requires installing from git.

This fixes wagtail#6393 by modifying the constraint to use an IN condition
which supports both Postgres and SQL Server. Previously, the `|` (OR)
condition was only supported by Postgres because SQL Server only
supports AND conditions.

The implementation follows suggestions from @gasman in
wagtail#6393 (comment):

* Migration 0050 is modified to not break on SQL Server
* Added migration 0060 to add or replace the constraint

Additionally, this allows for and documents a `DATABASE_DRIVER` env
variable to be set for testing, to allow a different SQL Server driver
(e.g. FreeTDS on Mac/Linux); and adds the specific `host_is_server`
option for FreeTDS (won't affect SQL Server Native Client on CI).
@davidjb
Copy link
Contributor Author

davidjb commented Jan 12, 2021

Rebased on the latest master so it's able to be merged; anything else you'd like me to change or update?

@tomdyson tomdyson requested a review from gasman January 12, 2021 10:52
gasman pushed a commit that referenced this pull request Jan 18, 2021
This fixes #6393 by modifying the constraint to use an IN condition
which supports both Postgres and SQL Server. Previously, the `|` (OR)
condition was only supported by Postgres because SQL Server only
supports AND conditions.

The implementation follows suggestions from @gasman in
#6393 (comment):

* Migration 0050 is modified to not break on SQL Server
* Added migration 0060 to add or replace the constraint

Additionally, this allows for and documents a `DATABASE_DRIVER` env
variable to be set for testing, to allow a different SQL Server driver
(e.g. FreeTDS on Mac/Linux); and adds the specific `host_is_server`
option for FreeTDS (won't affect SQL Server Native Client on CI).
@gasman
Copy link
Collaborator

gasman commented Jan 18, 2021

Thanks @davidjb - all looks good to me! Merged in 3a5b725.

@gasman gasman closed this Jan 18, 2021
@davidjb
Copy link
Contributor Author

davidjb commented Jan 18, 2021

Thanks @gasman - much appreciated. I also created the work-in-progress ESSolutions/django-mssql-backend#97 with the database backend to catch situations where an unsupportable constraint is present.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migration fails with MS SQL
3 participants