From 3e1293780c67fd9a8b6cd0173e989e03f053ab81 Mon Sep 17 00:00:00 2001
From: cramakri <cramakri>
Date: Tue, 12 Jun 2012 08:18:11 +0000
Subject: [PATCH] BIS-41 SP-57 Switch the update samples code in
 performEntityOperations to a batch-friendly implementation.

SVN: 25647
---
 .../openbis/generic/server/ETLService.java    |  15 +-
 .../server/batch/BatchOperationExecutor.java  |  36 ++++
 .../generic/server/batch/SampleUpdate.java    |  71 +++++++
 .../bo/AbstractSampleBusinessObject.java      |  15 ++
 .../server/business/bo/ISampleTable.java      |  10 +
 .../server/business/bo/SampleTable.java       | 179 ++++++++++++++++++
 .../server/ETLServiceDatabaseTest.java        | 108 +++++++++++
 .../generic/server/ETLServiceTest.java        |  13 +-
 8 files changed, 431 insertions(+), 16 deletions(-)
 create mode 100644 openbis/source/java/ch/systemsx/cisd/openbis/generic/server/batch/SampleUpdate.java

diff --git a/openbis/source/java/ch/systemsx/cisd/openbis/generic/server/ETLService.java b/openbis/source/java/ch/systemsx/cisd/openbis/generic/server/ETLService.java
index 36dd4057c35..30357e35a56 100644
--- a/openbis/source/java/ch/systemsx/cisd/openbis/generic/server/ETLService.java
+++ b/openbis/source/java/ch/systemsx/cisd/openbis/generic/server/ETLService.java
@@ -43,6 +43,8 @@ import ch.systemsx.cisd.common.serviceconversation.server.ServiceConversationSer
 import ch.systemsx.cisd.common.spring.HttpInvokerUtils;
 import ch.systemsx.cisd.common.spring.IInvocationLoggerContext;
 import ch.systemsx.cisd.openbis.generic.server.api.v1.SearchCriteriaToDetailedSearchCriteriaTranslator;
+import ch.systemsx.cisd.openbis.generic.server.batch.BatchOperationExecutor;
+import ch.systemsx.cisd.openbis.generic.server.batch.SampleUpdate;
 import ch.systemsx.cisd.openbis.generic.server.business.IDataStoreServiceFactory;
 import ch.systemsx.cisd.openbis.generic.server.business.IPropertiesBatchManager;
 import ch.systemsx.cisd.openbis.generic.server.business.bo.ICommonBusinessObjectFactory;
