IDEMPIERE-385 Resolve M_Storage locking and data consistency / lock storage record being updated

fitnesse/FitNesseRoot/RecentChanges/content.txt

@@ -1,4 +1,4 @@
-|||14:59:04 Tue, Nov 27, 2012|
+|||01:35:08 mié, nov 28, 2012|
 |CommonTests.CreateMaterialReceipt||01:36:56 mar, abr 03, 2012|
 |CommonTests.CreateProductPrice||01:29:57 mar, abr 03, 2012|
 |AvgCostSuite.BasicTest||01:18:34 mar, abr 03, 2012|

fitnesse/FitNesseRoot/content.txt

@@ -2,7 +2,7 @@ Define the global path:
 
 Where to find the fixtures classes: ( i.e. /home/hengsin/workspace/idempiere/fitnesse/bin )
 
-!define fitnesse_home {/home/hengsin/workspace/idempiere/fitnesse}
+!define fitnesse_home {/home/carlos/hgAdempiere/localosgi/fitnesse}
 
 !path ${fitnesse_home}/fitnesse.jar:${fitnesse_home}/lib/*.jar:${fitnesse_home}/bin
 
@@ -16,7 +16,7 @@ There are some important variables here:
 This variables can be redefined specifically at page level.
 
 !define TEST_RUNNER {fitnesse.client.FitServerServletInvoker}
-!define COMMAND_PATTERN {java -Xms32m -Xmx512m -DLOG4J_LEVEL=CONFIG -cp %p %m http://localhost:8080/fitnesse/FitServlet}
+!define COMMAND_PATTERN {java -Xms32m -Xmx512m -DLOG4J_LEVEL=CONFIG -cp %p %m http://localhost:8082/fitnesse/FitServlet}
 
 To enable remote debugging the tests will stop until you connect remotely via eclipse using RemoteADempiereFitnesse.launch
 

org.adempiere.base.process/src/org/compiere/process/M_Production_Run.java

@@ -158,7 +158,7 @@ public class M_Production_Run extends SvrProcess {
 							if (!MStorageOnHand.add(getCtx(), locator.getM_Warehouse_ID(),
 								locator.getM_Locator_ID(),
 								pline.getM_Product_ID(), 
-								pline.getM_AttributeSetInstance_ID(), 0 , 
+								pline.getM_AttributeSetInstance_ID(),
 								MovementQty,
 								get_TrxName()))
 							{

org.adempiere.base/src/org/compiere/model/MInOut.java

@@ -1356,7 +1356,7 @@ public class MInOut extends X_M_InOut implements DocAction
 						if (!MStorageOnHand.add(getCtx(), getM_Warehouse_ID(),
 							sLine.getM_Locator_ID(),
 							sLine.getM_Product_ID(),
-							ma.getM_AttributeSetInstance_ID(), reservationAttributeSetInstance_ID,
+							ma.getM_AttributeSetInstance_ID(),
 							QtyMA,
 							get_TrxName()))
 						{
@@ -1438,7 +1438,7 @@ public class MInOut extends X_M_InOut implements DocAction
 					if (!MStorageOnHand.add(getCtx(), getM_Warehouse_ID(),
 						sLine.getM_Locator_ID(),
 						sLine.getM_Product_ID(),
-						sLine.getM_AttributeSetInstance_ID(), reservationAttributeSetInstance_ID,
+						sLine.getM_AttributeSetInstance_ID(),
 						Qty, get_TrxName()))
 					{
 						String lastError = CLogger.retrieveErrorString("");

org.adempiere.base/src/org/compiere/model/MInventory.java

@@ -440,7 +440,7 @@ public class MInventory extends X_M_Inventory implements DocAction
 						if (!MStorageOnHand.add(getCtx(), getM_Warehouse_ID(),
 								line.getM_Locator_ID(),
 								line.getM_Product_ID(), 
-								ma.getM_AttributeSetInstance_ID(), 0, 
+								ma.getM_AttributeSetInstance_ID(), 
 								QtyMA.negate(), get_TrxName()))
 						{
 							String lastError = CLogger.retrieveErrorString("");
@@ -491,7 +491,7 @@ public class MInventory extends X_M_Inventory implements DocAction
 					if (!MStorageOnHand.add(getCtx(), getM_Warehouse_ID(),
 							line.getM_Locator_ID(),
 							line.getM_Product_ID(), 
-							line.getM_AttributeSetInstance_ID(), 0, 
+							line.getM_AttributeSetInstance_ID(), 
 							qtyDiff,get_TrxName()))
 					{
 						String lastError = CLogger.retrieveErrorString("");

org.adempiere.base/src/org/compiere/model/MMovement.java

@@ -403,7 +403,7 @@ public class MMovement extends X_M_Movement implements DocAction
 						if (!MStorageOnHand.add(getCtx(),locator.getM_Warehouse_ID(),
 								line.getM_Locator_ID(),
 								line.getM_Product_ID(), 
-								ma.getM_AttributeSetInstance_ID(), 0, 
+								ma.getM_AttributeSetInstance_ID(),
 								ma.getMovementQty().negate(), get_TrxName()))
 						{
 							String lastError = CLogger.retrieveErrorString("");
@@ -421,7 +421,7 @@ public class MMovement extends X_M_Movement implements DocAction
 						if (!MStorageOnHand.add(getCtx(),locator.getM_Warehouse_ID(),
 								line.getM_LocatorTo_ID(),
 								line.getM_Product_ID(), 
-								M_AttributeSetInstanceTo_ID, 0, 
+								M_AttributeSetInstanceTo_ID,
 								ma.getMovementQty(), get_TrxName()))
 						{
 							String lastError = CLogger.retrieveErrorString("");
@@ -461,7 +461,7 @@ public class MMovement extends X_M_Movement implements DocAction
 					if (!MStorageOnHand.add(getCtx(),locator.getM_Warehouse_ID(),
 							line.getM_Locator_ID(),
 							line.getM_Product_ID(), 
-							line.getM_AttributeSetInstance_ID(), 0, 
+							line.getM_AttributeSetInstance_ID(),
 							line.getMovementQty().negate(), get_TrxName()))
 					{
 						String lastError = CLogger.retrieveErrorString("");
@@ -473,7 +473,7 @@ public class MMovement extends X_M_Movement implements DocAction
 					if (!MStorageOnHand.add(getCtx(),locator.getM_Warehouse_ID(),
 							line.getM_LocatorTo_ID(),
 							line.getM_Product_ID(), 
-							line.getM_AttributeSetInstanceTo_ID(), 0, 
+							line.getM_AttributeSetInstanceTo_ID(),
 							line.getMovementQty(), get_TrxName()))
 					{
 						String lastError = CLogger.retrieveErrorString("");

org.adempiere.base/src/org/compiere/model/MProjectIssue.java

@@ -164,7 +164,7 @@ public class MProjectIssue extends X_C_ProjectIssue
 		//
 		MLocator loc = MLocator.get(getCtx(), getM_Locator_ID());
 		if (MStorageOnHand.add(getCtx(), loc.getM_Warehouse_ID(), getM_Locator_ID(), 
-				getM_Product_ID(), getM_AttributeSetInstance_ID(), getM_AttributeSetInstance_ID(),
+				getM_Product_ID(), getM_AttributeSetInstance_ID(),
 				getMovementQty().negate(), get_TrxName()))
 		{
 			if (mTrx.save(get_TrxName()))

org.adempiere.base/src/org/compiere/model/MStorageOnHand.java

@@ -358,16 +358,15 @@ public class MStorageOnHand extends X_M_StorageOnHand
 	 *	@return true if updated
 	 */
 	public static boolean add (Properties ctx, int M_Warehouse_ID, int M_Locator_ID, 
-		int M_Product_ID, int M_AttributeSetInstance_ID, int reservationAttributeSetInstance_ID,
+		int M_Product_ID, int M_AttributeSetInstance_ID,
 		BigDecimal diffQtyOnHand, String trxName)
 	{
-		MStorageOnHand storage = null;
+		if (diffQtyOnHand == null || diffQtyOnHand.signum() == 0)
-		StringBuffer diffText = new StringBuffer("(");
+			return true;
 
 		//	Get Storage
-		if (storage == null)
+		MStorageOnHand storage = getCreate (ctx, M_Locator_ID, M_Product_ID, M_AttributeSetInstance_ID, trxName);
-			storage = getCreate (ctx, M_Locator_ID, 
+		DB.getDatabase().forUpdate(storage, 120);
-				M_Product_ID, M_AttributeSetInstance_ID, trxName);
 		//	Verify
 		if (storage.getM_Locator_ID() != M_Locator_ID 
 			&& storage.getM_Product_ID() != M_Product_ID
@@ -378,64 +377,12 @@ public class MStorageOnHand extends X_M_StorageOnHand
 			return false;
 		}
 
-		// CarlosRuiz - globalqss - Fix [ 1725383 ] QtyOrdered wrongly updated
-		MProduct prd = new MProduct(ctx, M_Product_ID, trxName);
-		if (prd.getM_AttributeSet_ID() == 0) {
-			// Product doesn't manage attribute set, always reserved with 0
-			reservationAttributeSetInstance_ID = 0;
-		}
-		//		
-		
-		MStorageOnHand storage0 = null;
-		if (M_AttributeSetInstance_ID != reservationAttributeSetInstance_ID)
-		{
-			storage0 = get(ctx, M_Locator_ID, 
-				M_Product_ID, reservationAttributeSetInstance_ID, trxName);
-			if (storage0 == null)	//	create if not existing - should not happen
-			{
-				MWarehouse wh = MWarehouse.get(ctx, M_Warehouse_ID);
-				int xM_Locator_ID = wh.getDefaultLocator().getM_Locator_ID();
-				storage0 = getCreate (ctx, xM_Locator_ID, 
-					M_Product_ID, reservationAttributeSetInstance_ID, trxName);
-			}
-		}		
-		boolean changed = false;
-		if (diffQtyOnHand != null && diffQtyOnHand.signum() != 0)
-		{
 		storage.setQtyOnHand (storage.getQtyOnHand().add (diffQtyOnHand));
-			diffText.append("OnHand=").append(diffQtyOnHand);
+		if (s_log.isLoggable(Level.FINE)) {
-			changed = true;
+			StringBuilder diffText = new StringBuilder("(OnHand=").append(diffQtyOnHand).append(") -> ").append(storage.toString());
-		}
-		/*//@win commented out
-		if (diffQtyReserved != null && diffQtyReserved.signum() != 0)
-		{
-			if (storage0 == null)
-				storage.setQtyReserved (storage.getQtyReserved().add (diffQtyReserved));
-			else
-				storage0.setQtyReserved (storage0.getQtyReserved().add (diffQtyReserved));
-			diffText.append(" Reserved=").append(diffQtyReserved);
-			changed = true;
-		}
-		if (diffQtyOrdered != null && diffQtyOrdered.signum() != 0)
-		{
-			if (storage0 == null)
-				storage.setQtyOrdered (storage.getQtyOrdered().add (diffQtyOrdered));
-			else
-				storage0.setQtyOrdered (storage0.getQtyOrdered().add (diffQtyOrdered));
-			diffText.append(" Ordered=").append(diffQtyOrdered);
-			changed = true;
-		}
-		*/
-		if (changed)
-		{
-			diffText.append(") -> ").append(storage.toString());
 			s_log.fine(diffText.toString());
-			if (storage0 != null)
-				storage0.saveEx(trxName);		//	No AttributeSetInstance (reserved/ordered)
-			return storage.save (trxName);
 		}
-		
+		return storage.save (trxName);
-		return true;
 	}	//	add
 
 	

org.adempiere.base/src/org/compiere/model/MStorageReservation.java

@@ -225,6 +225,9 @@ public class MStorageReservation extends X_M_StorageReservation {
 		int M_Product_ID, int M_AttributeSetInstance_ID, int reservationAttributeSetInstance_ID,
 		BigDecimal diffQty, boolean isSOTrx, String trxName)
 	{
+		if (diffQty == null || diffQty.signum() == 0)
+			return true;
+
 		/* Do NOT use FIFO ASI for reservation */
 		MProduct prd = new MProduct(ctx, M_Product_ID, trxName);
 		if (prd.getM_AttributeSet_ID() == 0 || ! prd.getM_AttributeSet().isInstanceAttribute()) {
@@ -233,14 +236,14 @@ public class MStorageReservation extends X_M_StorageReservation {
 			M_AttributeSetInstance_ID = 0;
 		}
 		//
-
+		if (M_AttributeSetInstance_ID != reservationAttributeSetInstance_ID) {
-		MStorageReservation storage = null;
+			M_AttributeSetInstance_ID = reservationAttributeSetInstance_ID;
-		StringBuffer diffText = new StringBuffer("(");
+		}
 
 		//	Get Storage
-		if (storage == null)
+		MStorageReservation storage = getCreate (ctx, M_Warehouse_ID,
-			storage = getCreate (ctx, M_Warehouse_ID, 
 				M_Product_ID, M_AttributeSetInstance_ID, isSOTrx, trxName);
+		DB.getDatabase().forUpdate(storage, 120);
 		//	Verify
 		if (storage.getM_Warehouse_ID() != M_Warehouse_ID 
 			&& storage.getM_Product_ID() != M_Product_ID
@@ -251,37 +254,12 @@ public class MStorageReservation extends X_M_StorageReservation {
 			return false;
 		}
 
-		MStorageReservation storage0 = null;
-		if (M_AttributeSetInstance_ID != reservationAttributeSetInstance_ID)
-		{
-			storage0 = get(ctx, M_Warehouse_ID, 
-				M_Product_ID, reservationAttributeSetInstance_ID, isSOTrx, trxName);
-			if (storage0 == null)	//	create if not existing - should not happen
-			{
-				storage0 = getCreate (ctx, M_Warehouse_ID, 
-					M_Product_ID, reservationAttributeSetInstance_ID, isSOTrx, trxName);
-			}
-		}
-		boolean changed = false;
-		if (diffQty != null && diffQty.signum() != 0)
-		{
-			if (storage0 == null)
 		storage.setQty (storage.getQty().add(diffQty));
-			else
+		if (s_log.isLoggable(Level.FINE)) {
-				storage0.setQty (storage0.getQty().add (diffQty));
+			StringBuilder diffText = new StringBuilder("(Qty=").append(diffQty).append(") -> ").append(storage.toString());
-			diffText.append(" Qty=").append(diffQty);
-			changed = true;
-		}
-		if (changed)
-		{
-			diffText.append(") -> ").append(storage.toString());
 			s_log.fine(diffText.toString());
-			if (storage0 != null)
-				storage0.saveEx(trxName);		//	No AttributeSetInstance
-			return storage.save (trxName);
 		}
-		
+		return storage.save (trxName);
-		return true;
 	}	//	add
 
 	/**

org.adempiere.base/src/org/eevolution/model/MDDOrder.java

@@ -885,7 +885,7 @@ public class MDDOrder extends X_DD_Order implements DocAction
 					//	Update Storage
 					if (!MStorageOnHand.add(getCtx(), locator_to.getM_Warehouse_ID(), locator_to.getM_Locator_ID(), 
 						line.getM_Product_ID(), 
-						line.getM_AttributeSetInstance_ID(), line.getM_AttributeSetInstance_ID(),
+						line.getM_AttributeSetInstance_ID(),
 						Env.ZERO, get_TrxName()))
 					{
 						throw new AdempiereException();
@@ -893,7 +893,7 @@ public class MDDOrder extends X_DD_Order implements DocAction
 					
 					if (!MStorageOnHand.add(getCtx(), locator_from.getM_Warehouse_ID(), locator_from.getM_Locator_ID(), 
 						line.getM_Product_ID(), 
-						line.getM_AttributeSetInstanceTo_ID(), line.getM_AttributeSetInstance_ID(),
+						line.getM_AttributeSetInstanceTo_ID(),
 						Env.ZERO, get_TrxName()))
 					{
 						throw new AdempiereException();

org.compiere.db.oracle.provider/src/org/compiere/db/DB_Oracle.java

@@ -1255,10 +1255,21 @@ public class DB_Oracle implements AdempiereDatabase
 			for(int i = 0; i < keyColumns.length; i++) {
 				if (i > 0)
 					sqlBuffer.append(" AND ");
-				sqlBuffer.append(keyColumns[i]).append(" = ? ");
+				sqlBuffer.append(keyColumns[i]).append("=?");
+			}
+			sqlBuffer.append(" FOR UPDATE WAIT ").append((timeout > 0 ? timeout : LOCK_TIME_OUT));
+
+			Object[] parameters = new Object[keyColumns.length];
+			for(int i = 0; i < keyColumns.length; i++) {
+				Object parameter = po.get_Value(keyColumns[i]);
+				if (parameter != null && parameter instanceof Boolean) {
+					if ((Boolean) parameter)
+						parameter = "Y";
+					else
+						parameter = "N";
+				}
+				parameters[i] = parameter;
 			}
-			sqlBuffer.append(" FOR UPDATE ");
-			sqlBuffer.append(" WAIT ").append((timeout > 0 ? timeout : LOCK_TIME_OUT));
 
 			PreparedStatement stmt = null;
 			ResultSet rs = null;
@@ -1266,7 +1277,7 @@ public class DB_Oracle implements AdempiereDatabase
 				stmt = DB.prepareStatement(sqlBuffer.toString(),
 					ResultSet.TYPE_FORWARD_ONLY, ResultSet.CONCUR_UPDATABLE, po.get_TrxName());
 				for(int i = 0; i < keyColumns.length; i++) {
-					stmt.setObject(i+1, po.get_Value(keyColumns[i]));
+					stmt.setObject(i+1, parameters[i]);
 				}
 				rs = stmt.executeQuery();
 				if (rs.next()) {
@@ -1276,6 +1287,7 @@ public class DB_Oracle implements AdempiereDatabase
 				}
 			} catch (Exception e) {
 				log.log(Level.INFO, e.getLocalizedMessage(), e);
+				throw new DBException("Could not lock record for " + po.toString() + " caused by " + e.getLocalizedMessage());
 			} finally {
 				DB.close(rs, stmt);
 			}			

org.compiere.db.postgresql.provider/src/org/compiere/db/DB_PostgreSQL.java

@@ -994,9 +994,22 @@ public class DB_PostgreSQL implements AdempiereDatabase
 			for(int i = 0; i < keyColumns.length; i++) {
 				if (i > 0)
 					sqlBuffer.append(" AND ");
-				sqlBuffer.append(keyColumns[i]).append(" = ? ");
+				sqlBuffer.append(keyColumns[i]).append("=?");
 			}
 			sqlBuffer.append(" FOR UPDATE ");
+
+			Object[] parameters = new Object[keyColumns.length];
+			for(int i = 0; i < keyColumns.length; i++) {
+				Object parameter = po.get_Value(keyColumns[i]);
+				if (parameter != null && parameter instanceof Boolean) {
+					if ((Boolean) parameter)
+						parameter = "Y";
+					else
+						parameter = "N";
+				}
+				parameters[i] = parameter;
+			}
+
 			PreparedStatement stmt = null;
 			ResultSet rs = null;
 			int currentTimeout = -1;
@@ -1004,7 +1017,7 @@ public class DB_PostgreSQL implements AdempiereDatabase
 				stmt = DB.prepareStatement(sqlBuffer.toString(),
 					ResultSet.TYPE_FORWARD_ONLY, ResultSet.CONCUR_UPDATABLE, po.get_TrxName());
 				for(int i = 0; i < keyColumns.length; i++) {
-					stmt.setObject(i+1, po.get_Value(keyColumns[i]));
+					stmt.setObject(i+1, parameters[i]);
 				}
 				currentTimeout = setStatementTimeout(stmt.getConnection(), (timeout > 0 ? timeout : LOCK_TIME_OUT));
 				
@@ -1016,6 +1029,7 @@ public class DB_PostgreSQL implements AdempiereDatabase
 				}
 			} catch (Exception e) {
 				log.log(Level.INFO, e.getLocalizedMessage(), e);
+				throw new DBException("Could not lock record for " + po.toString() + " caused by " + e.getLocalizedMessage());
 			} finally {
 				try {
 					setStatementTimeout(stmt.getConnection(), currentTimeout);