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

Apply fix for count of group by query #13

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dwasyl
Copy link

@dwasyl dwasyl commented Nov 13, 2019

This addresses the issues I raised in #12 and is simply reapply a fix proposed upstream at nattster@b095b20 and michiya@761a3a8 that didn't seem to make it into this fork.

@OskarPersson
Copy link
Collaborator

Looking at the Linux job on Travis, we see that this generates a new error when running the tests:

======================================================================
ERROR: test_alter_field_m2m (migrations.test_operations.OperationTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/ESSolutions/django-mssql-backend/django/django/db/backends/utils.py", line 84, in _execute
    return self.cursor.execute(sql, params)
  File "/home/travis/build/ESSolutions/django-mssql-backend/sql_server/pyodbc/base.py", line 592, in execute
    return self.cursor.execute(sql, params)
pyodbc.ProgrammingError: ('42000', "[42000] [Microsoft][ODBC Driver 17 for SQL Server][SQL Server]Could not drop object 'test_alflmm_stable' because it is referenced by a FOREIGN KEY constraint. (3726) (SQLExecDirectW)")
The above exception was the direct cause of the following exception:
Traceback (most recent call last):
  File "/home/travis/build/ESSolutions/django-mssql-backend/django/tests/migrations/test_operations.py", line 31, in tearDown
    self.cleanup_test_tables()
  File "/home/travis/build/ESSolutions/django-mssql-backend/django/tests/migrations/test_operations.py", line 40, in cleanup_test_tables
    'table': editor.quote_name(table_name),
  File "/home/travis/build/ESSolutions/django-mssql-backend/sql_server/pyodbc/schema.py", line 675, in execute
    cursor.execute(sql, params)
  File "/home/travis/build/ESSolutions/django-mssql-backend/django/django/db/backends/utils.py", line 67, in execute
    return self._execute_with_wrappers(sql, params, many=False, executor=self._execute)
  File "/home/travis/build/ESSolutions/django-mssql-backend/django/django/db/backends/utils.py", line 76, in _execute_with_wrappers
    return executor(sql, params, many, context)
  File "/home/travis/build/ESSolutions/django-mssql-backend/django/django/db/backends/utils.py", line 84, in _execute
    return self.cursor.execute(sql, params)
  File "/home/travis/build/ESSolutions/django-mssql-backend/django/django/db/utils.py", line 89, in __exit__
    raise dj_exc_value.with_traceback(traceback) from exc_value
  File "/home/travis/build/ESSolutions/django-mssql-backend/django/django/db/backends/utils.py", line 84, in _execute
    return self.cursor.execute(sql, params)
  File "/home/travis/build/ESSolutions/django-mssql-backend/sql_server/pyodbc/base.py", line 592, in execute
    return self.cursor.execute(sql, params)
django.db.utils.ProgrammingError: ('42000', "[42000] [Microsoft][ODBC Driver 17 for SQL Server][SQL Server]Could not drop object 'test_alflmm_stable' because it is referenced by a FOREIGN KEY constraint. (3726) (SQLExecDirectW)")

@dwasyl
Copy link
Author

dwasyl commented Nov 14, 2019

@OskarPersson Any idea what in this would impact FK constraints? Admittedly, it's a bit beyond my internal knowledge of MSSQL/DB backends, but I don't mind plugging away if I have a general direction. On a working level, the patch seems to address a few issues and I haven't had any issue with M2M fields in my tests (although clearly there are issues somewhere).

@OskarPersson
Copy link
Collaborator

OskarPersson commented Nov 14, 2019

I have now tried to run this locally and it seems that test_alter_field_m2m is just a "flaky" test with this library, will have to look into that separately, i.e. we can ignore this error here.

Though I would prefer to see some change in the test results before merging this, ensuring that this works as it should.

@OskarPersson
Copy link
Collaborator

@dwasyl Can you add a test that fails without the change and succeeds after similar to the one in #15?

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.

2 participants