DB.executeUpdate not raise exception make hard to detect error
Description
Environment
Attachments
- 23 Sep 2015, 11:25 AM
- 09 Jul 2014, 02:28 AM
- 04 Jul 2014, 02:54 AM
relates to
Activity
Carlos Ruiz July 23, 2021 at 4:18 PM
Closing as potential idea after 3 years of inactivity.
Hiep Lq September 23, 2015 at 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.
Hiep Lq July 9, 2014 at 2:28 AM
a bit case of patern 1
Hiep Lq July 5, 2014 at 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.
Carlos Ruiz July 4, 2014 at 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
Details
Details
Assignee
Reporter
Labels
Components
Affects versions
Priority
Major
to figure out problem, i will description real case in https://idempiere.atlassian.net/browse/IDEMPIERE-1878#icft=IDEMPIERE-1878
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?