@@ -1531,16 +1533,11 @@ public class ETLService extends AbstractCommonServer<IETLLIMSService> implements
     private List<Sample> updateSamples(Session session,
             AtomicEntityOperationDetails operationDetails, IProgressListener progress)
     {
-        ArrayList<SamplePE> samplePEsUpdated = new ArrayList<SamplePE>();
         List<SampleUpdatesDTO> sampleUpdates = operationDetails.getSampleUpdates();
-        int index = 0;
-        for (SampleUpdatesDTO sampleUpdate : sampleUpdates)
-        {
-            SamplePE samplePE = updateSampleInternal(sampleUpdate, session).getSample();
-            samplePEsUpdated.add(samplePE);
-            progress.update("updateSamples", sampleUpdates.size(), ++index);
-        }
-        return SampleTranslator.translate(samplePEsUpdated, session.getBaseIndexURL());
+        ISampleTable sampleTable = businessObjectFactory.createSampleTable(session);
+        BatchOperationExecutor.executeInBatches(new SampleUpdate(sampleTable, sampleUpdates),
+                progress, "updateSamples");
+        return SampleTranslator.translate(sampleTable.getSamples(), session.getBaseIndexURL());
     }
 
     /**
diff --git a/openbis/source/java/ch/systemsx/cisd/openbis/generic/server/batch/BatchOperationExecutor.java b/openbis/source/java/ch/systemsx/cisd/openbis/generic/server/batch/BatchOperationExecutor.java
index 33e393040e8..4601a92ea17 100644
--- a/openbis/source/java/ch/systemsx/cisd/openbis/generic/server/batch/BatchOperationExecutor.java
+++ b/openbis/source/java/ch/systemsx/cisd/openbis/generic/server/batch/BatchOperationExecutor.java
@@ -4,6 +4,7 @@ import java.util.List;
 
 import org.apache.log4j.Logger;
 
+import ch.systemsx.cisd.common.conversation.IProgressListener;
 import ch.systemsx.cisd.common.logging.LogCategory;
 import ch.systemsx.cisd.common.logging.LogFactory;
 
@@ -24,7 +25,38 @@ public class BatchOperationExecutor
         executeInBatches(strategy, DEFAULT_BATCH_SIZE);
     }
 
+    /**
+     * Executes an operation in batches using the default batch size.
+     * 
+     * @param strategy The operation to execute
+     * @param progressListenerOrNull The progress listener to notify of progress. If this is
+     *            non-null, the progressPhaseOrNull must be non-null as well.
+     * @param progressPhaseOrNull The phase used in updating the progressListenerOrNull. Must be
+     *            non-null if the progressListenerOrNull is
+     */
+    public static <S> void executeInBatches(IBatchOperation<S> strategy,
+            IProgressListener progressListenerOrNull, String progressPhaseOrNull)
+    {
+        executeInBatches(strategy, DEFAULT_BATCH_SIZE, progressListenerOrNull, progressPhaseOrNull);
+    }
+
     public static <S> void executeInBatches(IBatchOperation<S> strategy, int batchSize)
