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)

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.

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.

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

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.

Ralf any update/thoughts here?