Fix and test CycleCheckAction and CheckEdgesAction#7697
Conversation
| // 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())) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
labkey-jeckels
changed the title
…add EdgeDiagnosticsTestCase CycleCheckAction and CheckEdgesAction read fromObjectId/toObjectId from exp.Edge using JDBC getInt() (Integer), but ExpObject.getObjectId() returns Long. This caused a silent failure in CycleCheckAction.getView() where LongHashMap<Long> lookups with Integer keys always returned null, so cycle-involved objects were never resolved to their names or links. Fix: use getLong() throughout both actions and carry Long consistently through all type parameters. Also fixed a null-check bug where cycleObjectIds was always set to an empty list (not null) when no cycles were found, making the "No cycles found" branch unreachable. Refactored the core logic of both actions into public static helpers (detectCycleEdges, detectCycleObjectIds, resolveCycleObjects) to enable direct testing. Changed getExpMaterialsByObjectId, getExpDatasByObjectId, and getRunsByObjectId in ExperimentServiceImpl from Collection<Integer> to Collection<Long> to match. Added EdgeDiagnosticsTestCase (org.labkey.experiment.api) with integration tests that insert real synthetic A↔B cycle edges and verify both actions correctly detect and resolve cycle-involved objects by Long key — the assertion that would fail with the pre-fix Integer-keyed code. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2 participants
Rationale
These actions never got converted to
longwhen we revamped the experiment schema RowIds, but they may still be useful for specialized troubleshootingChanges
longTasks 📍
Manual Testing