Skip to content

Remove unused actions: CycleCheckAction and CheckEdgesAction #7697

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
labkey-jeckels wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Remove unused actions: CycleCheckAction and CheckEdgesAction #7697

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
139 changes: 5 additions & 134 deletions experiment/src/org/labkey/experiment/controllers/exp/ExperimentController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,6 @@
import org.labkey.api.data.ConvertHelper;
import org.labkey.api.data.DataRegion;
import org.labkey.api.data.DataRegionSelection;
import org.labkey.api.data.DbSchema;
import org.labkey.api.data.DbScope;
import org.labkey.api.data.DisplayColumn;
import org.labkey.api.data.ExcelWriter;
Expand All @@ -90,7 +89,6 @@
import org.labkey.api.data.SimpleDisplayColumn;
import org.labkey.api.data.SimpleFilter;
import org.labkey.api.data.Sort;
import org.labkey.api.data.SqlSelector;
import org.labkey.api.data.TSVJSONWriter;
import org.labkey.api.data.TSVWriter;
import org.labkey.api.data.TableInfo;
Expand Down Expand Up @@ -201,7 +199,6 @@
import org.labkey.api.security.permissions.ReadPermission;
import org.labkey.api.security.permissions.SampleWorkflowDeletePermission;
import org.labkey.api.security.permissions.SiteAdminPermission;
import org.labkey.api.security.permissions.TroubleshooterPermission;
import org.labkey.api.security.permissions.UpdatePermission;
import org.labkey.api.settings.AppProps;
import org.labkey.api.settings.ConceptURIProperties;
Expand All @@ -218,7 +215,6 @@
import org.labkey.api.util.ExceptionUtil;
import org.labkey.api.util.FileStream;
import org.labkey.api.util.FileUtil;
import org.labkey.api.util.HtmlString;
import org.labkey.api.util.HtmlStringBuilder;
import org.labkey.api.util.ImageUtil;
import org.labkey.api.util.JSoupUtil;
Expand Down Expand Up @@ -292,7 +288,6 @@
import org.labkey.experiment.api.ExpSampleTypeImpl;
import org.labkey.experiment.api.Experiment;
import org.labkey.experiment.api.ExperimentServiceImpl;
import org.labkey.experiment.api.GraphAlgorithms;
import org.labkey.experiment.api.ProtocolActionStepDetail;
import org.labkey.experiment.api.SampleTypeServiceImpl;
import org.labkey.experiment.api.SampleTypeUpdateServiceDI;
Expand Down Expand Up @@ -356,18 +351,15 @@
import static org.labkey.api.util.DOM.Attribute.name;
import static org.labkey.api.util.DOM.Attribute.size;
import static org.labkey.api.util.DOM.Attribute.src;
import static org.labkey.api.util.DOM.Attribute.target;
import static org.labkey.api.util.DOM.Attribute.type;
import static org.labkey.api.util.DOM.Attribute.value;
import static org.labkey.api.util.DOM.Attribute.width;
import static org.labkey.api.util.DOM.DIV;
import static org.labkey.api.util.DOM.IMG;
import static org.labkey.api.util.DOM.INPUT;
import static org.labkey.api.util.DOM.LI;
import static org.labkey.api.util.DOM.TABLE;
import static org.labkey.api.util.DOM.TD;
import static org.labkey.api.util.DOM.TR;
import static org.labkey.api.util.DOM.UL;
import static org.labkey.api.util.DOM.at;
import static org.labkey.api.util.DOM.cl;
import static org.labkey.experiment.ExpDataIterators.setContainerFilterForImport;
Expand Down Expand Up @@ -797,7 +789,7 @@ private Pair<ExperimentRunListView, JspView<?>> createViews(ExpObjectForm form,
ExperimentRunType selectedType = ExperimentRunType.getSelectedFilter(types, getViewContext().getRequest().getParameter("experimentRunFilter"));

ChooseExperimentTypeBean bean = new ChooseExperimentTypeBean(types, selectedType, getViewContext().getActionURL().clone(), protocols);
JspView chooserView = new JspView<>("/org/labkey/experiment/experimentRunQueryHeader.jsp", bean, errors);
JspView<?> chooserView = new JspView<>("/org/labkey/experiment/experimentRunQueryHeader.jsp", bean, errors);

ExperimentRunListView runListView = ExperimentRunListView.createView(getViewContext(), bean.getSelectedFilter(), true);
runListView.getRunTable().setExperiment(_experiment);
Expand Down Expand Up @@ -2517,8 +2509,10 @@ protected ModelAndView getDataView(DataForm form, BindException errors) throws I
try
{
// Try to write the exception back to the caller if we haven't already flushed the buffer
ApiJsonWriter writer = new ApiJsonWriter(getViewContext().getResponse());
writer.writeResponse(e);
try(ApiJsonWriter writer = new ApiJsonWriter(getViewContext().getResponse()))
Copy link
Copy Markdown
Contributor

@labkey-nicka labkey-nicka May 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've certainly used this action with clients in the past. I agree it is not an often used endpoint but it can still be useful. Perhaps we fix it to work with longs instead?

Copy link
Copy Markdown
Contributor Author

@labkey-jeckels labkey-jeckels May 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've certainly used this action with clients in the past. I agree it is not an often used endpoint but it can still be useful. Perhaps we fix it to work with longs instead?

Sure, it should be a straightforward fix. I propose that we add at least one test case for each action if we want to retain them. Can you add them or at least outline a scenario where they were each useful?

Copy link
Copy Markdown
Contributor

@labkey-nicka labkey-nicka May 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our experiment lineage tooling/queries expect to process a directed acyclic graph which is why these utility endpoints were introduced. We do try to prevent cycles (e.g., here and here) from being formed but do not guarantee that condition due to it being computationally expensive.

They've proven useful for two clients where they wanted to identify where they had cycles as it almost always represented a bad data relationship (someone chose the wrong parent, etc.). One client then took action from these results and edited their relationships until they were either mostly or entirely free of cycles. I'm not sure what actions the other client took.

{
writer.writeResponse(e);
}
}
catch (IllegalStateException ise)
{
Expand Down Expand Up @@ -7373,43 +7367,6 @@ public Object execute(Object o, BindException errors) throws Exception
}
}

