Opened 3 years ago

Closed 3 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 3 years ago.
Fixes issue with unaccent when postgres has non-default standard_conforming_strings
django_patch2 (2.1 KB) - added by Tom McClure 3 years ago.
updated patch, now includes test
django_patch3 (2.1 KB) - added by Tom McClure 3 years ago.
cleaned up test per feedback

Download all attachments as: .zip

Change History (23)

Changed 3 years ago by Tom McClure

Attachment: django_patch added

Fixes issue with unaccent when postgres has non-default standard_conforming_strings

comment:1 Changed 3 years ago by Herbert Fortes

Cc: Herbert Fortes added

comment:2 Changed 3 years ago by Jayantha Gumballi

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 Changed 3 years ago by Tom McClure

I think there's already test coverage for this code, so, yeah, just use the patch to make a PR. Thanks!!

comment:4 Changed 3 years ago by Jayantha Gumballi

Owner: changed from nobody to Jayantha Gumballi
Status: newassigned

comment:5 Changed 3 years ago by Jayantha Gumballi

Hello Tom,

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

comment:6 Changed 3 years ago by Herbert Fortes

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 Changed 3 years ago by Jayantha Gumballi

Should we just merge the PR then?

Do I have to mark this as fixed?

comment:8 Changed 3 years ago by Tim Graham

Needs tests: set

All fixes require a regression test.

comment:9 Changed 3 years ago by Tom McClure

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

Changed 3 years ago by Tom McClure

Attachment: django_patch2 added

updated patch, now includes test

comment:10 Changed 3 years ago by 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?

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

comment:11 Changed 3 years ago by Herbert Fortes

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 in reply to:  10 Changed 3 years ago by Tom McClure

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'…

Changed 3 years ago by Tom McClure

Attachment: django_patch3 added

cleaned up test per feedback

comment:13 in reply to:  11 Changed 3 years ago by Tom McClure

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 Changed 3 years ago by Jayantha Gumballi

I have updated the PR again.

comment:15 Changed 3 years ago by Jayantha Gumballi

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 3 years ago by Jayantha Gumballi (previous) (diff)

comment:16 Changed 3 years ago by Jayantha Gumballi

@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 3 years ago by Jayantha Gumballi (previous) (diff)

comment:17 Changed 3 years ago by Simon Charette

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

comment:18 Changed 3 years ago by Herbert Fortes

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 Changed 3 years ago by Tim Graham

Needs tests: unset
Summary: postgres expression comparisons not using proper escapingPostgreSQL's unaccent lookup doesn't work if standard_conforming_strings is off
Triage Stage: UnreviewedReady for checkin

comment:20 Changed 3 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

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