Openbravo Issue Tracking System - Openbravo ERP
View Issue Details
IDProjectCategoryView StatusDate SubmittedLast Update
0033771Openbravo ERP07. Sales managementpublic2016-08-23 17:132016-09-01 12:12
Reportershuehner 
Assigned Toshuehner 
PrioritynormalSeverityminorReproducibilityhave not tried
StatusclosedResolutionfixed 
PlatformOS5OS Version
Product Version 
Target VersionFixed in Version3.0PR16Q4 
Merge Request Status
Review Assigned Toaferraz
OBNetwork customer
Web browser
ModulesCore
Support ticket
Regression level
Regression date
Regression introduced in release
Regression introduced by commit
Triggers an Emergency PackNo
Summary0033771: Accidental double query in SE_Invoice_BPartner.isAutomaticCombination
DescriptionCode in that method triggers double sql query by calling .list() twice on same OBCriteria instance:

          if (obc.list() == null || obc.list().size() == 0) {

Note: Afaik that '== null' should not be possible in practice, so that should be rechecked and that 2nd that part probably removed.

Also: the filter combination seems to be unique as per this db constraint:

    "fin_finacc_paymentmethod_un" UNIQUE CONSTRAINT, btree (fin_paymentmethod_id, fin_financial_account_id)

So a single line note in java would be nice to have clear that .list() will never load huge results into mem.
Steps To Reproducewith debugging code from: https://issues.openbravo.com/view.php?id=33767 [^]

Open Sales invoice window
new in form view
select a bpartner from dropdown list of selector
-> see double .list() call happen
Proposed Solution
Additional Information
TagsPerformance
Relationships
related to feature request 0033767 closed platform Add code to auto-detect 'accidental double query' on same OBQuery or OBCriteria object 
related to design defect 0036898 new Triage Finance Performance issues when using DAL 
Attached Files
Issue History
Date ModifiedUsernameFieldChange
2016-08-23 17:13shuehnerNew Issue
2016-08-23 17:13shuehnerAssigned To => Triage Finance
2016-08-23 17:13shuehnerModules => Core
2016-08-23 17:13shuehnerTriggers an Emergency Pack => No
2016-08-23 17:14shuehnerTag Attached: Performance
2016-08-23 17:14shuehnerRelationship addedrelated to 0033767
2016-08-24 14:39hgbotCheckin
2016-08-24 14:39hgbotNote Added: 0089346
2016-08-24 14:39hgbotStatusnew => resolved
2016-08-24 14:39hgbotResolutionopen => fixed
2016-08-24 14:39hgbotFixed in SCM revision => http://code.openbravo.com/erp/devel/pi/rev/e23c586b8dbf38c79bca558dfdb10a4bbc0f5f1e [^]
2016-08-24 14:40shuehnerAssigned ToTriage Finance => shuehner
2016-08-24 14:40shuehnerReview Assigned To => aferraz
2016-08-31 11:12hgbotCheckin
2016-08-31 11:12hgbotNote Added: 0089590
2016-08-31 11:26hgbotCheckin
2016-08-31 11:26hgbotNote Added: 0089592
2016-08-31 11:26aferrazNote Added: 0089593
2016-08-31 11:26aferrazStatusresolved => closed
2016-08-31 11:26aferrazFixed in Version => 3.0PR16Q4
2016-09-01 12:12hudsonbotCheckin
2016-09-01 12:12hudsonbotNote Added: 0089648
2016-09-01 12:12hudsonbotCheckin
2016-09-01 12:12hudsonbotNote Added: 0089649
2017-09-19 18:47markmm82Relationship addedrelated to 0036898

Notes
(0089346)
hgbot   
2016-08-24 14:39   
Repository: erp/devel/pi
Changeset: e23c586b8dbf38c79bca558dfdb10a4bbc0f5f1e
Author: Stefan Hühner <stefan.huehner <at> openbravo.com>
Date: Wed Aug 24 14:39:03 2016 +0200
URL: http://code.openbravo.com/erp/devel/pi/rev/e23c586b8dbf38c79bca558dfdb10a4bbc0f5f1e [^]

Fixed 33771. Avoid double .list() call to not run query twice.

Old code ran following a OBCriteria
  if (obc.list() == null || obc.list().size() == 0) {

However .list() does not return null as per Hibernate javadoc but empty or
non-empty list (of method raising an exception).

So remove the extra null check to avoid double .list() call.

Also improve .list().size() check slightly by using .list().isEmpty()

Add note that filter is on unique constraint so .list().size()<=1 also

---
M src/org/openbravo/erpCommon/ad_callouts/SE_Invoice_BPartner.java
---
(0089590)
hgbot   
2016-08-31 11:12   
Repository: erp/devel/pi
Changeset: e909d7e193e55e2b4b9163d4120cf04113bce9b1
Author: Alvaro Ferraz <alvaro.ferraz <at> openbravo.com>
Date: Wed Aug 31 11:11:13 2016 +0200
URL: http://code.openbravo.com/erp/devel/pi/rev/e909d7e193e55e2b4b9163d4120cf04113bce9b1 [^]

Related to issue 33771: Code review improvements

Use uniqueResult() == null instead of list().isEmpty(), adding setMaxResults(1) to avoid exceptions.
Update copyright.

---
M src/org/openbravo/erpCommon/ad_callouts/SE_Invoice_BPartner.java
---
(0089592)
hgbot   
2016-08-31 11:26   
Repository: erp/devel/pi
Changeset: 43453b625a063992658b0b391e42c9b263fccbc1
Author: Alvaro Ferraz <alvaro.ferraz <at> openbravo.com>
Date: Wed Aug 31 11:25:47 2016 +0200
URL: http://code.openbravo.com/erp/devel/pi/rev/43453b625a063992658b0b391e42c9b263fccbc1 [^]

Related to issue 33771: Remove setMaxResults(1)

setMaxResults(1) is not needed as we are filtering by unique constraint.

---
M src/org/openbravo/erpCommon/ad_callouts/SE_Invoice_BPartner.java
---
(0089593)
aferraz   
2016-08-31 11:26   
Code review OK
(0089648)
hudsonbot   
2016-09-01 12:12   
A changeset related to this issue has been promoted main and to the
Central Repository, after passing a series of tests.

Promotion changeset: https://code.openbravo.com/erp/devel/main/rev/a321ec570edd [^]
Maturity status: Test
(0089649)
hudsonbot   
2016-09-01 12:12   
A changeset related to this issue has been promoted main and to the
Central Repository, after passing a series of tests.

Promotion changeset: https://code.openbravo.com/erp/devel/main/rev/a321ec570edd [^]
Maturity status: Test