@Marshal(Marshaller.Jackson)
@RequiresPermission(AdminPermission.class)
public static class CheckEdgesAction extends ReadOnlyApiAction<Object>
{
@Override
public Object execute(Object o, BindException errors) throws Exception
{
List<Object[]> result;
DbSchema schema = ExperimentService.get().getSchema();
TableInfo edgeTable = schema.getTable("Edge");

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());
}
else
{
var edges = new SqlSelector(ExperimentService.get().getSchema(), "SELECT fromLsid, toLsid FROM exp.Edge")
.resultSetStream()
.map(r -> { try { return new Pair<>(r.getString(1), r.getString(2)); } catch (SQLException x) { throw new RuntimeException(x); } })
.collect(toList());
var cycles = (new GraphAlgorithms<String>()).detectCycleInDirectedGraph(edges);
result = cycles.stream().map(e -> new String[]{e.first, e.second}).collect(toList());
}

JSONObject ret = new JSONObject();
ret.put("result", result);
ret.put("success", true);
return ret;
}
}

@RequiresPermission(UpdatePermission.class)
public static class UpdateMaterialQueryRowAction extends UserSchemaAction
{
Expand Down Expand Up @@ -8190,92 +8147,6 @@ public ModelAndView getView(Object o, BindException errors) throws SQLException
}
}

/* Also see API CheckEdgesAction */
@RequiresPermission(TroubleshooterPermission.class)
public static class CycleCheckAction extends FormViewAction<Object>
{
List<Integer> cycleObjectIds = null;

@Override
public void validateCommand(Object target, Errors errors)
{

}

@Override
public ModelAndView getView(Object o, boolean reshow, BindException errors)
{
if (!reshow)
{
return new HtmlView(
DIV("This operation can use a lot of memory.",
LK.FORM(at(method,"POST"),
PageFlowUtil.button("Continue").submit(true)))
);
}

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));

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));
}))
)
);
}

@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();
return false;
}

@Override
public URLHelper getSuccessURL(Object o)
{
return null;
}

@Override
public void addNavTrail(NavTree root)
{

}
}

@RequiresPermission(AdminPermission.class)
public static class MissingFilesCheckAction extends ReadOnlyApiAction<Object>
{
Expand Down