DB.executeUpdate not raise exception make hard to detect error

Description

to figure out problem, i will description real case in
condition: table AD_PrintFormat_Trl has a wrong primary key (just column AD_PrintFormat_ID)
=> statement insert translate to AD_PrintFormat_Trl in PO.insertTranslationsinsert make exception duplicate key when more language insert.

step to run insert data to AD_PrintFormat
start a local transaction
1. in PO.saveNew
+ PO.doInsert process insert table AD_PrintFormat success, data of AD_PrintFormat instance is reload. new key is set to AD_PrintFormat instance
+ PO.saveFinish is called.
2. in PO.saveFinish
+ PO.insertTranslationsinsert is called.
DB.executeUpdate make a exception (but exception is hidden in catch block) => transaction is crash. every save data is miss. but nothing notify this
3. transaction is commit normal

after that, every command rely to new AD_PrintFormat instance is fail, because AD_PrintFormat instance is not exits in database

SOLUTION:
+ must add warning at DB.executeUpdate to dev easy see problem
+ in this case, we don't cake about result of DB.executeUpdate. must run it in separate transation
==========================================
1. patern 2. break transaction
no = DB.executeUpdate(sql.toString(), get_TrxName());
if (log.isLoggable(Level.INFO)) log.info("Delete Old Impored =" + no);

change
no = DB.executeUpdateEx(sql.toString(), get_TrxName());
if (log.isLoggable(Level.INFO)) log.info("Delete Old Impored =" + no);

remark: change current behavior in case exception

2. patern 2. no separate case eception and case update no record:
DocManager.save

change case by case.
more case just change DB.executeUpdate => DB.executeUpdateEx
because outer caller have handle exception

3. patern 3. it's ok just change for consistent:
noins = DB.executeUpdate(sql.toString(), get_TrxName())
if (noins == -1)
throw new AdempiereSystemError("Cannot insert into hst_"+tableName);

change:
add exception message paramenter to executeUpdateEx
noins = DB.executeUpdateEx(sql.toString(), get_TrxName())

4. patern 4. it's ok just change for consistent:
noins = DB.executeUpdate(sql.toString(), get_TrxName())
if (noins == -1)
create message error and rolback transaction

InOutGen.generate

change:
add exception message paramenter to executeUpdateEx
try{
noins = DB.executeUpdateEx(sql.toString(), get_TrxName())
}catch (Exerption){
create message,
rollback
}

5. patern 5. separate transation need combina (carefully, more change, must review case by case)
ValuePreference.insert ().
in this function, call delete and insert on separate transaction.
but i think it need do in a transaction.

6. ADSortTab.saveData

7. patern 7. separate transaction, and don't care result of command.
WPayPrint.cmd_print

not need change?

Environment

None

Activity

Show:
Hiep Lq
July 4, 2014, 1:32 PM

you mean we can change all executeUpdate to executeUpdateEx and we will replace it?
i also like it, at my view it's best way.

because i worry about change "executeUpdate to executeUpdateEx" can't include in trunk,
I suggest solution to can easy see potential break transaction.

i will up a patch for replace all executeUpdate to executeUpdateEx?

Carlos Ruiz
July 4, 2014, 2:55 PM

Some notes about:

  • Some cases could require deeper analysis, some cases could require a refactor to try/catch blocks

  • Also to note behavior is different in oracle than postgres:

  • in oracle a failing SQL statement doesn't stop the next statements to be processed and you can commit

  • in postgreSQL a failing SQL statement forces failure for the next statements within the same transaction and commit is not possible
    raising exception always would make the behavior consistent

  • the same assumption could be made for save vs saveEx - raising an exception on save in case of SQL failure could be desirable
    this case is slightly different as saveEx doesn't have a return value, but executeUpdate/executeUpdateEx have the same return value resulting in duplicate functionality as hengsin pointed

  • for the dup functionality then we could simply keep executeUpdate as a call to executeUpdateEx and make the annotation that both are synonyms

Hiep Lq
July 5, 2014, 2:40 AM

"executeUpdate as a call to executeUpdateEx and make the annotation that both are synonyms"
it's faster way. fix just one where.
but we must review, analysis all where call executeUpdate. because i will prefer to do as below step

1. keep executeUpdate as a call to executeUpdateEx in dev, test environment to other people can help detect any effect when replace.
2. find some pattern use executeUpdate and pattern replace to review
3. review case by case to replace executeUpdate by executeUpdateEx. the end when executeUpdate is total replace.

Hiep Lq
July 9, 2014, 2:28 AM

a bit case of patern 1

Hiep Lq
September 23, 2015, 11:36 AM

other case this issue:
each thread in idempiere share a connection for all un-transaction process.
so, at some place with careless code, call none-ex function (as getSQLValue, executeUpdate) or call ex function (getSQLValueEx, executeUpdateEx) but hidden exception (just catch and log error, don't close connection), that make further database action encounter exception "You can't operate on a closed Statement".
its very hard to investigate this type of issue because reason of issue at other place

patch

add a warning log, provide information to investigate issue.

Assignee

Unassigned

Reporter

Hiep Lq

Labels

Tested By

None

Components

Affects versions

Priority

Major
Configure