[18.0][FIX] edi_core_oca: handle OperationalError and IntegrityError exceptions to prevent finally …

[Ricardoalso]

…block execution in aborted transaction state

In failed transactions, the cursor enters an aborted state where any further SQL (including finally block writes) fails. Previously unhandled, causing cascading errors.

Cherry-picked the FakeModelLoader commit in here in order to let the CI run, but I will rebase it as soon the FakeModelLoader PR gets merged #238

[OCA-git-bot]

Hi @simahawk, @etobella,
some modules you are maintaining are being modified, check this out!

[guewen]

@Ricardoalso InFailedSqlTransaction is what happens when we try to run a new SQL query in an aborted transaction because of a past error, which means we already had an SQL error previously (for instance we can have an InFailedSqlTransaction if we do the write in the finally clause after a serialization error).
Catching this exception will not protect us against for e.g. serialization errors, because the error will pop, not be caught here, so the write in the finally will be done but fail with a InFailedSqlTransaction.

Now, I'm not sure what the authors would like to catch and report in the
exchange_record.write of the finally clause (@simahawk @etobella ?)

At the very least, the errors from from odoo.service.model import PG_CONCURRENCY_ERRORS_TO_RETRY should be caught and reraised so they are retried.

Now, if we do not catch other errors in psycopg2.errors.OperationalError and psycopg2.errors.IntegrityError, the write in the finally clause will fail anyway with a InFailedSqlTransaction. It would be bad because it obscures the original error.

So, one option is we catch and reraise these errors too. The other option is we catch them and use a savepoint around self._exchange_send(exchange_record) to be able to write the error status / do the write in a new transaction.

So maybe

try:
    self._exchange_send(exchange_record)
except ...:
except (OperationalError, IntegrityError):
    # if the error is retryable (one of PG_CONCURRENCY_ERRORS_TO_RETRY), it will be retried by the controller / job, otherwise the error will pop up
    _logger...
    state = "output_sql_error"
    raise

or

try:
    with self.env.cr.savepoint():
        self._exchange_send(exchange_record)
except ...:
except PG_CONCURRENCY_ERRORS_TO_RETRY:
    _logger...
    state = "output_sql_error"
    raise
except (OperationalError, IntegrityError):
    _logger...
   # raise or not, I don't know what is supposed to happen

There are some variants

[Ricardoalso]

Indeed, thanks @guewen

I’m more in favor of the “catch and raise” without the new transaction write approach. It prevents writing to the exchange_record, though the trade‑off is that we lose some information about the current state. However, since I see this as an intermediate state rather than a final one 🤔, it might be better to avoid creating a new transaction just to capture that temporary state.

I am open to suggestions

[simahawk]

Overall LGTM. Thanks!
Any chance to have a test?

[Ricardoalso]

Overall LGTM. Thanks! Any chance to have a test?

Done 🙏

[simahawk]

Thanks! Pls squash :)