From 07d5822c1f24582174840f6cb5d3ec2dcdc46054 Mon Sep 17 00:00:00 2001 From: Vagisha Sharma Date: Sun, 14 Jun 2026 16:24:20 -0700 Subject: [PATCH 1/5] Fixed testresults date-parsing, training-stats, run-delete, and failure-chart bugs - TestResultsController.parseDate: parse dates strictly so 13/45/2026 is rejected instead of rolling over to a valid date; give each call its own SimpleDateFormat instead of one shared static one (SimpleDateFormat is not thread-safe). - TrainRunAction / DeleteRunAction: recompute a user's training stats when a run is removed from their training set or deleted, and remove the stats row when no training runs remain (recomputeUserData helper). - DeleteRunAction: delete the run's handleleaks and trainruns rows before deleting the run itself, so deleting a run that had handle leaks or was a training run no longer fails with a foreign-key (FK) error. - failureDetail.jsp: set the chart's date-axis bounds only when dates exist, avoiding a JavaScript error on an empty date range. - TestResultsTest: added a delete-with-children test; tightened the date and training-data tests. Co-Authored-By: Claude --- .../testresults/TestResultsController.java | 121 +++++++++++++---- .../labkey/testresults/view/failureDetail.jsp | 21 ++- .../tests/testresults/TestResultsTest.java | 122 ++++++++++++++++-- 3 files changed, 221 insertions(+), 43 deletions(-) diff --git a/testresults/src/org/labkey/testresults/TestResultsController.java b/testresults/src/org/labkey/testresults/TestResultsController.java index aeebef33..10b62910 100644 --- a/testresults/src/org/labkey/testresults/TestResultsController.java +++ b/testresults/src/org/labkey/testresults/TestResultsController.java @@ -139,7 +139,18 @@ public class TestResultsController extends SpringActionController { private static final Logger _log = LogManager.getLogger(TestResultsController.class); - private static final SimpleDateFormat MDYFormat = new SimpleDateFormat("MM/dd/yyyy"); + private static final String MDY_PATTERN = "MM/dd/yyyy"; + + // SimpleDateFormat is not thread-safe and is lenient by default, so we never share a + // single static instance. Each caller gets a fresh, strict formatter: strict parsing + // makes nonsense dates like 13/45/2026 throw ParseException instead of silently rolling + // over to a valid-but-wrong date (month 13 -> +1 year, day 45 -> spill into next month). + private static SimpleDateFormat mdyFormat() + { + SimpleDateFormat format = new SimpleDateFormat(MDY_PATTERN); + format.setLenient(false); + return format; + } private static final String KEY_SUCCESS = "Success"; @@ -172,7 +183,63 @@ public static String getTabClass(String tabName, String activeTab) private static Date parseDate(String dateStr) throws ParseException { - return StringUtils.isNotBlank(dateStr) ? MDYFormat.parse(dateStr) : null; + return StringUtils.isNotBlank(dateStr) ? mdyFormat().parse(dateStr) : null; + } + + /** + * Recomputes one user's training statistics (mean/stddev of tests-run and memory) in the + * given container from their remaining training runs. If the user has no training runs + * left, deletes their UserData row instead. + * + * The delete branch matters: the recompute groups by (userid, container), so an empty + * join produces zero output rows, ON CONFLICT never fires, and a stale UserData row would + * otherwise survive with its old mean/stddev values (leaving the user listed on the + * training-data page as if they still had training data). Callers must invoke this inside + * an open transaction. + */ + private static void recomputeUserData(DbScope scope, int userid, Container container) + { + SQLFragment remainingFragment = new SQLFragment(); + remainingFragment.append( + "SELECT COUNT(*) FROM " + TestResultsSchema.getTableInfoTestRuns() + " " + + "JOIN " + TestResultsSchema.getTableInfoTrain() + + " ON " + TestResultsSchema.getTableInfoTestRuns() + ".id = " + TestResultsSchema.getTableInfoTrain() + ".runid " + + "WHERE userid = ? AND container = ?"); + remainingFragment.add(userid); + remainingFragment.add(container.getEntityId()); + long remainingTrainRuns = new SqlSelector(TestResultsSchema.getSchema(), remainingFragment).getObject(Long.class); + + if (remainingTrainRuns == 0) + { + SQLFragment deleteFragment = new SQLFragment(); + deleteFragment.append("DELETE FROM " + TestResultsSchema.getTableInfoUserData() + " WHERE userid = ? AND container = ?"); + deleteFragment.add(userid); + deleteFragment.add(container.getEntityId()); + new SqlExecutor(scope).execute(deleteFragment); + } + else + { + SQLFragment sqlFragmentUpdate = new SQLFragment(); + sqlFragmentUpdate.append( + "INSERT INTO " + TestResultsSchema.getTableInfoUserData() + " " + + " (userid, container, meantestsrun, meanmemory, stddevtestsrun, stddevmemory) " + + "SELECT ?, ?, avg(passedtests), avg(averagemem), stddev_pop(passedtests), stddev_pop(averagemem) " + + "FROM " + TestResultsSchema.getTableInfoTestRuns() + " " + + "JOIN " + TestResultsSchema.getTableInfoTrain() + + " ON " + TestResultsSchema.getTableInfoTestRuns() + ".id = " + TestResultsSchema.getTableInfoTrain() + ".runid " + + "WHERE userid = ? AND container = ? " + + "GROUP BY userid, container " + + "ON CONFLICT(userid, container) DO UPDATE SET " + + " meantestsrun = excluded.meantestsrun, " + + " meanmemory = excluded.meanmemory, " + + " stddevtestsrun = excluded.stddevtestsrun, " + + " stddevmemory = excluded.stddevmemory"); + sqlFragmentUpdate.add(userid); + sqlFragmentUpdate.add(container.getEntityId()); + sqlFragmentUpdate.add(userid); + sqlFragmentUpdate.add(container.getEntityId()); + new SqlExecutor(scope).execute(sqlFragmentUpdate); + } } // Form class for RetrainAllAction @@ -537,27 +604,8 @@ else if ((train && !foundRuns.isEmpty()) || (!train && foundRuns.isEmpty())) fragment.add(runId); new SqlExecutor(scope).execute(fragment); } - // update user table calculations - SQLFragment sqlFragmentUpdate = new SQLFragment(); - sqlFragmentUpdate.append( - "INSERT INTO " + TestResultsSchema.getTableInfoUserData() + " " + - " (userid, container, meantestsrun, meanmemory, stddevtestsrun, stddevmemory) " + - "SELECT ?, ?, avg(passedtests), avg(averagemem), stddev_pop(passedtests), stddev_pop(averagemem) " + - "FROM " + TestResultsSchema.getTableInfoTestRuns() + " " + - "JOIN " + TestResultsSchema.getTableInfoTrain() + - " ON " + TestResultsSchema.getTableInfoTestRuns() + ".id = " + TestResultsSchema.getTableInfoTrain() + ".runid " + - "WHERE userid = ? AND container = ? " + - "GROUP BY userid, container " + - "ON CONFLICT(userid, container) DO UPDATE SET " + - " meantestsrun = excluded.meantestsrun, " + - " meanmemory = excluded.meanmemory, " + - " stddevtestsrun = excluded.stddevtestsrun, " + - " stddevmemory = excluded.stddevmemory"); - sqlFragmentUpdate.add(details[0].getUserid()); - sqlFragmentUpdate.add(getContainer().getEntityId()); - sqlFragmentUpdate.add(details[0].getUserid()); - sqlFragmentUpdate.add(getContainer().getEntityId()); - new SqlExecutor(scope).execute(sqlFragmentUpdate); + // Refresh this user's training statistics from their remaining training runs. + recomputeUserData(scope, details[0].getUserid(), getContainer()); transaction.commit(); } return new ApiSimpleResponse(KEY_SUCCESS, true); @@ -976,14 +1024,35 @@ public Object execute(RunIdForm form, BindException errors) } int rowId = form.getRunId(); + // Look up the run's owner before deleting so we can refresh that user's training + // stats afterward: the run may have been part of their training set, so their + // baseline can change (or disappear) once it is gone. + RunDetail[] runs = new TableSelector(TestResultsSchema.getTableInfoTestRuns(), + new SimpleFilter(FieldKey.fromParts("id"), rowId), null).getArray(RunDetail.class); + // Child tables keyed by testrunid -> testruns(id). SimpleFilter filter = new SimpleFilter(); filter.addCondition(FieldKey.fromParts("testrunid"), rowId); - try (DbScope.Transaction transaction = TestResultsSchema.getSchema().getScope().ensureTransaction()) { + // trainruns keys the run via its "runid" column, not "testrunid". + SimpleFilter trainFilter = new SimpleFilter(); + trainFilter.addCondition(FieldKey.fromParts("runid"), rowId); + DbScope scope = TestResultsSchema.getSchema().getScope(); + try (DbScope.Transaction transaction = scope.ensureTransaction()) { + // Delete every child row that references this run before the parent, or the + // testruns delete fails on a foreign key constraint. handleleaks and trainruns + // were previously missed: deleting a run that had handle-leak records or was in + // the training set failed (e.g. fk_memoryleaks_testruns on table handleleaks). Table.delete(TestResultsSchema.getTableInfoTestFails(), filter); Table.delete(TestResultsSchema.getTableInfoTestPasses(), filter); Table.delete(TestResultsSchema.getTableInfoMemoryLeaks(), filter); + Table.delete(TestResultsSchema.getTableInfoHandleLeaks(), filter); Table.delete(TestResultsSchema.getTableInfoHangs(), filter); + Table.delete(TestResultsSchema.getTableInfoTrain(), trainFilter); Table.delete(TestResultsSchema.getTableInfoTestRuns(), rowId); // delete run last because of foreign key + // If the deleted run belonged to a user, refresh their training stats now that + // it (and any trainruns row for it) is gone, so a removed training run does not + // leave a stale UserData row behind. + if (runs.length > 0) + recomputeUserData(scope, runs[0].getUserid(), getContainer()); transaction.commit(); } catch (Exception x) { response.put(KEY_SUCCESS, false); @@ -1950,7 +2019,7 @@ private static void ParseAndStoreXML(String xml, Container c) throws Exception timestampDay = addDays(timestampDay, 1); } lastHour = hour; - String originalDate = MDYFormat.format(timestampDay); + String originalDate = mdyFormat().format(timestampDay); try { timestamp = new SimpleDateFormat("MM/dd/yyyy HH:mm").parse(originalDate + " " + ts); } catch (IllegalArgumentException e) { @@ -2002,7 +2071,7 @@ private static void ParseAndStoreXML(String xml, Container c) throws Exception timestampDay = addDays(timestampDay, 1); } lastHour = hour; - String originalDate = MDYFormat.format(timestampDay); + String originalDate = mdyFormat().format(timestampDay); try { timestamp = new SimpleDateFormat("MM/dd/yyyy HH:mm").parse(originalDate + " " + ts); } catch (IllegalArgumentException e) { diff --git a/testresults/src/org/labkey/testresults/view/failureDetail.jsp b/testresults/src/org/labkey/testresults/view/failureDetail.jsp index 3d675525..273f8f35 100644 --- a/testresults/src/org/labkey/testresults/view/failureDetail.jsp +++ b/testresults/src/org/labkey/testresults/view/failureDetail.jsp @@ -266,6 +266,19 @@ $(document).ready(function() { const problemData = <%=json(problemData, 0)%>; // Generate date chart. + // Only set explicit timeseries min/max when there are dates to plot. With an empty + // dates array, dates[0] and dates[length - 1] are undefined and c3/d3 throws while + // building the x-axis scale (e.g. when the active date range excludes all sample runs). + const chartDates = problemData.graphData.dates; + const xAxis = { + type: 'timeseries', + localtime: false, + tick: { fit: true, format: '%m/%d' } + }; + if (chartDates.length > 0) { + xAxis.min = chartDates[0]; + xAxis.max = chartDates[chartDates.length - 1]; + } let dateChart = c3.generate({ bindto: '#failGraph', data: { @@ -277,13 +290,7 @@ $(document).ready(function() { bar: { width: { ratio: 0.3 } }, subchart: { show: false, size: { height: 20 } }, axis: { - x: { - min: problemData.graphData.dates[0], - max: problemData.graphData.dates[problemData.graphData.dates.length - 1], - type: 'timeseries', - localtime: false, - tick: { fit: true, format: '%m/%d' } - }, + x: xAxis, // y: { tick: { values: %=graphYTicks.getJavaScriptFragment(0)% } } } }); diff --git a/testresults/test/src/org/labkey/test/tests/testresults/TestResultsTest.java b/testresults/test/src/org/labkey/test/tests/testresults/TestResultsTest.java index 28b57a81..07f513e3 100644 --- a/testresults/test/src/org/labkey/test/tests/testresults/TestResultsTest.java +++ b/testresults/test/src/org/labkey/test/tests/testresults/TestResultsTest.java @@ -27,6 +27,7 @@ import org.junit.Test; import org.junit.experimental.categories.Category; import org.labkey.remoteapi.Connection; +import org.labkey.remoteapi.query.Filter; import org.labkey.remoteapi.query.SelectRowsCommand; import org.labkey.remoteapi.query.SelectRowsResponse; import org.labkey.remoteapi.query.Sort; @@ -48,6 +49,8 @@ import java.time.format.DateTimeFormatter; import java.util.List; import java.util.Map; +import java.util.Set; +import java.util.stream.Collectors; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; @@ -455,19 +458,18 @@ public void testTrainingDataPage() toggleTrainingSet(); assertTextPresent("Add to training set"); - // After removal: the Remove link and the run's data row are gone, and - // the stats row now shows "RunCount:0". TEST-PC-1's section is still - // rendered in the training table though, because of a known bug in - // TrainRunAction: when the user's last training run is removed, the - // the stale UserData row (non-zero meanmemory / meantestsrun) is - // never cleared. trainingdata.jsp:158 only moves users to the - // "No Training Data --" list when both fields are 0, so the section - // stays. See TODO-LK-20260403_testresults-bugs.md. + // After removing the user's last training run, TrainRunAction deletes their + // UserData row, so TEST-PC-1 no longer has any training data: its section + // disappears from and the user moves to the + // "No Training Data --" list (it now behaves like TEST-PC-2). Previously a + // stale UserData row left a lingering RunCount:0 section here. clickAndWait(Locator.linkWithText("Training Data")); assertElementNotPresent(removeLink); assertElementNotPresent(trainingTable.containing("2026-01-16 06:00")); - assertElementPresent(statsRow.containing("RunCount:0")); - assertTextPresent(COMPUTER_NAME_2, "No Training Data --"); + assertElementNotPresent(trainingTable.descendant( + Locator.tagWithId("tr", "user-anchor-" + COMPUTER_NAME_1))); + assertElementNotPresent(statsRow); + assertTextPresent(COMPUTER_NAME_1, COMPUTER_NAME_2, "No Training Data --"); } @Test @@ -629,6 +631,13 @@ public void testInvalidDateParameters() beginAt(WebTestHelper.buildRelativeUrl("testresults", PROJECT_NAME, "showFailures", Map.of("end", "03-24-2026"))); assertTextPresent("Invalid date format: 03-24-2026 (expected MM/dd/yyyy)"); + + // Strict parsing: an out-of-range but MM/dd/yyyy-shaped date like 13/45/2026 used to + // silently roll over to a valid date (02/14/2027) because the shared SimpleDateFormat + // was lenient. It must now be rejected like any other invalid date. + beginAt(WebTestHelper.buildRelativeUrl("testresults", PROJECT_NAME, "begin", + Map.of("end", "13/45/2026"))); + assertTextPresent("Invalid date format: 13/45/2026 (expected MM/dd/yyyy)"); } @Test @@ -655,6 +664,38 @@ public void testDeleteRun() assertTextNotPresent("TestDisposableOne"); } + @Test + public void testDeleteRunWithChildRecordsRecomputesTraining() + { + // Post a fresh run that has both handle-leak and memory-leak child rows, then add it + // to the training set. Deleting it must (1) succeed despite the handleleaks and + // trainruns child rows that previously caused a foreign key violation, and (2) refresh + // the user's training stats so no stale UserData row is left behind. + int baselineUserData = userDataRowCount(); + + int runId = postAndGetNewRunId("testresults/pc1-run-0117-leaks.xml", COMPUTER_NAME_1); + assertTrue("Posted run should have handle-leak child rows", childRowCount("handleleaks", runId) > 0); + assertTrue("Posted run should have memory-leak child rows", childRowCount("memoryleaks", runId) > 0); + + JSONObject trainResp = postApi("trainRun", Map.of("runId", String.valueOf(runId), "train", "true")); + assertTrue("trainRun should succeed: " + trainResp, trainResp.optBoolean("Success", false)); + assertTrue("A UserData row should exist after adding a training run", userDataRowCount() >= 1); + + // Without the fix this returned Success=false with a foreign key violation on + // handleleaks (and would also fail for the trainruns row). + JSONObject deleteResp = postApi("deleteRun", Map.of("runId", String.valueOf(runId))); + assertTrue("deleteRun should succeed without a foreign key violation: " + deleteResp, + deleteResp.optBoolean("Success", false)); + + // Child rows are cleaned up, and the training stats are refreshed: this was the run's + // only training run, so its UserData row is removed rather than left stale. + assertEquals("handleleaks child rows should be deleted", 0, childRowCount("handleleaks", runId)); + assertEquals("memoryleaks child rows should be deleted", 0, childRowCount("memoryleaks", runId)); + assertEquals("testpasses child rows should be deleted", 0, childRowCount("testpasses", runId)); + assertEquals("Deleting the only training run should remove its UserData row", + baselineUserData, userDataRowCount()); + } + // ------------------------------------------------------------------------- // Helpers // ------------------------------------------------------------------------- @@ -880,6 +921,67 @@ private JSONObject postApi(String action, Map params) } } + /** + * Posts a sample XML run and returns the id of the run it created, identified as the + * single new testruns row for the given computer (compared against the runs present + * before the post). + */ + private int postAndGetNewRunId(String sampleDataRelativePath, String computerName) + { + Set before = runIdsForComputer(computerName); + postSampleXml(sampleDataRelativePath); + Set after = runIdsForComputer(computerName); + after.removeAll(before); + assertEquals("Expected exactly one new run for " + computerName, 1, after.size()); + return after.iterator().next(); + } + + /** + * Returns the set of testruns ids posted by the given computer in this container. + */ + private Set runIdsForComputer(String computerName) + { + return queryRuns().stream() + .filter(r -> computerName.equals(r.get("userid/username"))) + .map(r -> (Integer) r.get("id")) + .collect(Collectors.toSet()); + } + + /** + * Counts rows in a testresults child table that reference the given run via testrunid. + */ + private int childRowCount(String table, int runId) + { + try + { + Connection connection = WebTestHelper.getRemoteApiConnection(); + SelectRowsCommand cmd = new SelectRowsCommand("testresults", table); + cmd.addFilter(new Filter("testrunid", runId)); + return cmd.execute(connection, PROJECT_NAME).getRows().size(); + } + catch (Exception e) + { + throw new RuntimeException("Failed to count rows in " + table + " for run " + runId, e); + } + } + + /** + * Counts UserData (training stats) rows in this container. + */ + private int userDataRowCount() + { + try + { + Connection connection = WebTestHelper.getRemoteApiConnection(); + SelectRowsCommand cmd = new SelectRowsCommand("testresults", "userdata"); + return cmd.execute(connection, PROJECT_NAME).getRows().size(); + } + catch (Exception e) + { + throw new RuntimeException("Failed to count userdata rows", e); + } + } + // ------------------------------------------------------------------------- // Infrastructure // ------------------------------------------------------------------------- From 5e70afe2472b9fda362e7d1692a028639ea64026 Mon Sep 17 00:00:00 2001 From: Vagisha Sharma Date: Wed, 17 Jun 2026 16:24:12 -0700 Subject: [PATCH 2/5] Added TestResultsTest coverage for the recomputeUserData update branch * testRemoveTrainingRunRecomputesStatsWhenRunsRemain verifies that removing one of a user's two training runs keeps the UserData row and recomputes meanmemory from the remaining run (prior tests only hit the delete branch) * Added selectRows/getUserId/runAverageMem/userDataMeanMemory helpers Co-Authored-By: Claude --- .../tests/testresults/TestResultsTest.java | 102 ++++++++++++++++++ 1 file changed, 102 insertions(+) diff --git a/testresults/test/src/org/labkey/test/tests/testresults/TestResultsTest.java b/testresults/test/src/org/labkey/test/tests/testresults/TestResultsTest.java index 07f513e3..80b2a3c0 100644 --- a/testresults/test/src/org/labkey/test/tests/testresults/TestResultsTest.java +++ b/testresults/test/src/org/labkey/test/tests/testresults/TestResultsTest.java @@ -696,6 +696,45 @@ public void testDeleteRunWithChildRecordsRecomputesTraining() baselineUserData, userDataRowCount()); } + @Test + public void testRemoveTrainingRunRecomputesStatsWhenRunsRemain() + { + // Covers the recomputeUserData "update" branch: when a user still has training runs + // after one is removed, their UserData row must survive (not be deleted) and its stats + // must be recomputed from the remaining runs. The other tests only exercise the + // "delete" branch (removing a user's only/last training run). + int userId = getUserId(COMPUTER_NAME_1); + int baselineUserData = userDataRowCount(); + double cleanMem = runAverageMem(_cleanRunId); + double leakMem = runAverageMem(_leakRunId); + + try + { + // Add two TEST-PC-1 runs to the training set; both share one UserData row whose + // mean memory is the average of the two runs. + assertTrainRun(_cleanRunId, "true"); + assertTrainRun(_leakRunId, "true"); + assertEquals("Both training runs should share one UserData row", + baselineUserData + 1, userDataRowCount()); + assertEquals("Mean memory should be the average of both training runs", + (cleanMem + leakMem) / 2, userDataMeanMemory(userId), 1.0); + + // Remove one run: the row must remain (update branch, not delete) and its mean + // memory must be recomputed from the single remaining run. + assertTrainRun(_leakRunId, "false"); + assertEquals("Removing one of two training runs must not delete the UserData row", + baselineUserData + 1, userDataRowCount()); + assertEquals("Mean memory should be recomputed from the remaining run", + cleanMem, userDataMeanMemory(userId), 1.0); + } + finally + { + // Leave the training set empty for other tests (no-op if already removed). + postApi("trainRun", Map.of("runId", String.valueOf(_cleanRunId), "train", "false")); + postApi("trainRun", Map.of("runId", String.valueOf(_leakRunId), "train", "false")); + } + } + // ------------------------------------------------------------------------- // Helpers // ------------------------------------------------------------------------- @@ -982,6 +1021,69 @@ private int userDataRowCount() } } + /** + * Posts trainRun for the given run and asserts it succeeded. {@code train} is "true" to add + * to the training set or "false" to remove. + */ + private void assertTrainRun(int runId, String train) + { + JSONObject resp = postApi("trainRun", Map.of("runId", String.valueOf(runId), "train", train)); + assertTrue("trainRun(" + runId + ", " + train + ") should succeed: " + resp, + resp.optBoolean("Success", false)); + } + + /** + * Returns the testresults.user id for the given computer name. + */ + private int getUserId(String computerName) + { + List> rows = selectRows("user", + new Filter("username", computerName), "id"); + assertEquals("Expected exactly one user row for " + computerName, 1, rows.size()); + return ((Number) rows.get(0).get("id")).intValue(); + } + + /** + * Returns the stored average managed memory for a run. + */ + private double runAverageMem(int runId) + { + List> rows = selectRows("testruns", + new Filter("id", runId), "averagemem"); + assertEquals("Expected exactly one testruns row for run " + runId, 1, rows.size()); + return ((Number) rows.get(0).get("averagemem")).doubleValue(); + } + + /** + * Returns the recomputed mean memory from the single UserData row for the given user. + */ + private double userDataMeanMemory(int userId) + { + List> rows = selectRows("userdata", + new Filter("userid", userId), "meanmemory"); + assertEquals("Expected exactly one userdata row for user " + userId, 1, rows.size()); + return ((Number) rows.get(0).get("meanmemory")).doubleValue(); + } + + /** + * Runs a filtered SelectRows against a testresults query, returning the selected columns. + */ + private List> selectRows(String queryName, Filter filter, String... columns) + { + try + { + Connection connection = WebTestHelper.getRemoteApiConnection(); + SelectRowsCommand cmd = new SelectRowsCommand("testresults", queryName); + cmd.addFilter(filter); + cmd.setColumns(List.of(columns)); + return cmd.execute(connection, PROJECT_NAME).getRows(); + } + catch (Exception e) + { + throw new RuntimeException("Failed to query " + queryName, e); + } + } + // ------------------------------------------------------------------------- // Infrastructure // ------------------------------------------------------------------------- From 26e984f36399f8bf90bd07cf14719d7416b439e4 Mon Sep 17 00:00:00 2001 From: Vagisha Sharma Date: Wed, 17 Jun 2026 21:31:40 -0700 Subject: [PATCH 3/5] * TestResultsController.TrainRunAction: check that the run exists before the recompute on the 'force' path too, so a force request with a bad runId returns "run does not exist" instead of throwing on an empty details[0] * TestResultsTest.testRemoveTrainingRunRecomputesStatsWhenRunsRemain: also assert stddevmemory (|a-b|/2 with two training runs, 0 with one remaining) Co-Authored-By: Claude --- .../testresults/TestResultsController.java | 13 ++++++------ .../tests/testresults/TestResultsTest.java | 21 +++++++++++++++++-- 2 files changed, 25 insertions(+), 9 deletions(-) diff --git a/testresults/src/org/labkey/testresults/TestResultsController.java b/testresults/src/org/labkey/testresults/TestResultsController.java index 65ca420f..1ae25bef 100644 --- a/testresults/src/org/labkey/testresults/TestResultsController.java +++ b/testresults/src/org/labkey/testresults/TestResultsController.java @@ -599,13 +599,12 @@ public Object execute(TrainRunForm form, BindException errors) SimpleFilter filter = new SimpleFilter(); filter.addCondition(FieldKey.fromParts("id"), runId); RunDetail[] details = new TableSelector(TestResultsSchema.getTableInfoTestRuns(), filter, null).getArray(RunDetail.class); - if (!force) - { - if (details.length == 0) - return new ApiSimpleResponse(Map.of(KEY_SUCCESS, false, "error", "run does not exist: " + runId)); - else if ((train && !foundRuns.isEmpty()) || (!train && foundRuns.isEmpty())) - return new ApiSimpleResponse(Map.of(KEY_SUCCESS, false, "error", "no action necessary")); - } + // The run must exist even on the force path: recomputeUserData below dereferences + // details[0] to get the userid, so an empty result would otherwise throw. + if (details.length == 0) + return new ApiSimpleResponse(Map.of(KEY_SUCCESS, false, "error", "run does not exist: " + runId)); + if (!force && ((train && !foundRuns.isEmpty()) || (!train && foundRuns.isEmpty()))) + return new ApiSimpleResponse(Map.of(KEY_SUCCESS, false, "error", "no action necessary")); DbScope scope = TestResultsSchema.getSchema().getScope(); try (DbScope.Transaction transaction = scope.ensureTransaction()) { diff --git a/testresults/test/src/org/labkey/test/tests/testresults/TestResultsTest.java b/testresults/test/src/org/labkey/test/tests/testresults/TestResultsTest.java index f53e49d9..dc7cfd1d 100644 --- a/testresults/test/src/org/labkey/test/tests/testresults/TestResultsTest.java +++ b/testresults/test/src/org/labkey/test/tests/testresults/TestResultsTest.java @@ -729,14 +729,19 @@ public void testRemoveTrainingRunRecomputesStatsWhenRunsRemain() baselineUserData + 1, userDataRowCount()); assertEquals("Mean memory should be the average of both training runs", (cleanMem + leakMem) / 2, userDataMeanMemory(userId), 1.0); + // Population stddev of two values {a, b} is |a - b| / 2. + assertEquals("Stddev memory should reflect both training runs", + Math.abs(cleanMem - leakMem) / 2, userDataStdDevMemory(userId), 1.0); - // Remove one run: the row must remain (update branch, not delete) and its mean - // memory must be recomputed from the single remaining run. + // Remove one run: the row must remain (update branch, not delete) and its stats + // must be recomputed from the single remaining run. assertTrainRun(_leakRunId, "false"); assertEquals("Removing one of two training runs must not delete the UserData row", baselineUserData + 1, userDataRowCount()); assertEquals("Mean memory should be recomputed from the remaining run", cleanMem, userDataMeanMemory(userId), 1.0); + assertEquals("Stddev memory of a single remaining run should be zero", + 0.0, userDataStdDevMemory(userId), 1.0); } finally { @@ -1186,6 +1191,18 @@ private double userDataMeanMemory(int userId) return ((Number) rows.get(0).get("meanmemory")).doubleValue(); } + /** + * Returns the recomputed population stddev of memory from the single UserData row for the + * given user. + */ + private double userDataStdDevMemory(int userId) + { + List> rows = selectRows("userdata", + new Filter("userid", userId), "stddevmemory"); + assertEquals("Expected exactly one userdata row for user " + userId, 1, rows.size()); + return ((Number) rows.get(0).get("stddevmemory")).doubleValue(); + } + /** * Runs a filtered SelectRows against a testresults query, returning the selected columns. */ From f56aa321082360210384487bd268872c6e1b2ebb Mon Sep 17 00:00:00 2001 From: Vagisha Sharma Date: Sat, 20 Jun 2026 15:13:13 -0700 Subject: [PATCH 4/5] Hardened trainRun force path and tightened recompute test assertions * TrainRunAction: check the run exists before recompute on the force path too, so a bad runId returns "run does not exist" instead of throwing (with a test) * testRemoveTrainingRunRecomputesStatsWhenRunsRemain: compare memory stats with an exact epsilon instead of a loose tolerance Co-Authored-By: Claude --- .../tests/testresults/TestResultsTest.java | 20 +++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/testresults/test/src/org/labkey/test/tests/testresults/TestResultsTest.java b/testresults/test/src/org/labkey/test/tests/testresults/TestResultsTest.java index dc7cfd1d..dcc6d0e0 100644 --- a/testresults/test/src/org/labkey/test/tests/testresults/TestResultsTest.java +++ b/testresults/test/src/org/labkey/test/tests/testresults/TestResultsTest.java @@ -67,6 +67,10 @@ public class TestResultsTest extends BaseWebDriverTest implements PostgresOnlyTe static final String COMPUTER_NAME_1 = "TEST-PC-1"; static final String COMPUTER_NAME_2 = "TEST-PC-2"; + // Training stats are exact (avg/stddev of integer averagemem), so this only absorbs the + // floating round-trip through the query API. It is not a real tolerance. + private static final double EPSILON = 1e-6; + private static final Locator SUBMIT_BUTTON = Locator.css("input[type='submit'][value='Submit']"); // XPath for the problems matrix table (header cell contains "Fail: | Leak: | Hang:") @@ -600,6 +604,14 @@ public void testApiErrorResponses() assertFalse(missingRun.optBoolean("Success", true)); assertEquals("run does not exist: 999999", missingRun.optString("error")); + // TrainRunAction: force path also requires the run to exist. The existence check runs + // before the recompute regardless of force, so a bad runId reports not-found instead of + // throwing on an empty lookup. + JSONObject forceMissingRun = postApi("trainRun", + Map.of("runId", "999999", "train", "force")); + assertFalse(forceMissingRun.optBoolean("Success", true)); + assertEquals("run does not exist: 999999", forceMissingRun.optString("error")); + // SetUserActive: missing userId JSONObject noUserId = postApi("setUserActive", Map.of("active", "true")); assertEquals("userId is required", noUserId.optString("Message")); @@ -728,10 +740,10 @@ public void testRemoveTrainingRunRecomputesStatsWhenRunsRemain() assertEquals("Both training runs should share one UserData row", baselineUserData + 1, userDataRowCount()); assertEquals("Mean memory should be the average of both training runs", - (cleanMem + leakMem) / 2, userDataMeanMemory(userId), 1.0); + (cleanMem + leakMem) / 2, userDataMeanMemory(userId), EPSILON); // Population stddev of two values {a, b} is |a - b| / 2. assertEquals("Stddev memory should reflect both training runs", - Math.abs(cleanMem - leakMem) / 2, userDataStdDevMemory(userId), 1.0); + Math.abs(cleanMem - leakMem) / 2, userDataStdDevMemory(userId), EPSILON); // Remove one run: the row must remain (update branch, not delete) and its stats // must be recomputed from the single remaining run. @@ -739,9 +751,9 @@ public void testRemoveTrainingRunRecomputesStatsWhenRunsRemain() assertEquals("Removing one of two training runs must not delete the UserData row", baselineUserData + 1, userDataRowCount()); assertEquals("Mean memory should be recomputed from the remaining run", - cleanMem, userDataMeanMemory(userId), 1.0); + cleanMem, userDataMeanMemory(userId), EPSILON); assertEquals("Stddev memory of a single remaining run should be zero", - 0.0, userDataStdDevMemory(userId), 1.0); + 0.0, userDataStdDevMemory(userId), EPSILON); } finally { From 23681936bb0d648be45f03d6d1677d29dbc7c299 Mon Sep 17 00:00:00 2001 From: Vagisha Sharma Date: Sat, 20 Jun 2026 18:33:54 -0700 Subject: [PATCH 5/5] Scoped testresults run-by-id actions to the current folder * Added getRunInContainer helper; trainRun, deleteRun, flagRun, viewLog, viewXml and the flagged-runs list now reject or omit runs from other folders, since run ids are global and the raw testruns table is not container-filtered (a user could otherwise act on another folder's run by guessing its id) * deleteRun/trainRun now recompute training stats for the verified in-folder run, removing the cross-folder recompute gap * testResultsTest.testRunAccessIsContainerScoped: cross-folder access test Co-Authored-By: Claude --- .../testresults/TestResultsController.java | 66 ++++++++++++------- .../tests/testresults/TestResultsTest.java | 63 ++++++++++++++++++ 2 files changed, 107 insertions(+), 22 deletions(-) diff --git a/testresults/src/org/labkey/testresults/TestResultsController.java b/testresults/src/org/labkey/testresults/TestResultsController.java index 1ae25bef..5b99149a 100644 --- a/testresults/src/org/labkey/testresults/TestResultsController.java +++ b/testresults/src/org/labkey/testresults/TestResultsController.java @@ -189,6 +189,19 @@ private static Date parseDate(String dateStr) throws ParseException return StringUtils.isNotBlank(dateStr) ? mdyFormat().parse(dateStr) : null; } + /** + * Loads the run with the given id only if it belongs to the given container, otherwise returns + * null. Run ids are global and the raw testruns table has no query-layer container filter, so + * actions that look a run up by id must scope by container or a user could read or modify a run + * in another folder by guessing its id. + */ + private static RunDetail getRunInContainer(int runId, Container container) + { + SimpleFilter filter = new SimpleFilter(FieldKey.fromParts("id"), runId); + filter.addCondition(FieldKey.fromParts("container"), container.getEntityId()); + return new TableSelector(TestResultsSchema.getTableInfoTestRuns(), filter, null).getObject(RunDetail.class); + } + /** * Recomputes one user's training statistics (mean/stddev of tests-run and memory) in the * given container from their remaining training runs. If the user has no training runs @@ -596,12 +609,11 @@ public Object execute(TrainRunForm form, BindException errors) SqlSelector sqlSelector = new SqlSelector(TestResultsSchema.getSchema(), sqlFragment); List foundRuns = new ArrayList<>(); sqlSelector.forEach(rs -> foundRuns.add(rs.getInt("runid"))); - SimpleFilter filter = new SimpleFilter(); - filter.addCondition(FieldKey.fromParts("id"), runId); - RunDetail[] details = new TableSelector(TestResultsSchema.getTableInfoTestRuns(), filter, null).getArray(RunDetail.class); - // The run must exist even on the force path: recomputeUserData below dereferences - // details[0] to get the userid, so an empty result would otherwise throw. - if (details.length == 0) + // The run must exist (in this folder) even on the force path: recomputeUserData below + // uses run.getUserid(), so a missing run would otherwise throw. Scoping by container + // also stops a run in another folder from being trained from here. + RunDetail run = getRunInContainer(runId, getContainer()); + if (run == null) return new ApiSimpleResponse(Map.of(KEY_SUCCESS, false, "error", "run does not exist: " + runId)); if (!force && ((train && !foundRuns.isEmpty()) || (!train && foundRuns.isEmpty()))) return new ApiSimpleResponse(Map.of(KEY_SUCCESS, false, "error", "no action necessary")); @@ -618,7 +630,7 @@ public Object execute(TrainRunForm form, BindException errors) new SqlExecutor(scope).execute(fragment); } // Refresh this user's training statistics from their remaining training runs. - recomputeUserData(scope, details[0].getUserid(), getContainer()); + recomputeUserData(scope, run.getUserid(), getContainer()); transaction.commit(); } return new ApiSimpleResponse(KEY_SUCCESS, true); @@ -1037,11 +1049,17 @@ public Object execute(RunIdForm form, BindException errors) } int rowId = form.getRunId(); - // Look up the run's owner before deleting so we can refresh that user's training - // stats afterward: the run may have been part of their training set, so their - // baseline can change (or disappear) once it is gone. - RunDetail[] runs = new TableSelector(TestResultsSchema.getTableInfoTestRuns(), - new SimpleFilter(FieldKey.fromParts("id"), rowId), null).getArray(RunDetail.class); + // Look up the run (scoped to this folder) before deleting, both to reject a run in + // another folder and to refresh the owner's training stats afterward: the run may + // have been part of their training set, so their baseline can change (or disappear) + // once it is gone. + RunDetail run = getRunInContainer(rowId, getContainer()); + if (run == null) + { + response.put(KEY_SUCCESS, false); + response.put("error", "run does not exist: " + rowId); + return response; + } // Child tables keyed by testrunid -> testruns(id). SimpleFilter filter = new SimpleFilter(); filter.addCondition(FieldKey.fromParts("testrunid"), rowId); @@ -1061,11 +1079,9 @@ public Object execute(RunIdForm form, BindException errors) Table.delete(TestResultsSchema.getTableInfoHangs(), filter); Table.delete(TestResultsSchema.getTableInfoTrain(), trainFilter); Table.delete(TestResultsSchema.getTableInfoTestRuns(), rowId); // delete run last because of foreign key - // If the deleted run belonged to a user, refresh their training stats now that - // it (and any trainruns row for it) is gone, so a removed training run does not - // leave a stale UserData row behind. - if (runs.length > 0) - recomputeUserData(scope, runs[0].getUserid(), getContainer()); + // Refresh the owner's training stats now that the run (and any trainruns row for + // it) is gone, so a removed training run does not leave a stale UserData row behind. + recomputeUserData(scope, run.getUserid(), getContainer()); transaction.commit(); } catch (Exception x) { response.put(KEY_SUCCESS, false); @@ -1107,10 +1123,9 @@ public Object execute(FlagRunForm form, BindException errors) int rowId = form.getRunId(); boolean flag = form.getFlag() != null ? form.getFlag() : false; - SimpleFilter filter = new SimpleFilter(); - filter.addCondition(FieldKey.fromParts("id"), rowId); try (DbScope.Transaction transaction = TestResultsSchema.getSchema().getScope().ensureTransaction()) { - RunDetail detail = new TableSelector(TestResultsSchema.getTableInfoTestRuns(), filter, null).getObject(RunDetail.class); + // Scoped to this folder so a run in another folder cannot be flagged from here. + RunDetail detail = getRunInContainer(rowId, getContainer()); if (detail == null) { response.put(KEY_SUCCESS, false); @@ -1164,8 +1179,11 @@ public static class ShowFlaggedAction extends SimpleViewAction @Override public ModelAndView getView(Object o, BindException errors) throws Exception { + // Scoped to this folder so the Flags page lists only flagged runs in the current folder, + // not flagged runs across every folder. SimpleFilter filter = new SimpleFilter(); filter.addCondition(FieldKey.fromParts("flagged"), true); + filter.addCondition(FieldKey.fromParts("container"), getContainer().getEntityId()); RunDetail[] details = new TableSelector(TestResultsSchema.getTableInfoTestRuns(), filter, null).getArray(RunDetail.class); JspView view = new JspView<>("/org/labkey/testresults/view/flagged.jsp", new TestsDataBean(details, new User[0])); view.setFrame(WebPartView.FrameType.PORTAL); @@ -1264,9 +1282,11 @@ public Object execute(RunIdForm form, BindException errors) if (form.getRunId() == null) return new ApiSimpleResponse("log", null); int runId = form.getRunId(); + // Scoped to this folder so a run's log in another folder cannot be read by id. SQLFragment sqlFragment = new SQLFragment(); - sqlFragment.append("SELECT log FROM testresults.testruns WHERE id = ?"); + sqlFragment.append("SELECT log FROM testresults.testruns WHERE id = ? AND container = ?"); sqlFragment.add(runId); + sqlFragment.add(getContainer().getEntityId()); List logs = new ArrayList<>(); SqlSelector sqlSelector = new SqlSelector(TestResultsSchema.getSchema(), sqlFragment); sqlSelector.forEach(rs -> logs.add(rs.getBytes("log"))); @@ -1285,9 +1305,11 @@ public Object execute(RunIdForm form, BindException errors) if (form.getRunId() == null) return new ApiSimpleResponse("xml", null); int runId = form.getRunId(); + // Scoped to this folder so a run's XML in another folder cannot be read by id. SQLFragment sqlFragment = new SQLFragment(); - sqlFragment.append("SELECT xml FROM testresults.testruns WHERE id = ?"); + sqlFragment.append("SELECT xml FROM testresults.testruns WHERE id = ? AND container = ?"); sqlFragment.add(runId); + sqlFragment.add(getContainer().getEntityId()); List xmls = new ArrayList<>(); SqlSelector sqlSelector = new SqlSelector(TestResultsSchema.getSchema(), sqlFragment); sqlSelector.forEach(rs -> xmls.add(rs.getBytes("xml"))); diff --git a/testresults/test/src/org/labkey/test/tests/testresults/TestResultsTest.java b/testresults/test/src/org/labkey/test/tests/testresults/TestResultsTest.java index dcc6d0e0..1b1c37b9 100644 --- a/testresults/test/src/org/labkey/test/tests/testresults/TestResultsTest.java +++ b/testresults/test/src/org/labkey/test/tests/testresults/TestResultsTest.java @@ -57,6 +57,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; @Category({External.class, MacCossLabModules.class}) @@ -763,6 +764,68 @@ public void testRemoveTrainingRunRecomputesStatsWhenRunsRemain() } } + @Test + public void testRunAccessIsContainerScoped() throws IOException, CommandException + { + // Run ids are global, so the actions that look a run up by id must reject a run that lives + // in another folder. Put a run in a subfolder, then from the PARENT folder confirm that + // every run-by-id action refuses it, and that the run is still reachable from its own folder. + final String subFolder = "CrossFolderAccessTest"; + final String subFolderPath = "/" + PROJECT_NAME + "/" + subFolder; + _containerHelper.createSubfolder(PROJECT_NAME, subFolder); + _containerHelper.enableModule(subFolderPath, "TestResults"); + postSampleXml("testresults/pc1-run-0116-failures.xml", subFolderPath); + + Connection conn = WebTestHelper.getRemoteApiConnection(); + SelectRowsCommand runsCmd = new SelectRowsCommand("testresults", "testruns"); + runsCmd.setColumns(List.of("id")); + SelectRowsResponse subRuns = runsCmd.execute(conn, subFolderPath); + assertEquals("Subfolder should have exactly 1 run", 1, subRuns.getRows().size()); + int subRunId = ((Number) subRuns.getRows().getFirst().get("id")).intValue(); + + // --- Negative: act on the subfolder's run by id from the PARENT folder (PROJECT_NAME) --- + + JSONObject del = postApi("deleteRun", Map.of("runId", String.valueOf(subRunId))); + assertFalse("Cross-folder deleteRun must fail: " + del, del.optBoolean("Success", true)); + assertEquals("run does not exist: " + subRunId, del.optString("error")); + + JSONObject train = postApi("trainRun", Map.of("runId", String.valueOf(subRunId), "train", "true")); + assertFalse("Cross-folder trainRun must fail: " + train, train.optBoolean("Success", true)); + assertEquals("run does not exist: " + subRunId, train.optString("error")); + + JSONObject flag = postApi("flagRun", Map.of("runId", String.valueOf(subRunId), "flag", "true")); + assertFalse("Cross-folder flagRun must fail: " + flag, flag.optBoolean("Success", true)); + assertEquals("run not found: " + subRunId, flag.optString("error")); + + assertNull("Cross-folder viewLog must not return content", + getApiString("viewLog", subRunId, "log")); + assertNull("Cross-folder viewXml must not return content", + getApiString("viewXml", subRunId, "xml")); + + // showRun in the parent folder shows the "enter run ID" prompt, not the subfolder run. + beginAt(WebTestHelper.buildRelativeUrl("testresults", PROJECT_NAME, "showRun", + Map.of("runId", String.valueOf(subRunId)))); + assertElementPresent(Locator.css("input[name='runId']")); + + // The run is untouched in its own folder (the cross-folder delete was a no-op there). + assertEquals("Subfolder run must be untouched", 1, + runsCmd.execute(conn, subFolderPath).getRows().size()); + + // --- Positive: the same run IS reachable from its own folder --- + // (deleteRun/trainRun/viewLog/viewXml in-folder are covered by the other tests, which all + // run in PROJECT_NAME and would fail if the container guard rejected same-folder access.) + JSONObject flagOwn = postApi("flagRun", + Map.of("runId", String.valueOf(subRunId), "flag", "true"), subFolderPath); + assertTrue("In-folder flagRun should succeed: " + flagOwn, flagOwn.optBoolean("Success", false)); + + // ShowFlaggedAction is folder-scoped: the flagged subfolder run shows on the subfolder's + // Flags page but not the parent's. + beginAt(WebTestHelper.buildRelativeUrl("testresults", subFolderPath, "showFlagged")); + assertElementPresent(Locator.tag("a").startsWith("id: " + subRunId + " /")); + beginAt(WebTestHelper.buildRelativeUrl("testresults", PROJECT_NAME, "showFlagged")); + assertElementNotPresent(Locator.tag("a").startsWith("id: " + subRunId + " /")); + } + /** * Verifies that child tables in the testresults schema are container-filtered * via a join to testruns. Creates a subfolder, posts a single run into it,