Fix and test CycleCheckAction and CheckEdgesAction

experiment/src/org/labkey/experiment/ExperimentModule.java

@@ -117,6 +117,7 @@
 import org.labkey.api.webdav.WebdavService;
 import org.labkey.api.writer.ContainerUser;
 import org.labkey.experiment.api.DataClassDomainKind;
+import org.labkey.experiment.api.EdgeDiagnosticsTestCase;
 import org.labkey.experiment.api.ExpDataClassImpl;
 import org.labkey.experiment.api.ExpDataClassTableImpl;
 import org.labkey.experiment.api.ExpDataClassType;
@@ -1114,6 +1115,7 @@ public Collection<String> getSummary(Container c)
     public @NotNull Set<Class<?>> getIntegrationTests()
     {
         return Set.of(
+            EdgeDiagnosticsTestCase.class,
             DomainImpl.TestCase.class,
             DomainPropertyImpl.TestCase.class,
             ExpDataTableImpl.TestCase.class,

experiment/src/org/labkey/experiment/api/EdgeDiagnosticsTestCase.java

@@ -0,0 +1,134 @@
+package org.labkey.experiment.api;
+
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+import org.labkey.api.collections.CaseInsensitiveHashMap;
+import org.labkey.api.data.ContainerFilter;
+import org.labkey.api.data.DbSchema;
+import org.labkey.api.data.SQLFragment;
+import org.labkey.api.data.SqlExecutor;
+import org.labkey.api.data.TableInfo;
+import org.labkey.api.exp.api.ExpMaterial;
+import org.labkey.api.exp.api.ExpObject;
+import org.labkey.api.exp.api.ExperimentService;
+import org.labkey.api.exp.api.SampleTypeService;
+import org.labkey.api.gwt.client.model.GWTPropertyDescriptor;
+import org.labkey.api.query.BatchValidationException;
+import org.labkey.api.query.QueryService;
+import org.labkey.api.query.QueryUpdateService;
+import org.labkey.api.query.SchemaKey;
+import org.labkey.api.query.UserSchema;
+import org.labkey.api.security.User;
+import org.labkey.api.util.JunitUtil;
+import org.labkey.api.util.Pair;
+import org.labkey.api.util.TestContext;
+import org.labkey.experiment.controllers.exp.ExperimentController;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+
+public class EdgeDiagnosticsTestCase extends Assert
+{
+    private User _user;
+    private ExpMaterial _sampleA;
+    private ExpMaterial _sampleB;
+
+    @Before
+    public void setUp() throws Exception
+    {
+        JunitUtil.deleteTestContainer();
+        _user = TestContext.get().getUser();
+        var container = JunitUtil.getTestContainer();
+
+        List<GWTPropertyDescriptor> props = new ArrayList<>();
+        props.add(new GWTPropertyDescriptor("name", "string"));
+        var sampleType = SampleTypeService.get().createSampleType(container, _user, "EdgeDiagSamples", null, props, Collections.emptyList(), -1, -1, -1, -1, null);
+
+        UserSchema schema = QueryService.get().getUserSchema(_user, container, SchemaKey.fromParts("Samples"));
+        TableInfo table = schema.getTable("EdgeDiagSamples");
+        QueryUpdateService svc = table.getUpdateService();
+        BatchValidationException errors = new BatchValidationException();
+        svc.insertRows(_user, container, List.of(
+                CaseInsensitiveHashMap.of("name", "edgeA"),
+                CaseInsensitiveHashMap.of("name", "edgeB")
+        ), errors, null, null);
+        if (errors.hasErrors())
+            throw errors;
+
+        _sampleA = sampleType.getSample(container, "edgeA");
+        _sampleB = sampleType.getSample(container, "edgeB");
+    }
+
+    @After
+    public void tearDown()
+    {
+        JunitUtil.deleteTestContainer();
+    }
+
+    @Test
+    public void testCycleCheckActionDetectsAndResolves()
+    {
+        Long idA = _sampleA.getObjectId();
+        Long idB = _sampleB.getObjectId();
+        DbSchema expSchema = ExperimentService.get().getSchema();
+        try
+        {
+            insertCycleEdges(expSchema, idA, idB);
+
+            List<Long> cycleIds = ExperimentController.CycleCheckAction.detectCycleObjectIds(expSchema);
+            assertNotNull("Cycle should be detected", cycleIds);
+            assertTrue("Cycle should include idA", cycleIds.contains(idA));
+            assertTrue("Cycle should include idB", cycleIds.contains(idB));
+
+            Map<Long, ExpObject> resolved = ExperimentController.CycleCheckAction.resolveCycleObjects(new ContainerFilter.AllFolders(_user), cycleIds);
+            assertEquals(_sampleA.getName(), resolved.get(idA).getName());
+            assertEquals(_sampleB.getName(), resolved.get(idB).getName());
+        }
+        finally
+        {
+            deleteCycleEdges(expSchema, idA, idB);
+        }
+    }
+
+    @Test
+    public void testCheckEdgesActionDetectsCycles()
+    {
+        Long idA = _sampleA.getObjectId();
+        Long idB = _sampleB.getObjectId();
+        DbSchema expSchema = ExperimentService.get().getSchema();
+        try
+        {
+            insertCycleEdges(expSchema, idA, idB);
+
+            Collection<Pair<Long, Long>> cycleEdges = ExperimentController.CheckEdgesAction.detectCycleEdges(expSchema);
+            assertFalse("Cycle edges should be detected", cycleEdges.isEmpty());
+            assertTrue("idA should appear in a cycle edge",
+                    cycleEdges.stream().anyMatch(e -> e.first.equals(idA) || e.second.equals(idA)));
+            assertTrue("idB should appear in a cycle edge",
+                    cycleEdges.stream().anyMatch(e -> e.first.equals(idB) || e.second.equals(idB)));
+        }
+        finally
+        {
+            deleteCycleEdges(expSchema, idA, idB);
+        }
+    }
+
+    private static void insertCycleEdges(DbSchema schema, Long idA, Long idB)
+    {
+        var executor = new SqlExecutor(schema);
+        executor.execute(new SQLFragment("INSERT INTO exp.Edge (FromObjectId, ToObjectId) VALUES (?,?)", idA, idB));
+        executor.execute(new SQLFragment("INSERT INTO exp.Edge (FromObjectId, ToObjectId) VALUES (?,?)", idB, idA));
+    }
+
+    private static void deleteCycleEdges(DbSchema schema, Long idA, Long idB)
+    {
+        new SqlExecutor(schema).execute(new SQLFragment(
+                "DELETE FROM exp.Edge WHERE (FromObjectId=? AND ToObjectId=?) OR (FromObjectId=? AND ToObjectId=?)",
+                idA, idB, idB, idA));
+    }
+}

experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java

@@ -991,7 +991,7 @@ public List<ExpMaterialImpl> getExpMaterials(Container container, User user, Col
         return getExpMaterials(filter);
     }
 
-    public List<ExpMaterialImpl> getExpMaterialsByObjectId(ContainerFilter containerFilter, Collection<Integer> objectIds)
+    public List<ExpMaterialImpl> getExpMaterialsByObjectId(ContainerFilter containerFilter, Collection<Long> objectIds)
     {
         if (objectIds.isEmpty())
             return emptyList();
@@ -1871,7 +1871,7 @@ public List<? extends ExpData> getExpDatas(ExpDataClass dataClass)
         return datas.stream().map(ExpDataImpl::new).collect(toList());
     }
 
-    public List<ExpDataImpl> getExpDatasByObjectId(ContainerFilter containerFilter, Collection<Integer> objectIds)
+    public List<ExpDataImpl> getExpDatasByObjectId(ContainerFilter containerFilter, Collection<Long> objectIds)
     {
         SimpleFilter filter = new SimpleFilter();
         filter.addInClause(FieldKey.fromParts("ObjectId"), objectIds);
@@ -5711,7 +5711,7 @@ public List<ExpRunImpl> getRunsUsingDataIds(List<Long> ids)
         return ExpRunImpl.fromRuns(new SqlSelector(getExpSchema(), sql).getArrayList(ExperimentRun.class));
     }
 
-    public List<ExpRunImpl> getRunsByObjectId(ContainerFilter containerFilter, Collection<Integer> objectIds)
+    public List<ExpRunImpl> getRunsByObjectId(ContainerFilter containerFilter, Collection<Long> objectIds)
     {
         SimpleFilter filter = new SimpleFilter();
         filter.addInClause(FieldKey.fromParts("ObjectId"), objectIds);
@@ -7550,13 +7550,13 @@ else if (_aliquotRootCache.containsKey(parent.getLSID()))
                         _aliquotRootCache.put(outputAliquot.getLSID(), rootMaterialRowId); // add self's root to cache
 
                         sql.addAll(rec._protApp.getRowId(), rec._protApp._object.getRunId(), rootMaterialRowId, parent.getLSID(), outputAliquot.getRowId());
-                        
+
                         new SqlExecutor(tableInfo.getSchema()).execute(sql);
                     }
                 }
             }
         }
