Issue 2551241
Created on 2022-11-22 15:35 by schlatterbeck, last changed 2023-02-20 00:07 by rouilj.
Messages | |||
---|---|---|---|
msg7664 | Author: [hidden] (schlatterbeck) | Date: 2022-11-22 15:35 | |
I'm getting several failed tests with python3.7 and the then-current version of psycopg2. This fails most of the indexer tests in test/test_liveserver.py, e.g. test/test_liveserver.py:1305: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ roundup/instance.py:116: in open 'db': backend.Database(self.config, name) roundup/backends/rdbms_common.py:232: in __init__ self.indexer = get_indexer(config, self) roundup/backends/indexer_common.py:158: in get_indexer from roundup.backends.indexer_postgresql_fts import Indexer _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ """ import re from roundup.backends.indexer_common import Indexer as IndexerBase from roundup.i18n import _ from roundup.cgi.exceptions import IndexerQueryError > from psycopg2.errors import InFailedSqlTransaction, SyntaxError, \ UndefinedObject E ModuleNotFoundError: No module named 'psycopg2.errors' roundup/backends/indexer_postgresql_fts.py:13: ModuleNotFoundError Testing this on the command-line it looks like older psycopg2 modules do not have 'errors': On Debian stable bullseye (version 11): % python3 Python 3.9.2 (default, Feb 28 2021, 17:03:44) [GCC 10.2.1 20210110] on linux Type "help", "copyright", "credits" or "license" for more information. >>> from psycopg2 import errors >>> On Debian oldstable buster (version 10): % python3 Python 3.7.3 (default, Jan 22 2021, 20:04:44) [GCC 8.3.0] on linux Type "help", "copyright", "credits" or "license" for more information. >>> from psycopg2 import errors Traceback (most recent call last): File "<stdin>", line 1, in <module> ImportError: cannot import name 'errors' from 'psycopg2' (/usr/lib/python3/dist-packages/psycopg2/__init__.py) |
|||
msg7669 | Author: [hidden] (rouilj) | Date: 2022-11-23 23:08 | |
Looks like errors is new in version 2.8 of psycopg2. https://www.psycopg.org/docs/errors.html Can you try: from psycopg2 import InFailedSqlTransaction, SyntaxError, \ UndefinedObject and see if that works. If so wrap the `from psycopg.errors import` in a try/except and put the `from psycopg2 import` in the except clause. |
|||
msg7670 | Author: [hidden] (rouilj) | Date: 2022-11-24 01:57 | |
I can reproduce this on issue.roundup-tracker.org that has an older psycopg2. from psycopg2 import InFailedSqlTransaction, SyntaxError, \ UndefinedObject doesn't work. https://www.psycopg.org/docs/errors.html maps the 2.8+ exceptions to their base exceptions. So we can use the base exceptions instead. from psycopg2 import InternalError as InFailedSqlTransaction, \ ProgrammingError as SyntaxError UndefinedObject = SyntaxError >>> from psycopg2 import InternalError as InFailedSqlTransaction, \ ... ProgrammingError as SyntaxError >>> UndefinedObject = SyntaxError >>> UndefinedObject <class 'psycopg2.ProgrammingError'> >>> SyntaxError <class 'psycopg2.ProgrammingError'> >>> InFailedSqlTransaction <class 'psycopg2.InternalError'> I think that will do. It's not as targeted as the original errors, but should do the trick with the older psycopg2. Ralf, can you try patching the except clause with the above import and assignment? Then run the test suite to see if it fixes the issue. Surprisingly, the SyntaxError and UndefinedObject error seem to be raised by the test suite according to: https://app.codecov.io/gh/roundup- tracker/roundup/blob/master/roundup/backends/indexer_postgresql_fts.py So if this isn't the right thing to do, hopefully some test will fail. InFailedSqlTransaction isn't raised by tests so that might still be an issue. But if the other two pass it gives me hope that this is correct too. |
|||
msg7672 | Author: [hidden] (schlatterbeck) | Date: 2022-11-24 10:44 | |
On Thu, Nov 24, 2022 at 01:57:09AM +0000, John Rouillard wrote: > > https://www.psycopg.org/docs/errors.html maps the 2.8+ exceptions > to their base exceptions. So we can use the base exceptions instead. > > from psycopg2 import InternalError as InFailedSqlTransaction, \ > ProgrammingError as SyntaxError > UndefinedObject = SyntaxError > > >>> from psycopg2 import InternalError as InFailedSqlTransaction, \ > ... ProgrammingError as SyntaxError > >>> UndefinedObject = SyntaxError > >>> UndefinedObject > <class 'psycopg2.ProgrammingError'> > >>> SyntaxError > <class 'psycopg2.ProgrammingError'> > >>> InFailedSqlTransaction > <class 'psycopg2.InternalError'> > > I think that will do. It's not as targeted as the original errors, but > should do the trick with the older psycopg2. Looks like this works sort of. I still get some failing tests in test/test_indexer.py roundup/backends/indexer_postgresql_fts.py we have - In find, line 123 it catches 'SyntaxError' and re-raises IndexerQueryError but the test in postgresqlFtsIndexerTest.test_invalid_language expects ValueError - In add_text, the try/except line 73 only catches ValueError and several tests test for this. So add_text should probably also catch SyntaxError and Re-raise IndexerQueryError (and it should probably rollback?) and we might want to make IndexerQueryError inherit from ValueError? Or fix the tests so that they look for either ValueError or IndexerQueryError? I currently do not see how postgresqlFtsIndexerTest.test_invalid_language will ever get a ValueError (line 570) because this is caught in find and re-raised as IndexerQueryError. Ralf -- Dr. Ralf Schlatterbeck Tel: +43/2243/26465-16 Open Source Consulting www: www.runtux.com Reichergasse 131, A-3411 Weidling email: office@runtux.com |
|||
msg7676 | Author: [hidden] (rouilj) | Date: 2022-11-24 19:18 | |
Hi Ralf: tl;dr 1) require version of psycopg2 that has psycopg2.errors 2) merge UndefinedObject and SyntaxError handling In message <20221124104405.xmkzbte4pjuwazda@runtux.com>, Ralf Schlatterbeck writes: >On Thu, Nov 24, 2022 at 01:57:09AM +0000, John Rouillard wrote: >> >> https://www.psycopg.org/docs/errors.html maps the 2.8+ exceptions >> to their base exceptions. So we can use the base exceptions instead. >> >> from psycopg2 import InternalError as InFailedSqlTransaction, \ >> ProgrammingError as SyntaxError >> UndefinedObject = SyntaxError >> >> >>> from psycopg2 import InternalError as InFailedSqlTransaction, \ >> ... ProgrammingError as SyntaxError >> >>> UndefinedObject = SyntaxError >> >>> UndefinedObject >> <class 'psycopg2.ProgrammingError'> >> >>> SyntaxError >> <class 'psycopg2.ProgrammingError'> >> >>> InFailedSqlTransaction >> <class 'psycopg2.InternalError'> >> >> I think that will do. It's not as targeted as the original errors, but >> should do the trick with the older psycopg2. > >Looks like this works sort of. > >I still get some failing tests in test/test_indexer.py >roundup/backends/indexer_postgresql_fts.py we have >- In find, line 123 it catches 'SyntaxError' and re-raises > IndexerQueryError but the test in > postgresqlFtsIndexerTest.test_invalid_language expects ValueError [...] >I currently do not see how >postgresqlFtsIndexerTest.test_invalid_language will ever get a >ValueError (line 570) because this is caught in find and re-raised as >IndexerQueryError. See the handler for UndefinedObject in indexer_postgres_fts.py line 145: raise ValueError(_("Check tracker config.ini for a bad " "indexer_language setting. Error is: %s") % e) I raise ValueError for a specific error message for the UndefinedObject Exception. I'm sure it won't work if postgresql is in a different locale (e.g. de) but.... So the library returns SyntaxError aka UndefinedObject. But the SyntaxError is captured earlier in the chain. (I'm confused by how this works wit a newer psycopg2. If class UndefinedObject(SyntaxError) I should never reach UndefinedClass hander, but it works with a new enough psycopg2.) What do you think about using this clause and removing the existing SyntaxError handler: except (SyntaxError, UndefinedObject) as e: # try for a nicer user error self.db.rollback() lookfor = ('text search configuration "%s" does ' 'not exist' % self.db.config['INDEXER_LANGUAGE']) if lookfor in e.args[0]: raise ValueError(_("Check tracker config.ini for a bad " "indexer_language setting. Error is: %s") % e) else: raise IndexerQueryError(e.args[0]) I preferred a simple raise for UndefinedObject so that we didn't loose info when the top level exception hander does the stack traceback etc. Reraising it as an IndexerQueryError might loose info needed to debug it but.... Another way to handle this is to do a test for psycopg2 version and require a specific version (> 2.8) where the .errors modue is available to run the fts indexer. Since this is a new feature this might be the best way to handle it. Then just doc it in installation.txt. Thoughts? >So add_text should probably also catch SyntaxError and Re-raise >IndexerQueryError (and it should probably rollback?) The add_text part of that test looks for psycopg2.errors.UndefinedObject which is SyntaxError. So it should be working as SyntaxError bubbles up. >- In add_text, the try/except line 73 only catches ValueError and > several tests test for this. [...] > So add_text should probably also catch SyntaxError and Re-raise > IndexerQueryError (and it should probably rollback?) SyntaxError (and UndefinedObject) for that matter could be handled as above for find() and raise ValueError. > and we might want > to make IndexerQueryError inherit from ValueError? Or fix the tests so > that they look for either ValueError or IndexerQueryError? If we choose the find() exception handling method then: with self.assertRaises(psycopg2.errors.UndefinedObject) as ctx: in the test should be: with self.assertRaises(ValueError) as ctx: the same as find. Quips, comments, evasions, questions, or answers welcome. |
|||
msg7719 | Author: [hidden] (rouilj) | Date: 2023-02-20 00:07 | |
Ralf any update/thoughts here? |
History | |||
---|---|---|---|
Date | User | Action | Args |
2023-02-20 00:07:42 | rouilj | set | messages: + msg7719 |
2022-11-24 19:18:55 | rouilj | set | messages: + msg7676 |
2022-11-24 10:44:08 | schlatterbeck | set | messages: + msg7672 |
2022-11-24 01:57:09 | rouilj | set | status: new -> open severity: normal -> major messages: + msg7670 priority: high assignee: schlatterbeck type: compile error |
2022-11-23 23:08:43 | rouilj | set | nosy:
+ rouilj messages: + msg7669 |
2022-11-22 15:36:05 | schlatterbeck | set | components:
+ Database versions: + devel |
2022-11-22 15:35:24 | schlatterbeck | create |