Opened 5 years ago

Closed 5 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)

django_patch​ (709 bytes ) - added by Tom McClure 5 years ago.
Fixes issue with unaccent when postgres has non-default standard_conforming_strings
django_patch2​ (2.1 KB ) - added by Tom McClure 5 years ago.
updated patch, now includes test
django_patch3​ (2.1 KB ) - added by Tom McClure 5 years ago.
cleaned up test per feedback

Download all attachments as: .zip

Change History (23)

by Tom McClure, 5 years ago

Attachment: django_patch​ added

Fixes issue with unaccent when postgres has non-default standard_conforming_strings

comment:1 by Herbert Fortes, 5 years ago

Cc: Herbert Fortes added

comment:2 by Jayantha Gumballi, 5 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 Tom McClure, 5 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 Jayantha Gumballi, 5 years ago

Owner: changed from nobody to Jayantha Gumballi
Status: new → assigned

comment:5 by Jayantha Gumballi, 5 years ago

Hello Tom,

I have made the PR, but one of the checks is failing, is this ok?

comment:6 by Herbert Fortes, 5 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

​mariadb fail

comment:7 by Jayantha Gumballi, 5 years ago

Should we just merge the PR then?

Do I have to mark this as fixed?

comment:8 by Tim Graham, 5 years ago

Needs tests: set

All fixes require a regression test.

comment:9 by Tom McClure, 5 years ago

a test would probably have to do the following --

issue some setup/teardown SQL on the low-level postgres connection

  1. set up: execute "set standard_conforming_strings to off;"
  2. run the postgres/test_unaccent tests (especially the test_unaccent_chained test)
  3. tear down: execute "set standard_conforming_strings to on;" -- set it back to the default

by Tom McClure, 5 years ago

Attachment: django_patch2​ added

updated patch, now includes test

comment:10 by Jayantha Gumballi, 5 years ago

Version 1, edited 5 years ago by Jayantha Gumballi (previous) (next) (diff)

comment:11 by Herbert Fortes, 5 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.

in reply to:  10 comment:12 by Tom McClure, 5 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'…

by Tom McClure, 5 years ago

Attachment: django_patch3​ added

cleaned up test per feedback

in reply to:  11 comment:13 by Tom McClure, 5 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 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.

yes thank you, moved import and using "with" now. and the with will automatically close the cursor.

comment:14 by Jayantha Gumballi, 5 years ago

I have updated the PR again.

comment:15 by Jayantha Gumballi, 5 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?

Last edited 5 years ago by Jayantha Gumballi (previous) (diff)

comment:16 by Jayantha Gumballi, 5 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.

Last edited 5 years ago by Jayantha Gumballi (previous) (diff)

comment:17 by Simon Charette, 5 years ago

Jayantha, you definitely want to keep the cleanup around else the per-connection setting override could leak between tests.

comment:18 by Herbert Fortes, 5 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 Tim Graham, 5 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

comment:20 by Tim Graham <timograham@…>, 5 years ago

Resolution: → fixed
Status: assigned → closed

In dfcdc899:

Fixed #29886 -- Fixed unaccent lookup when PostgreSQL's standard_conforming_strings option is off.

Thanks Tom McClure for the patch.

Note: See TracTickets for help on using tickets.
Back to Top