-        
+
         private void saveExpMaterialOutputs(List<ProtocolAppRecord> protAppRecords)
         {
             for (ProtocolAppRecord rec : protAppRecords)

experiment/src/org/labkey/experiment/controllers/exp/ExperimentController.java

@@ -7386,12 +7386,7 @@ public Object execute(Object o, BindException errors) throws Exception
 
             if (null != edgeTable.getColumn("fromObjectId"))
             {
-                var edges = new SqlSelector(ExperimentService.get().getSchema(), "SELECT fromObjectId, toObjectId FROM exp.Edge")
-                        .resultSetStream()
-                        .map(r -> { try { return new Pair<>(r.getInt(1), r.getInt(2)); } catch (SQLException x) { throw new RuntimeException(x); } })
-                        .collect(toList());
-                var cycles = (new GraphAlgorithms<Integer>()).detectCycleInDirectedGraph(edges);
-                result = cycles.stream().map(e -> new Integer[]{e.first, e.second}).collect(toList());
+                result = detectCycleEdges(schema).stream().map(e -> new Long[]{e.first, e.second}).collect(toList());
             }
             else
             {
@@ -7408,6 +7403,15 @@ public Object execute(Object o, BindException errors) throws Exception
             ret.put("success", true);
             return ret;
         }
+
+        public static Collection<Pair<Long, Long>> detectCycleEdges(DbSchema schema)
+        {
+            var edges = new SqlSelector(schema, "SELECT fromObjectId, toObjectId FROM exp.Edge")
+                    .resultSetStream()
+                    .map(r -> { try { return new Pair<>(r.getLong(1), r.getLong(2)); } catch (SQLException x) { throw new RuntimeException(x); } })
+                    .collect(toList());
+            return (new GraphAlgorithms<Long>()).detectCycleInDirectedGraph(edges);
+        }
     }
 
     @RequiresPermission(UpdatePermission.class)
