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.