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

Migration fails with MS SQL #6393

Closed
rjschave opened this issue Sep 21, 2020 · 7 comments
Closed

Migration fails with MS SQL #6393

rjschave opened this issue Sep 21, 2020 · 7 comments

Comments

@rjschave
Copy link

rjschave commented Sep 21, 2020

Issue Summary

Wagtail versions < 2.10 have worked with Microsoft SQL Server. Wagtail 2.10 introduced a change (related to configurable moderation workflow) that is not supported by Microsoft SQL Server. Specifically, Microsoft SQL Server does not support the OR keyword in the WHERE clause for unique indexes.

Migration 0050 in Wagtail Core (issue #6047 and commit 74a8cf8) only allows one record with status of In Progress OR Needs Changes per page.

migrations.AddConstraint(
    model_name='workflowstate',
    constraint=models.UniqueConstraint(condition=models.Q(('status', 'in_progress'), ('status', 'needs_changes'), _connector='OR'), fields=('page',), name='unique_in_progress_workflow'),
),

This migration generates the following SQL statement, which is invalid:

CREATE UNIQUE INDEX [unique_in_progress_widget]
    ON [widgets_widgetstate] ([widget_id])
    WHERE ([status] = 'in_progress' OR [status] = 'needs_changes');

Steps to Reproduce

  1. Start a new project with wagtail start myproject.
  2. Configure Django to connect to MS SQL server.

Example:

DATABASES = {
    'default': {
        'ENGINE': 'sql_server.pyodbc',
        'HOST': 'localhost',
        'PORT': '',
        'NAME': 'database-name',
        'USER': 'username',
        'PASSWORD': 'password',
        'OPTIONS': {
            'driver': 'ODBC Driver 17 for SQL Server',
        },
    },
}
  1. Run migrate command.

Error message:

 Applying wagtailcore.0050_workflow_rejected_to_needs_changes...Traceback (most recent call last):
  File "/Users/rjschave/pycharm-projects/clients/rjs/wagtailtest2/venv/lib/python3.8/site-packages/django/db/backends/utils.py", line 86, in _execute
    return self.cursor.execute(sql, params)
  File "/Users/rjschave/pycharm-projects/clients/rjs/wagtailtest2/venv/lib/python3.8/site-packages/sql_server/pyodbc/base.py", line 553, in execute
    return self.cursor.execute(sql, params)
pyodbc.ProgrammingError: ('42000', "[42000] [Microsoft][ODBC Driver 17 for SQL Server][SQL Server]Incorrect syntax near the keyword 'OR'. (156) (SQLExecDirectW)")

I have confirmed that this issue can be reproduced as described on a fresh Wagtail project: Yes

I have also confirmed this problem is not specific to Wagtail. A unique constraint with the OR keyword on any model (allowed by Django) will fail with MS SQL Server.

This is the only reference I could find in published Wagtail documentation related to Microsoft SQL Server - https://docs.wagtail.io/en/v2.10.1/advanced_topics/performance.html#database

I know Microsoft SQL Server is not officially supported by the Django or Wagtail projects, but until Wagtail version 2.10, it has worked great!

Is it possible to get Wagtail working with Microsoft SQL Server again?

Technical details

  • Python version: 3.8.1
  • Django version: 3.0.10
  • Wagtail version: 2.10
  • pyodbc verson: 4.0.30
  • ODBC Driver 17 for SQL Server
  • django-mssql-backend version: 2.8.1
  • SQL Server 2017 (but relevant for any version of MS SQL supported by django-mssql-backend)
@gasman
Copy link
Collaborator

gasman commented Sep 30, 2020

Hi @rjschave - thanks for the report. This sounds like a bug / limitation in django-mssql-backend - since the SQL here is generated on the Django side rather than within Wagtail itself, it would be django-mssql-backend's responsibility to translate the ORM expression into valid MS SQL syntax, or providing a best approximation in the case that the functionality doesn't exist (e.g. skipping the constraint entirely). On that basis, I think it would be appropriate to open this as an issue on https://github.com/ESSolutions/django-mssql-backend/. Meanwhile, I'll update the "known to work" note in the docs.

@gasman gasman closed this as completed Sep 30, 2020
gasman added a commit that referenced this issue Sep 30, 2020
@davidjb
Copy link
Contributor

davidjb commented Nov 23, 2020

I've hit the same issue. I agree that the SQL Server backend needs to ignore this constraint given this resultant syntax (working on that now), but it could be supported on SQL Server if it didn't use OR and used IN instead.

@gasman Is that a change that could be made to Wagtail? This syntax would still support Postgres.

@gasman
Copy link
Collaborator

gasman commented Nov 23, 2020

@davidjb Sure - I have no way to test, but if you can find an alternative definition for the constraint in models.py that works in SQL Server while still keeping the same behaviour, then I'd be happy to accept a PR. Note that we'll need to take care that the outcome of the migrations remains consistent for everyone whether they run them from scratch or upgrade from the current version, so retrospectively changing migration 0050 wouldn't be enough - probably the best approach is to fix migration 0050 (so that it doesn't break on MS SQL) and add an extra migration to drop and recreate the constraint (which will effectively be a no-operation for people running the 'new' migration 0050, but will ensure that users who upgrade also have the change applied).

@gasman gasman reopened this Nov 23, 2020
@rjschave
Copy link
Author

@gasman I can help test (both new installs and upgrades).

@quick-chipmunk
Copy link

@gasman

It can be done by changing existing index condition in 0050 migration:
constraint=models.UniqueConstraint(condition=models.Q(('status', 'in_progress'), ('status', 'needs_changes'), _connector='OR'), ...
to this one:
constraint=models.UniqueConstraint(condition=models.Q(status__in=['in_progress', 'needs_changes']), ...

After this change migration successfully works for MSSQL and (I think) for others as well.
Sorry I'm not ready for creating PR now.

@davidjb
Copy link
Contributor

davidjb commented Nov 30, 2020

Just a quick update - I’ve written the PR, I’m just ensuring the tests run on Postgres and SQL Server. I’ve got a 2nd PR in the works for modifying some of the test environment variables that I needed; should be done later today, assuming no major test issues.

davidjb added a commit to davidjb/wagtail that referenced this issue Dec 3, 2020
This modifies the constraint mentioned in wagtail#6393 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 added a commit to davidjb/wagtail that referenced this issue Dec 3, 2020
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 added a commit to davidjb/wagtail that referenced this issue Dec 3, 2020
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 added a commit to davidjb/wagtail that referenced this issue Dec 3, 2020
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 added a commit to davidjb/wagtail that referenced this issue Jan 12, 2021
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).
@gasman gasman closed this as completed in 3a5b725 Jan 18, 2021
@davidjb
Copy link
Contributor

davidjb commented Jan 18, 2021

FYI I also opened ESSolutions/django-mssql-backend#97 with the database engine to catch a situation where an unsupportable constraint is being attempted to be created under SQL Server.

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

Successfully merging a pull request may close this issue.

5 participants