+    {
+        executeInBatches(strategy, batchSize, null, null);
+    }
+
+    /**
+     * Executes an operation in batches.
+     * 
+     * @param strategy The operation to execute
+     * @param batchSize The size of the batches
+     * @param progressListenerOrNull The progress listener to notify of progress. If this is
+     *            non-null, the progressPhaseOrNull must be non-null as well.
+     * @param progressPhaseOrNull The phase used in updating the progressListenerOrNull. Must be
+     *            non-null if the progressListenerOrNull is
+     */
+    public static <S> void executeInBatches(IBatchOperation<S> strategy, int batchSize,
+            IProgressListener progressListenerOrNull, String progressPhaseOrNull)
     {
         assert strategy != null : "Unspecified operation.";
 
@@ -38,6 +70,10 @@ public class BatchOperationExecutor
         {
             final List<S> batch = allEntities.subList(startIndex, endIndex);
             strategy.execute(batch);
+            if (null != progressListenerOrNull)
+            {
+                progressListenerOrNull.update(progressPhaseOrNull, endIndex, maxIndex);
+            }
             operationLog.info(String.format("%s %s progress: %d/%d", strategy.getEntityName(),
                     strategy.getOperationName(), endIndex, maxIndex));
             if (operationLog.isDebugEnabled())
diff --git a/openbis/source/java/ch/systemsx/cisd/openbis/generic/server/batch/SampleUpdate.java b/openbis/source/java/ch/systemsx/cisd/openbis/generic/server/batch/SampleUpdate.java
new file mode 100644
index 00000000000..6c16b05bb43
--- /dev/null
+++ b/openbis/source/java/ch/systemsx/cisd/openbis/generic/server/batch/SampleUpdate.java
@@ -0,0 +1,71 @@
+/*
+ * Copyright 2010 ETH Zuerich, CISD
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package ch.systemsx.cisd.openbis.generic.server.batch;
+
+import java.util.List;
+
+import ch.systemsx.cisd.openbis.generic.server.business.bo.ISampleTable;
+import ch.systemsx.cisd.openbis.generic.shared.dto.SampleUpdatesDTO;
+
+/**
+ * {@link IBatchOperation} updating samples. It is like {@link SampleBatchUpdate}, but uses
+ * {@link SampleUpdatesDTO} to specify the updates instead of {@link SampleBatchUpdate} and thus has
+ * slightly different semantics.
+ * <p>
+ * Whereas SampleBatchUpdate only makes changes to the sample that are explicitly specified in it
+ * the details object of its DTO, SampleUpdate changes the sample to match the DTO.
+ * 
+ * @author Chandrasekhar Ramakrishnan
+ */
+public class SampleUpdate implements IBatchOperation<SampleUpdatesDTO>
+{
+    private final ISampleTable businessTable;
+
+    private final List<SampleUpdatesDTO> entities;
+
+    public SampleUpdate(ISampleTable businessTable, List<SampleUpdatesDTO> entities)
+    {
+        this.businessTable = businessTable;
+        this.entities = entities;
+    }
+
+    @Override
+    public void execute(List<SampleUpdatesDTO> updates)
+    {
+        businessTable.prepareForUpdateWithSampleUpdates(updates);
+        businessTable.save();
+    }
+
+    @Override
+    public List<SampleUpdatesDTO> getAllEntities()
+    {
+        return entities;
+    }
+
+    @Override
+    public String getEntityName()
+    {
+        return "sample";
+    }
+
+    @Override
+    public String getOperationName()
+    {
+        return "update";
+    }
+
+}
\ No newline at end of file
diff --git a/openbis/source/java/ch/systemsx/cisd/openbis/generic/server/business/bo/AbstractSampleBusinessObject.java b/openbis/source/java/ch/systemsx/cisd/openbis/generic/server/business/bo/AbstractSampleBusinessObject.java
index 2432d423ca1..642970fc091 100644
--- a/openbis/source/java/ch/systemsx/cisd/openbis/generic/server/business/bo/AbstractSampleBusinessObject.java
+++ b/openbis/source/java/ch/systemsx/cisd/openbis/generic/server/business/bo/AbstractSampleBusinessObject.java
@@ -628,6 +628,21 @@ abstract class AbstractSampleBusinessObject extends AbstractSampleIdentifierBusi
         return results;
     }
 
+    protected List<SamplePE> listSamplesByTechIds(final List<TechId> sampleTechIds)
+    {
+        assert sampleTechIds != null : "Sample identifiers unspecified.";
+
+        final ISampleDAO sampleDAO = getSampleDAO();
+        List<Long> ids = new ArrayList<Long>();
+        for (TechId sampleTechId : sampleTechIds)
+        {
+            ids.add(sampleTechId.getId());
+        }
+        final List<SamplePE> results = new ArrayList<SamplePE>();
+        results.addAll(sampleDAO.listByIDs(ids));
+        return results;
+    }
+
     /** Helper class encapsulating {@link SampleOwner} and code of container of a sample. */
     private static class SampleOwnerWithContainer
     {
diff --git a/openbis/source/java/ch/systemsx/cisd/openbis/generic/server/business/bo/ISampleTable.java b/openbis/source/java/ch/systemsx/cisd/openbis/generic/server/business/bo/ISampleTable.java
index edf03ef6e9e..82d421c8a5f 100644
--- a/openbis/source/java/ch/systemsx/cisd/openbis/generic/server/business/bo/ISampleTable.java
+++ b/openbis/source/java/ch/systemsx/cisd/openbis/generic/server/business/bo/ISampleTable.java
@@ -25,6 +25,7 @@ import ch.systemsx.cisd.openbis.generic.shared.dto.ListSamplesByPropertyCriteria
 import ch.systemsx.cisd.openbis.generic.shared.dto.PersonPE;
 import ch.systemsx.cisd.openbis.generic.shared.dto.SampleBatchUpdatesDTO;
 import ch.systemsx.cisd.openbis.generic.shared.dto.SamplePE;
+import ch.systemsx.cisd.openbis.generic.shared.dto.SampleUpdatesDTO;
 
 /**
  * A generic sample <i>Business Object</i>.
@@ -58,6 +59,15 @@ public interface ISampleTable
      */
     public void prepareForUpdate(List<SampleBatchUpdatesDTO> updates) throws UserFailureException;
 
+    /**
+     * Comparable to {@link #prepareForUpdate(List)} but takes a {@link SampleUpdatesDTO} object
+     * instead of a {@link SampleBatchUpdatesDTO} object. Whereas prepareForUpdate only changes the
+     * fields requested in the updates' details object, this method changes the samples to match the
+     * updates object.
+     */
+    void prepareForUpdateWithSampleUpdates(List<SampleUpdatesDTO> updates)
+            throws UserFailureException;
+
     /**
      * Writes added data to the Data Access Layers.
      */
diff --git a/openbis/source/java/ch/systemsx/cisd/openbis/generic/server/business/bo/SampleTable.java b/openbis/source/java/ch/systemsx/cisd/openbis/generic/server/business/bo/SampleTable.java
index ee5c8706d91..541b70a0787 100644
--- a/openbis/source/java/ch/systemsx/cisd/openbis/generic/server/business/bo/SampleTable.java
+++ b/openbis/source/java/ch/systemsx/cisd/openbis/generic/server/business/bo/SampleTable.java
@@ -17,13 +17,16 @@
 package ch.systemsx.cisd.openbis.generic.server.business.bo;
 
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
 
 import org.springframework.dao.DataAccessException;
 
+import ch.systemsx.cisd.common.exceptions.EnvironmentFailureException;
 import ch.systemsx.cisd.common.exceptions.UserFailureException;
 import ch.systemsx.cisd.openbis.generic.server.business.bo.util.SampleOwner;
 import ch.systemsx.cisd.openbis.generic.server.dataaccess.IDAOFactory;
@@ -41,6 +44,7 @@ import ch.systemsx.cisd.openbis.generic.shared.dto.SampleBatchUpdatesDTO;
 import ch.systemsx.cisd.openbis.generic.shared.dto.SamplePE;
 import ch.systemsx.cisd.openbis.generic.shared.dto.SamplePropertyPE;
 import ch.systemsx.cisd.openbis.generic.shared.dto.SampleTypePE;
+import ch.systemsx.cisd.openbis.generic.shared.dto.SampleUpdatesDTO;
 import ch.systemsx.cisd.openbis.generic.shared.dto.Session;
 import ch.systemsx.cisd.openbis.generic.shared.dto.SpacePE;
 import ch.systemsx.cisd.openbis.generic.shared.dto.identifier.ExperimentIdentifier;
@@ -276,6 +280,113 @@ public final class SampleTable extends AbstractSampleBusinessObject implements I
         }
     }
 
