Opened 6 years ago
Closed 6 years ago
#29886 closed Bug (fixed)
PostgreSQL's unaccent lookup doesn't work if standard_conforming_strings is off
Reported by: | Tom McClure | Owned by: | Jayantha Gumballi |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | 2.1 |
Severity: | Normal | Keywords: | |
Cc: | Herbert Fortes | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | yes | UI/UX: | no |
Description
So, it turns out the postgres db server setting:
set standard_conforming_strings=off/on
affects whether backslashes are interpreted as escape characters in quoted strings.
To avoid being affected by this, SQL strings that contain backslashes should use "escape string" syntax (ie, prefix the quoted string with E a la E'some string') for 100% unambiguous interpretation.
Tried to use unaccent today and it only worked on a postgres db when the "standard_conforming_strings" setting was one way. Murphy's law, our prod db was set the other way.
Tracked the problem back to here, a bit of SQL that is not using E'string':
https://github.com/django/django/blob/master/django/db/backends/postgresql/base.py#L124
patch included
Attachments (3)
Change History (23)
by , 6 years ago
Attachment: | django_patch added |
---|
comment:1 by , 6 years ago
Cc: | added |
---|
comment:2 by , 6 years ago
Hello Tom,
I am a beginner who is interested to contribute to this issue.
However, I am not clear what the requirements are; am I just supposed to implement the patch in a pull request and get it merged, or am I supposed to write tests for this also?
Cheers!
comment:3 by , 6 years ago
I think there's already test coverage for this code, so, yeah, just use the patch to make a PR. Thanks!!
comment:4 by , 6 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:5 by , 6 years ago
Hello Tom,
I have made the PR, but one of the checks is failing, is this ok?
comment:6 by , 6 years ago
Hi,
The failing test is pr-mariadb/database=mysql_gis,label=mariadb,python=python3.7.
I do not see how is related to the patch:
Importing application django.contrib.gis Traceback (most recent call last): File "./runtests.py", line 484, in <module> options.exclude_tags, File "./runtests.py", line 274, in django_tests TestRunner = get_runner(settings) File "/home/jenkins/workspace/pr-mariadb/database/mysql_gis/label/mariadb/python/python3.7/django/test/utils.py", line 308, in get_runner test_module = __import__(test_module_name, {}, {}, test_path[-1]) ModuleNotFoundError: No module named 'xmlrunner' Build step 'Execute shell' marked build as failure Recording test results ERROR: Step ‘Publish JUnit test result report’ failed: No test report files were found. Configuration error? Finished: FAILURE
comment:9 by , 6 years ago
a test would probably have to do the following --
issue some setup/teardown SQL on the low-level postgres connection
- set up: execute "set standard_conforming_strings to off;"
- run the postgres/test_unaccent tests (especially the test_unaccent_chained test)
- tear down: execute "set standard_conforming_strings to on;" -- set it back to the default
follow-up: 12 comment:10 by , 6 years ago
Updated the PR to include the tests.
But when this test fails, it throws an error instead of an assertion error, is this ok?
follow-up: 13 comment:11 by , 6 years ago
I can not run this. "skipped 'PostgreSQL specific tests'"
Can you show the error message?
I am not a member. But as an observation, what I note
in other tests is with connection.cursor as cur:
instead of cur = connection.cursor()
There is no .close()
. And the import can be put at the
beginning, I think.
comment:12 by , 6 years ago
Replying to Jayantha Gumballi:
Updated the PR to include the tests.
But when this test fails, it throws an error instead of an assertion error, is this ok?
IMHO error v. failure isn't much different, but I cleaned up the test and now it explicitly fails without the fix. see latest attached patch.
test output without the fix
$ ./runtests.py --settings=test_postgresql postgres_tests Testing against Django installed in '/Users/tmcclure/code/django/django' with up to 8 processes Creating test database for alias 'default'… Cloning test database for alias 'default'… Cloning test database for alias 'default'… Cloning test database for alias 'default'… Cloning test database for alias 'default'… Cloning test database for alias 'default'… Cloning test database for alias 'default'… Cloning test database for alias 'default'… Cloning test database for alias 'default'… Creating test database for alias 'other'… Cloning test database for alias 'other'… Cloning test database for alias 'other'… Cloning test database for alias 'other'… Cloning test database for alias 'other'… Cloning test database for alias 'other'… Cloning test database for alias 'other'… Cloning test database for alias 'other'… Cloning test database for alias 'other'… System check identified no issues (0 silenced). .........................................................................................................................................................................................x.........x....x........x....................................................................................................................................................F....F.............................................................................. ====================================================================== FAIL: test_unaccent_chained_with_conforming_strings_off (postgres_tests.test_unaccent.UnaccentTest) ---------------------------------------------------------------------- Traceback (most recent call last): File "/usr/local/Cellar/python/3.7.0/Frameworks/Python.framework/Versions/3.7/lib/python3.7/unittest/case.py", line 59, in testPartExecutor yield File "/usr/local/Cellar/python/3.7.0/Frameworks/Python.framework/Versions/3.7/lib/python3.7/unittest/case.py", line 615, in run testMethod() File "/Users/tmcclure/code/django/tests/postgres_tests/test_unaccent.py", line 61, in test_unaccent_chained_with_conforming_strings_off self.fail("DatabaseError should not have occurred.") File "/usr/local/Cellar/python/3.7.0/Frameworks/Python.framework/Versions/3.7/lib/python3.7/unittest/case.py", line 680, in fail raise self.failureException(msg) AssertionError: DatabaseError should not have occurred. ====================================================================== FAIL: test_unaccent_chained_with_conforming_strings_off (postgres_tests.test_unaccent.UnaccentTextFieldTest) ---------------------------------------------------------------------- Traceback (most recent call last): File "/usr/local/Cellar/python/3.7.0/Frameworks/Python.framework/Versions/3.7/lib/python3.7/unittest/case.py", line 59, in testPartExecutor yield File "/usr/local/Cellar/python/3.7.0/Frameworks/Python.framework/Versions/3.7/lib/python3.7/unittest/case.py", line 615, in run testMethod() File "/Users/tmcclure/code/django/tests/postgres_tests/test_unaccent.py", line 61, in test_unaccent_chained_with_conforming_strings_off self.fail("DatabaseError should not have occurred.") File "/usr/local/Cellar/python/3.7.0/Frameworks/Python.framework/Versions/3.7/lib/python3.7/unittest/case.py", line 680, in fail raise self.failureException(msg) AssertionError: DatabaseError should not have occurred. ---------------------------------------------------------------------- Ran 442 tests in 1.068s FAILED (failures=2, expected failures=4) Destroying test database for alias 'default'… Destroying test database for alias 'default'… Destroying test database for alias 'default'… Destroying test database for alias 'default'… Destroying test database for alias 'default'… Destroying test database for alias 'default'… Destroying test database for alias 'default'… Destroying test database for alias 'default'… Destroying test database for alias 'default'… Destroying test database for alias 'other'… Destroying test database for alias 'other'… Destroying test database for alias 'other'… Destroying test database for alias 'other'… Destroying test database for alias 'other'… Destroying test database for alias 'other'… Destroying test database for alias 'other'… Destroying test database for alias 'other'… Destroying test database for alias 'other'…
test output with the fix
$ ./runtests.py --settings=test_postgresql postgres_tests Testing against Django installed in '/Users/tmcclure/code/django/django' with up to 8 processes Creating test database for alias 'default'… Cloning test database for alias 'default'… Cloning test database for alias 'default'… Cloning test database for alias 'default'… Cloning test database for alias 'default'… Cloning test database for alias 'default'… Cloning test database for alias 'default'… Cloning test database for alias 'default'… Cloning test database for alias 'default'… Creating test database for alias 'other'… Cloning test database for alias 'other'… Cloning test database for alias 'other'… Cloning test database for alias 'other'… Cloning test database for alias 'other'… Cloning test database for alias 'other'… Cloning test database for alias 'other'… Cloning test database for alias 'other'… Cloning test database for alias 'other'… System check identified no issues (0 silenced). .....................................................................................................................................................................................x.........x....x........x............................................................................................................................................................................................................................................ ---------------------------------------------------------------------- Ran 442 tests in 1.891s OK (expected failures=4) Destroying test database for alias 'default'… Destroying test database for alias 'default'… Destroying test database for alias 'default'… Destroying test database for alias 'default'… Destroying test database for alias 'default'… Destroying test database for alias 'default'… Destroying test database for alias 'default'… Destroying test database for alias 'default'… Destroying test database for alias 'default'… Destroying test database for alias 'other'… Destroying test database for alias 'other'… Destroying test database for alias 'other'… Destroying test database for alias 'other'… Destroying test database for alias 'other'… Destroying test database for alias 'other'… Destroying test database for alias 'other'… Destroying test database for alias 'other'… Destroying test database for alias 'other'…
comment:13 by , 6 years ago
Replying to Herbert Fortes:
I can not run this. "skipped 'PostgreSQL specific tests'"
Here's what I had to do to run the postgres-specific tests.
Bonus, this is not explicitly documented anywhere!
Install and start postgres, then:
createdb django_test createdb django_test_other
make a file tests/test_postgresql.py with the following contents:
DATABASES = { 'default': { 'ENGINE': 'django.db.backends.postgresql', 'NAME': 'django_test' }, 'other': { 'ENGINE': 'django.db.backends.postgresql', 'NAME': 'django_test_other' } } SECRET_KEY = "django_tests_secret_key" # Use a fast hasher to speed up tests. PASSWORD_HASHERS = [ 'django.contrib.auth.hashers.MD5PasswordHasher', ]
Finally, run just the postgres tests:
cd tests ./runtests.py --settings=test_postgresql postgres_tests
Can you show the error message?
See comment above.
I am not a member. But as an observation, what I note
in other tests iswith connection.cursor as cur:
instead ofcur = connection.cursor()
There is no
.close()
. And the import can be put at the
beginning, I think.
yes thank you, moved import and using "with" now. and the with will automatically close the cursor.
comment:15 by , 6 years ago
Hey People,
Tim commented on my PR that this test could go somewhere else, I didn't understand how it would be better if we did that,
any thoughts on that?
comment:16 by , 6 years ago
@tomj74@hpfn
In the next commit I am removing the try...finally part.
Since the test database gets destroyed anyway, no use in trying to set the standard_conforming_strings to its original value, so I am removing that part too.
Waiting to hear your thoughts.
comment:17 by , 6 years ago
Jayantha, you definitely want to keep the cleanup around else the per-connection setting override could leak between tests.
comment:18 by , 6 years ago
The test has:
try: if conforming_strings_state == 'on': cur.execute('SET standard_conforming_strings TO OFF;')
It needs a fix here to get back the config:
finally: if conforming_strings_state == 'on': cur.execute('SET standard_conforming_strings TO ON;')
comment:19 by , 6 years ago
Needs tests: | unset |
---|---|
Summary: | postgres expression comparisons not using proper escaping → PostgreSQL's unaccent lookup doesn't work if standard_conforming_strings is off |
Triage Stage: | Unreviewed → Ready for checkin |
Fixes issue with unaccent when postgres has non-default standard_conforming_strings