@@ -7445,7 +7449,7 @@ private static long getSampleId(QueryUpdateForm tableForm)
             {
                 sampleId = ConvertHelper.convert(tableForm.getPkVal(), Long.class);
             }
-            catch (ConversionException e)
+            catch (ConversionException _)
             {
             }
             if (null == sampleId)
@@ -8194,7 +8198,7 @@ public ModelAndView getView(Object o, BindException errors) throws SQLException
     @RequiresPermission(TroubleshooterPermission.class)
     public static class CycleCheckAction extends FormViewAction<Object>
     {
-        List<Integer> cycleObjectIds = null;
+        List<Long> cycleObjectIds = null;
 
         @Override
         public void validateCommand(Object target, Errors errors)
@@ -8217,29 +8221,25 @@ public ModelAndView getView(Object o, boolean reshow, BindException errors)
             if (null == cycleObjectIds)
                 return new HtmlView(HtmlString.of("No cycles found"));
 
-            Map<Long, ExpObject> map = new LongHashMap<>();
             var cf = new ContainerFilter.AllFolders(getUser());
-            var materials = ExperimentServiceImpl.get().getExpMaterialsByObjectId(cf, cycleObjectIds);
-            materials.forEach( (m) -> map.put(m.getObjectId(), m));
-            var datas = ExperimentServiceImpl.get().getExpDatasByObjectId(cf, cycleObjectIds);
-            datas.forEach( (d) -> map.put(d.getObjectId(), d));
-            var runs = ExperimentServiceImpl.get().getRunsByObjectId(cf, cycleObjectIds);
-            runs.forEach( (r) -> map.put(r.getObjectId(), r));
+            Map<Long, ExpObject> map = resolveCycleObjects(cf, cycleObjectIds);
 
             ExperimentUrls urls = ExperimentUrls.get();
             return new HtmlView(
                     DIV("Cycle found involving these objects.",
                             UL(cycleObjectIds.stream().map((objectid) ->
                             {
                                 ExpObject exp = map.get(objectid);
-                                if (exp instanceof ExpMaterial mat)
-                                    return LI(A(at(target, "_blank", href, urls.getMaterialDetailsURL(mat)), objectid + " : material - " + mat.getName()));
-                                else if (exp instanceof ExpRun run)
-                                    return LI(A(at(target, "_blank", href, urls.getRunTextURL(run)), objectid + " : run - " + run.getName()));
-                                else if (exp instanceof ExpData data)
-                                    return LI(A(at(target, "_blank", href, urls.getDataDetailsURL(data)), objectid + " : run - " + data.getName()));
-                                else
-                                    return LI(String.valueOf(objectid));
+                                return switch (exp)
+                                {
+                                    case ExpMaterial mat ->
+                                            LI(A(at(target, "_blank", href, urls.getMaterialDetailsURL(mat)), objectid + " : material - " + mat.getName()));
+                                    case ExpRun run ->
+                                            LI(A(at(target, "_blank", href, urls.getRunTextURL(run)), objectid + " : run - " + run.getName()));
+                                    case ExpData data ->
+                                            LI(A(at(target, "_blank", href, urls.getDataDetailsURL(data)), objectid + " : run - " + data.getName()));
+                                    case null, default -> LI(objectid + " : unknown object");
+                                };
                             }))
                     )
             );
@@ -8248,18 +8248,7 @@ else if (exp instanceof ExpData data)
         @Override
         public boolean handlePost(Object o, BindException errors)
         {
-            var edges = new SqlSelector(ExperimentService.get().getSchema(), "SELECT fromObjectId, toObjectId FROM exp.Edge")
-                    .resultSetStream()
-                    .map(r -> { try { return new Pair<>(r.getInt(1), r.getInt(2)); } catch (SQLException x) { throw new RuntimeException(x); } })
-                    .collect(toList());
-            var cyclesEdges = (new GraphAlgorithms<Integer>()).detectCycleInDirectedGraph(edges);
-
-            var set = new LinkedHashSet<Integer>();
-            cyclesEdges.forEach( (edge) -> {
-                set.add(edge.first);
-                set.add(edge.second);
-            });
-            cycleObjectIds = set.stream().toList();
+            cycleObjectIds = detectCycleObjectIds(ExperimentService.get().getSchema());
             return false;
         }
 
@@ -8272,8 +8261,32 @@ public URLHelper getSuccessURL(Object o)
         @Override
         public void addNavTrail(NavTree root)
         {
+            root.addChild("Cycle check");
+        }
 
+        public static @Nullable List<Long> detectCycleObjectIds(DbSchema schema)
+        {
+            var edges = new SqlSelector(schema, "SELECT fromObjectId, toObjectId FROM exp.Edge")
+                    .resultSetStream()
+                    .map(r -> { try { return new Pair<>(r.getLong(1), r.getLong(2)); } catch (SQLException x) { throw new RuntimeException(x); } })
+                    .collect(toList());
+            var cyclesEdges = (new GraphAlgorithms<Long>()).detectCycleInDirectedGraph(edges);
+            if (cyclesEdges.isEmpty())
+                return null;
+            var set = new LinkedHashSet<Long>();
+            cyclesEdges.forEach(e -> { set.add(e.first); set.add(e.second); });
+            return set.stream().toList();
         }
+
+        public static Map<Long, ExpObject> resolveCycleObjects(ContainerFilter cf, List<Long> ids)
+        {
+            Map<Long, ExpObject> map = new LongHashMap<>();
+            ExperimentServiceImpl.get().getExpMaterialsByObjectId(cf, ids).forEach(m -> map.put(m.getObjectId(), m));
+            ExperimentServiceImpl.get().getExpDatasByObjectId(cf, ids).forEach(d -> map.put(d.getObjectId(), d));
+            ExperimentServiceImpl.get().getRunsByObjectId(cf, ids).forEach(r -> map.put(r.getObjectId(), r));
+            return map;
+        }
+
    }
 
     @RequiresPermission(AdminPermission.class)