+    /**
+     * Prepare a batch update using the SampleUpdatesDTO, not the SampleBatchUpdatesDTO. This
+     * version assumes that all the properties are provided in the updates object, not just those
+     * that should explicitly be updated.
+     */
+    private void prepareBatchUpdate(SamplePE sample, SampleUpdatesDTO updates,
+            Map<SampleOwnerIdentifier, SampleOwner> sampleOwnerCache,
+            Map<String, ExperimentPE> experimentCache,
+            Map<EntityTypePE, List<EntityTypePropertyTypePE>> propertiesCache)
+    {
+        if (sample == null)
+        {
+            throw UserFailureException.fromTemplate(
+                    "No sample could be found with given identifier '%s'.",
+                    updates.getSampleIdentifier());
+        }
+
+        updateProperties(sample, updates.getProperties());
+        checkPropertiesBusinessRules(sample, propertiesCache);
+
+        updateSpace(sample, updates.getSampleIdentifier(), sampleOwnerCache);
+        if (updates.isUpdateExperimentLink())
+        {
+            updateExperiment(sample, updates.getExperimentIdentifierOrNull(), experimentCache);
+            checkExperimentBusinessRules(getDataDAO(), sample);
+        }
+
+        boolean parentsUpdated = updateParents(sample, updates);
+
+        boolean containerUpdated = updateContainer(sample, updates);
+
+        // NOTE: Checking business rules with relationships is expensive.
+        // Don't perform them unless relevant data were changed.
+        if (updates.isUpdateExperimentLink() || parentsUpdated)
+        {
+            checkParentBusinessRules(sample);
+        }
+        if (updates.isUpdateExperimentLink() || containerUpdated)
+        {
+            checkContainerBusinessRules(sample);
+        }
+    }
+
+    private boolean updateContainer(SamplePE sample, SampleUpdatesDTO updates)
+    {
+        SamplePE container = sample.getContainer();
+        String oldContainerIdentifierOrNull =
+                (null == container) ? null : container.getIdentifier();
+        // Figure out if we need to update the container
+        if (oldContainerIdentifierOrNull == null)
+        {
+            if (updates.getContainerIdentifierOrNull() == null)
+            {
+                return false;
+            }
+        } else
+        {
+            if (oldContainerIdentifierOrNull.equals(updates.getContainerIdentifierOrNull()))
+            {
+                return false;
+            }
+        }
+        setContainer(updates.getSampleIdentifier(), sample, updates.getContainerIdentifierOrNull(),
+                null);
+        return true;
+    }
+
+    /**
+     * Update parents and return whether or not something was changed.
+     * 
+     * @return True if the parents were changed, false if nothing was changed
+     */
+    private boolean updateParents(SamplePE sample, SampleUpdatesDTO updates)
+    {
+        final String[] newParents = updates.getModifiedParentCodesOrNull();
+        // If the parents are null, don't touch them
+        if (null == newParents)
+        {
+            return false;
+        }
+
+        // Compare the old and new parents
+        Set<String> oldParentsSet = new HashSet<String>();
+        for (SamplePE parent : sample.getParents())
+        {
+            oldParentsSet.add(parent.getCode());
+        }
+        Set<String> newParentsSet = new HashSet<String>();
+        newParentsSet.addAll(Arrays.asList(newParents));
+
+        // Nothing to change
+        if (oldParentsSet.equals(newParentsSet))
+        {
+            return false;
+        }
+
+        setParents(sample, newParents, null);
+        return true;
+    }
+
+    private void updateProperties(SamplePE sample, List<IEntityProperty> properties)
+    {
+        final Set<SamplePropertyPE> existingProperties = sample.getProperties();
+        final SampleTypePE type = sample.getSampleType();
+        sample.setProperties(convertProperties(type, existingProperties, properties));
+    }
+
     private void batchUpdateProperties(SamplePE sample, List<IEntityProperty> properties,
             Set<String> propertiesToUpdate)
     {
@@ -318,6 +429,74 @@ public final class SampleTable extends AbstractSampleBusinessObject implements I
         setBatchUpdateMode(false);
     }
 
