Hi Ralf: tl;dr 1) require version of psycopg2 that has psycopg2.errors 2) merge UndefinedObject and SyntaxError handling In message <firstname.lastname@example.org>, 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: raise ValueError(_("Check tracker config.ini for a bad " "indexer_language setting. Error is: %s") % e) else: raise IndexerQueryError(e.args) 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.
|2022-11-24 19:18:55||rouilj||set||recipients: + schlatterbeck|
|2022-11-24 19:18:55||rouilj||link||issue2551241 messages|