diff --git a/experiment/src/org/labkey/experiment/ExperimentModule.java b/experiment/src/org/labkey/experiment/ExperimentModule.java index 5c9c65763ae..8bc6e44d4da 100644 --- a/experiment/src/org/labkey/experiment/ExperimentModule.java +++ b/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 getSummary(Container c) public @NotNull Set> getIntegrationTests() { return Set.of( + EdgeDiagnosticsTestCase.class, DomainImpl.TestCase.class, DomainPropertyImpl.TestCase.class, ExpDataTableImpl.TestCase.class, diff --git a/experiment/src/org/labkey/experiment/api/EdgeDiagnosticsTestCase.java b/experiment/src/org/labkey/experiment/api/EdgeDiagnosticsTestCase.java new file mode 100644 index 00000000000..e7ebaa2d14f --- /dev/null +++ b/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 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 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 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> 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)); + } +} diff --git a/experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java b/experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java index bf51266dc3f..ac0a3b9dbcd 100644 --- a/experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java +++ b/experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java @@ -991,7 +991,7 @@ public List getExpMaterials(Container container, User user, Col return getExpMaterials(filter); } - public List getExpMaterialsByObjectId(ContainerFilter containerFilter, Collection objectIds) + public List getExpMaterialsByObjectId(ContainerFilter containerFilter, Collection objectIds) { if (objectIds.isEmpty()) return emptyList(); @@ -1871,7 +1871,7 @@ public List getExpDatas(ExpDataClass dataClass) return datas.stream().map(ExpDataImpl::new).collect(toList()); } - public List getExpDatasByObjectId(ContainerFilter containerFilter, Collection objectIds) + public List getExpDatasByObjectId(ContainerFilter containerFilter, Collection objectIds) { SimpleFilter filter = new SimpleFilter(); filter.addInClause(FieldKey.fromParts("ObjectId"), objectIds); @@ -5711,7 +5711,7 @@ public List getRunsUsingDataIds(List ids) return ExpRunImpl.fromRuns(new SqlSelector(getExpSchema(), sql).getArrayList(ExperimentRun.class)); } - public List getRunsByObjectId(ContainerFilter containerFilter, Collection objectIds) + public List getRunsByObjectId(ContainerFilter containerFilter, Collection 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 protAppRecords) { for (ProtocolAppRecord rec : protAppRecords) diff --git a/experiment/src/org/labkey/experiment/controllers/exp/ExperimentController.java b/experiment/src/org/labkey/experiment/controllers/exp/ExperimentController.java index 27f2e4eaf57..9a188ba7177 100644 --- a/experiment/src/org/labkey/experiment/controllers/exp/ExperimentController.java +++ b/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()).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> 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()).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 { - List cycleObjectIds = null; + List cycleObjectIds = null; @Override public void validateCommand(Object target, Errors errors) @@ -8217,14 +8221,8 @@ public ModelAndView getView(Object o, boolean reshow, BindException errors) if (null == cycleObjectIds) return new HtmlView(HtmlString.of("No cycles found")); - Map 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 map = resolveCycleObjects(cf, cycleObjectIds); ExperimentUrls urls = ExperimentUrls.get(); return new HtmlView( @@ -8232,14 +8230,16 @@ public ModelAndView getView(Object o, boolean reshow, BindException errors) 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()).detectCycleInDirectedGraph(edges); - - var set = new LinkedHashSet(); - 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 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()).detectCycleInDirectedGraph(edges); + if (cyclesEdges.isEmpty()) + return null; + var set = new LinkedHashSet(); + cyclesEdges.forEach(e -> { set.add(e.first); set.add(e.second); }); + return set.stream().toList(); } + + public static Map resolveCycleObjects(ContainerFilter cf, List ids) + { + Map 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)