+    @Override
+    public void prepareForUpdateWithSampleUpdates(List<SampleUpdatesDTO> updates)
+    {
+        assert updates != null : "Unspecified samples.";
+
+        setBatchUpdateMode(true);
+        final Map<SampleOwnerIdentifier, SampleOwner> sampleOwnerCache =
+                new HashMap<SampleOwnerIdentifier, SampleOwner>();
+        final Map<String, ExperimentPE> experimentCache = new HashMap<String, ExperimentPE>();
+        final Map<EntityTypePE, List<EntityTypePropertyTypePE>> propertiesCache =
+                new HashMap<EntityTypePE, List<EntityTypePropertyTypePE>>();
+        samples = loadSamplesByTechId(updates);
+        Map<Long, SamplePE> samplesById = new HashMap<Long, SamplePE>();
+        for (SamplePE sample : samples)
+        {
+            samplesById.put(sample.getId(), sample);
+        }
+        for (SampleUpdatesDTO sampleUpdates : updates)
+        {
+            Long id =
+                    (null == sampleUpdates.getSampleIdOrNull()) ? null : sampleUpdates
+                            .getSampleIdOrNull().getId();
+            if (null == id)
+            {
+                throw new UserFailureException("Sample with identifier "
+                        + sampleUpdates.getSampleIdentifier()
+                        + " is not in the database and therefore cannot be updated.");
+            }
+
+            final SamplePE sample = samplesById.get(id);
+            if (null == sample)
+            {
+                throw new UserFailureException("Sample with identifier "
+                        + sampleUpdates.getSampleIdentifier()
+                        + " is not in the database and therefore cannot be updated.");
+            }
+            if (false == sample.getModificationDate().equals(sampleUpdates.getVersion()))
+            {
+                throw new EnvironmentFailureException("Sample with identifier "
+                        + sampleUpdates.getSampleIdentifier()
+                        + " has been updated since it was retrieved.");
+            }
+            prepareBatchUpdate(sample, sampleUpdates, sampleOwnerCache, experimentCache,
+                    propertiesCache);
+        }
+
+        dataChanged = true;
+        businessRulesChecked = true;
+
+        setBatchUpdateMode(false);
+    }
+
+    private List<SamplePE> loadSamplesByTechId(List<SampleUpdatesDTO> updates)
+    {
+        final List<SamplePE> results = new ArrayList<SamplePE>();
+        List<TechId> identifiers = new ArrayList<TechId>();
+        for (SampleUpdatesDTO sampleUpdates : updates)
+        {
+            TechId id = sampleUpdates.getSampleIdOrNull();
+            if (id != null)
+            {
+                identifiers.add(id);
+            }
+        }
+        results.addAll(listSamplesByTechIds(identifiers));
+        return results;
+    }
+
     private List<SamplePE> loadSamples(List<SampleBatchUpdatesDTO> updates,
             Map<SampleOwnerIdentifier, SampleOwner> sampleOwnerCache)
     {
diff --git a/openbis/sourceTest/java/ch/systemsx/cisd/openbis/generic/server/ETLServiceDatabaseTest.java b/openbis/sourceTest/java/ch/systemsx/cisd/openbis/generic/server/ETLServiceDatabaseTest.java
index 6c7c774dd76..7fe387dd1d7 100644
--- a/openbis/sourceTest/java/ch/systemsx/cisd/openbis/generic/server/ETLServiceDatabaseTest.java
+++ b/openbis/sourceTest/java/ch/systemsx/cisd/openbis/generic/server/ETLServiceDatabaseTest.java
@@ -17,11 +17,15 @@
 package ch.systemsx.cisd.openbis.generic.server;
 
 import static junit.framework.Assert.assertEquals;
+import static junit.framework.Assert.assertFalse;
 import static junit.framework.Assert.assertTrue;
 
 import java.sql.SQLException;
+import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
+import java.util.Map;
+import java.util.Set;
 
 import org.springframework.beans.factory.annotation.Autowired;
 import org.testng.annotations.BeforeClass;
@@ -29,11 +33,28 @@ import org.testng.annotations.Test;
 
 import ch.systemsx.cisd.openbis.generic.server.dataaccess.db.AbstractDAOTest;
 import ch.systemsx.cisd.openbis.generic.shared.IETLLIMSService;
+import ch.systemsx.cisd.openbis.generic.shared.api.v1.dto.SearchCriteria;
+import ch.systemsx.cisd.openbis.generic.shared.api.v1.dto.SearchCriteria.MatchClauseAttribute;
+import ch.systemsx.cisd.openbis.generic.shared.basic.TechId;
 import ch.systemsx.cisd.openbis.generic.shared.basic.dto.Experiment;
 import ch.systemsx.cisd.openbis.generic.shared.basic.dto.ExperimentFetchOption;
 import ch.systemsx.cisd.openbis.generic.shared.basic.dto.ExperimentFetchOptions;
+import ch.systemsx.cisd.openbis.generic.shared.basic.dto.NewAttachment;
+import ch.systemsx.cisd.openbis.generic.shared.basic.dto.NewExperiment;
+import ch.systemsx.cisd.openbis.generic.shared.basic.dto.NewMaterial;
+import ch.systemsx.cisd.openbis.generic.shared.basic.dto.NewProject;
+import ch.systemsx.cisd.openbis.generic.shared.basic.dto.NewSample;
+import ch.systemsx.cisd.openbis.generic.shared.basic.dto.NewSpace;
+import ch.systemsx.cisd.openbis.generic.shared.basic.dto.Sample;
+import ch.systemsx.cisd.openbis.generic.shared.dto.AtomicEntityOperationDetails;
+import ch.systemsx.cisd.openbis.generic.shared.dto.DataSetUpdatesDTO;
+import ch.systemsx.cisd.openbis.generic.shared.dto.NewExternalData;
+import ch.systemsx.cisd.openbis.generic.shared.dto.SampleUpdatesDTO;
 import ch.systemsx.cisd.openbis.generic.shared.dto.identifier.ExperimentIdentifier;
+import ch.systemsx.cisd.openbis.generic.shared.dto.identifier.ExperimentIdentifierFactory;
 import ch.systemsx.cisd.openbis.generic.shared.dto.identifier.ProjectIdentifier;
+import ch.systemsx.cisd.openbis.generic.shared.dto.identifier.SampleIdentifierFactory;
+import ch.systemsx.cisd.openbis.generic.shared.util.EntityHelper;
 
 /**
  * @author pkupczyk
@@ -108,4 +129,91 @@ public class ETLServiceDatabaseTest extends AbstractDAOTest
         assertTrue(result.get(0).getFetchOptions().isSetOf(ExperimentFetchOption.values()));
     }
 
+    @Test
+    public void testPerformEntityOperationsUpdateSample()
+    {
+        // Find the samples to add
+        SearchCriteria searchCriteria = new SearchCriteria();
+        searchCriteria.addMatchClause(SearchCriteria.MatchClause.createAttributeMatch(
+                MatchClauseAttribute.CODE, "3VCP7"));
+        List<Sample> samplesToUpdate = service.searchForSamples(sessionToken, searchCriteria);
+        assertEquals(1, samplesToUpdate.size());
+        Sample sampleToUpdate = samplesToUpdate.get(0);
+
+        // Update the comment
+        String newComment = "This is a new comment. This is not the old comment.";
+        String oldComment = EntityHelper.tryFindPropertyValue(sampleToUpdate, "COMMENT");
+        assertFalse(newComment.equals(oldComment));
+        EntityHelper.createOrUpdateProperty(sampleToUpdate, "COMMENT", newComment);
+
+        // Update the parents
+        SearchCriteria parentSearchCriteria = new SearchCriteria();
+        parentSearchCriteria.addMatchClause(SearchCriteria.MatchClause.createAttributeMatch(
+                MatchClauseAttribute.CODE, "3V-126"));
+        List<Sample> parentsToAdd = service.searchForSamples(sessionToken, parentSearchCriteria);
+        assertEquals(1, parentsToAdd.size());
+        Sample parentToAdd = parentsToAdd.get(0);
+        assertEquals(1, sampleToUpdate.getParents().size());
+        sampleToUpdate.addParent(parentToAdd);
+
+        performSampleUpdate(sampleToUpdate);
+
+        // Now retrieve the sample again and check that the properties were updated.
+        List<Sample> updatedSamples = service.searchForSamples(sessionToken, searchCriteria);
+        assertEquals(1, updatedSamples.size());
+        Sample updatedSample = updatedSamples.get(0);
+        assertTrue("The modification date should have been updated", updatedSample
+                .getModificationDate().compareTo(sampleToUpdate.getModificationDate()) > 0);
+        assertEquals(newComment, EntityHelper.tryFindPropertyValue(updatedSample, "COMMENT"));
+        assertEquals(2, updatedSample.getParents().size());
+    }
+
+    private void performSampleUpdate(Sample sampleToUpdate)
+    {
+        TechId registrationid = new TechId(service.drawANewUniqueID(sessionToken));
+        List<NewSpace> spaceRegistrations = Collections.emptyList();
+        List<NewProject> projectRegistrations = Collections.emptyList();
+        List<NewExperiment> experimentRegistrations = Collections.emptyList();
+
+        SampleUpdatesDTO sampleUpdate = convertToSampleUpdateDTO(sampleToUpdate);
+        List<SampleUpdatesDTO> sampleUpdates = Arrays.asList(sampleUpdate);
+        List<NewSample> sampleRegistrations = Collections.emptyList();
+        Map<String, List<NewMaterial>> materialRegistrations = Collections.emptyMap();
+        List<? extends NewExternalData> dataSetRegistrations = Collections.emptyList();
+        List<DataSetUpdatesDTO> dataSetUpdates = Collections.emptyList();
+        AtomicEntityOperationDetails details =
+                new AtomicEntityOperationDetails(registrationid, null, spaceRegistrations,
+                        projectRegistrations, experimentRegistrations, sampleUpdates,
+                        sampleRegistrations, materialRegistrations, dataSetRegistrations,
+                        dataSetUpdates);
+        service.performEntityOperations(sessionToken, details);
+    }
+
+    private SampleUpdatesDTO convertToSampleUpdateDTO(Sample sample)
+    {
+        List<NewAttachment> attachments = Collections.emptyList();
+        String containerIdentifier =
+                (sample.getContainer() != null) ? sample.getContainer().getIdentifier() : null;
+        Set<Sample> sampleParents = sample.getParents();
+        String[] parentCodes = new String[sampleParents.size()];
+        int i = 0;
+        for (Sample parent : sampleParents)
+        {
+            parentCodes[i++] = parent.getCode();
+        }
+        SampleUpdatesDTO sampleUpdate =
+                new SampleUpdatesDTO(TechId.create(sample), // db id
+                        sample.getProperties(), // List<IEntityProperty>
+                        sample.getExperiment() == null ? null
+                                : ExperimentIdentifierFactory.parse(sample.getExperiment()
+                                        .getIdentifier()), // ExperimentIdentifier
+                        attachments, // Collection<NewAttachment>
+                        sample.getModificationDate(), // Sample version
+                        SampleIdentifierFactory.parse(sample.getIdentifier()), // Sample Identifier
+                        containerIdentifier, // Container Identifier
+                        parentCodes // Parent Identifiers
+                );
+        return sampleUpdate;
+    }
+
 }
diff --git a/openbis/sourceTest/java/ch/systemsx/cisd/openbis/generic/server/ETLServiceTest.java b/openbis/sourceTest/java/ch/systemsx/cisd/openbis/generic/server/ETLServiceTest.java
index 13a60c0d589..4f3ba1cf252 100644
--- a/openbis/sourceTest/java/ch/systemsx/cisd/openbis/generic/server/ETLServiceTest.java
+++ b/openbis/sourceTest/java/ch/systemsx/cisd/openbis/generic/server/ETLServiceTest.java
@@ -1093,13 +1093,12 @@ public class ETLServiceTest extends AbstractServerTestCase
                     one(sampleTable).getSamples();
                     will(returnValue(Arrays.asList(newSamplePE)));
 
-                    one(boFactory).createSampleBO(SESSION);
-                    will(returnValue(sampleBO));
-
-                    one(sampleBO).update(sampleUpdate);
-                    one(sampleBO).save();
-                    one(sampleBO).getSample();
-                    will(returnValue(samplePE));
+                    one(boFactory).createSampleTable(SESSION);
+                    will(returnValue(sampleTable));
+                    one(sampleTable).prepareForUpdateWithSampleUpdates(Arrays.asList(sampleUpdate));
+                    one(sampleTable).save();
+                    one(sampleTable).getSamples();
+                    will(returnValue(Arrays.asList(samplePE)));
                 }
             });
 
-- 
GitLab