From e3c83182ed1778266ded5bb8e4be01e03a73025c Mon Sep 17 00:00:00 2001 From: Marcus Hirt Date: Sun, 13 Sep 2026 14:03:33 +0000 Subject: [PATCH] 8608: Severity filtering is wrong for IGNORE results Reviewed-by: aharmaz --- .../jmc/flightrecorder/ui/RuleManager.java | 8 +- .../ui/common/AbstractDataPage.java | 4 +- .../ui/common/DataPageToolkit.java | 9 +- .../jmc/flightrecorder/rules/Severity.java | 63 ++++++- .../rules/report/JfrRulesReport.java | 4 +- .../html/internal/RulesHtmlToolkit.java | 6 +- .../rules/util/RulesToolkit.java | 2 +- .../test/rules/jdk/TestRulesWithJfr.java | 6 +- .../flightrecorder/rules/SeverityTest.java | 157 ++++++++++++++++++ 9 files changed, 235 insertions(+), 24 deletions(-) create mode 100644 core/tests/org.openjdk.jmc.flightrecorder.rules.test/src/main/java/org/openjdk/jmc/flightrecorder/rules/SeverityTest.java diff --git a/application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/RuleManager.java b/application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/RuleManager.java index 33e10eba53..1e278bb95d 100644 --- a/application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/RuleManager.java +++ b/application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/RuleManager.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2018, 2025, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2018, 2026, Oracle and/or its affiliates. All rights reserved. * * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * @@ -111,7 +111,7 @@ private boolean shouldEvaluate(IRule rule) throws InterruptedException { if (dependencyType != null) { while (true) { if (evaluatedRules.containsKey(dependencyType)) { - if (evaluatedRules.get(dependencyType).compareTo(dependency.severity()) < 0) { + if (!evaluatedRules.get(dependencyType).isAtLeast(dependency.severity())) { return false; } return true; @@ -304,8 +304,8 @@ public Collection getRules(String ... topics) { } public Severity getMaxSeverity(String ... topics) { - return getResults(topics).parallelStream().map(IResult::getSeverity).max(Comparator.naturalOrder()) - .orElse(Severity.NA); + return getResults(topics).parallelStream().map(IResult::getSeverity) + .max(Comparator.comparingDouble(Severity::getLimit)).orElse(Severity.NA); } public Collection getUnmappedResults() { diff --git a/application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/common/AbstractDataPage.java b/application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/common/AbstractDataPage.java index c34a527910..788c248045 100644 --- a/application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/common/AbstractDataPage.java +++ b/application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/common/AbstractDataPage.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2018, 2025, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2018, 2026, Oracle and/or its affiliates. All rights reserved. * * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * @@ -144,7 +144,7 @@ public AbstractDataPage(IPageDefinition definition, StreamModel model, IPageCont private long getNumberOfInterestingResults() { return editor.getRuleManager().getResults(definition.getTopics()).parallelStream() - .filter(r -> r.getSeverity().compareTo(Severity.INFO) >= 0).count(); + .filter(r -> r.getSeverity().isAtLeast(Severity.INFO)).count(); } private String getRulesStatistics() { diff --git a/application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/common/DataPageToolkit.java b/application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/common/DataPageToolkit.java index 8ca9839b11..d8f0a28bdb 100644 --- a/application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/common/DataPageToolkit.java +++ b/application/org.openjdk.jmc.flightrecorder.ui/src/main/java/org/openjdk/jmc/flightrecorder/ui/common/DataPageToolkit.java @@ -823,12 +823,11 @@ public static class ShowResultAction extends Action { maxSeverity = pageContainer.getRuleManager().getMaxSeverity(topics); for (String topic : topics) { Consumer listener = result -> { - Severity severity = result.getSeverity(); - if (severity.compareTo(maxSeverity) > 0) { - maxSeverity = severity; + Severity updatedMaxSeverity = Severity.nextMax(maxSeverity, result.getSeverity(), + () -> pageContainer.getRuleManager().getMaxSeverity(topics)); + if (updatedMaxSeverity != maxSeverity) { + maxSeverity = updatedMaxSeverity; setImageDescriptor(getResultIcon(maxSeverity)); - } else if (severity.compareTo(maxSeverity) < 0) { // severity could be less than previous max - maxSeverity = pageContainer.getRuleManager().getMaxSeverity(topics); } setToolTipText(tooltip.get()); }; diff --git a/core/org.openjdk.jmc.flightrecorder.rules/src/main/java/org/openjdk/jmc/flightrecorder/rules/Severity.java b/core/org.openjdk.jmc.flightrecorder.rules/src/main/java/org/openjdk/jmc/flightrecorder/rules/Severity.java index 31c0a63ddc..bfb52c54d0 100644 --- a/core/org.openjdk.jmc.flightrecorder.rules/src/main/java/org/openjdk/jmc/flightrecorder/rules/Severity.java +++ b/core/org.openjdk.jmc.flightrecorder.rules/src/main/java/org/openjdk/jmc/flightrecorder/rules/Severity.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2018, 2025, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2018, 2026, Oracle and/or its affiliates. All rights reserved. * * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * @@ -32,10 +32,26 @@ */ package org.openjdk.jmc.flightrecorder.rules; +import java.util.function.Supplier; + import org.openjdk.jmc.flightrecorder.rules.messages.internal.Messages; +/** + * The severity of a rule result. + *

+ * The constants are declared in ascending order of severity, so that the natural ordering of this + * enum agrees with the ordering of the severity scores returned by {@link #getLimit()}. Keep it + * that way when adding constants: code that compares severities relies on it. Prefer + * {@link #isAtLeast(Severity)} over {@link #compareTo(Object)} when filtering, since it states the + * intent and compares the scores directly. + */ public enum Severity { + /** + * Results with this severity score should not be presented at all, because the rule does not + * apply in a way that is worth reporting. + */ + IGNORE(-3, Messages.getString(Messages.Severity_IGNORE)), /** * Results with this severity score are not applicable to the recording, but it should be * possible to view them so the user can see which results have not been possible to evaluate. @@ -52,9 +68,7 @@ public enum Severity { /** * Results with this severity score should be presented as warnings. */ - WARNING(75, Messages.getString(Messages.Severity_WARNING)), - - IGNORE(-3, Messages.getString(Messages.Severity_IGNORE)); + WARNING(75, Messages.getString(Messages.Severity_WARNING)); private final double score; private final String localizedName; @@ -72,6 +86,47 @@ public double getLimit() { return score; } + /** + * Checks whether this severity is at least as severe as the given one, comparing the severity + * scores. Use this rather than comparing severities directly when applying a minimum severity + * threshold. + * + * @param other + * the severity to compare against + * @return true if this severity's score is greater than or equal to the other's + */ + public boolean isAtLeast(Severity other) { + return score >= other.score; + } + + /** + * Determines the new maximum severity after a result with {@code resultSeverity} arrives, given + * the {@code currentMax} tracked so far. A result more severe than the current max simply + * becomes the new max. A result less severe than the current max means the previous max may no + * longer be in effect (e.g. its rule was re-evaluated with a lower severity), so the true max + * is recomputed from scratch via {@code recompute}. + * + * @param currentMax + * the maximum severity tracked so far + * @param resultSeverity + * the severity of the result that just arrived + * @param recompute + * supplies the true maximum severity across all results, used when + * {@code resultSeverity} is less severe than {@code currentMax} + * @return the new maximum severity + */ + public static Severity nextMax(Severity currentMax, Severity resultSeverity, Supplier recompute) { + if (resultSeverity.getLimit() > currentMax.getLimit()) { + return resultSeverity; + } else if (resultSeverity.getLimit() < currentMax.getLimit()) { + return recompute.get(); + } + return currentMax; + } + + /** + * The constants in descending order of severity score, as required by {@link #get(double)}. + */ private static final Severity[] VALUES = {WARNING, INFO, OK, NA, IGNORE}; public static Severity get(double score) { diff --git a/core/org.openjdk.jmc.flightrecorder.rules/src/main/java/org/openjdk/jmc/flightrecorder/rules/report/JfrRulesReport.java b/core/org.openjdk.jmc.flightrecorder.rules/src/main/java/org/openjdk/jmc/flightrecorder/rules/report/JfrRulesReport.java index 6f69eb3d36..bea0473465 100644 --- a/core/org.openjdk.jmc.flightrecorder.rules/src/main/java/org/openjdk/jmc/flightrecorder/rules/report/JfrRulesReport.java +++ b/core/org.openjdk.jmc.flightrecorder.rules/src/main/java/org/openjdk/jmc/flightrecorder/rules/report/JfrRulesReport.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2018, 2025, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2018, 2026, Oracle and/or its affiliates. All rights reserved. * * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * @@ -265,7 +265,7 @@ public int compare(Entry o1, Entry o2) { continue; } - if (result != null && result.getSeverity().compareTo(minSeverity) >= 0) { + if (result != null && result.getSeverity().isAtLeast(minSeverity)) { Element ruleNode = createRuleNode(parent, reportNode, result.getRule()); ruleNode.appendChild(createValueNode(parent.getOwnerDocument(), "severity", //$NON-NLS-1$ diff --git a/core/org.openjdk.jmc.flightrecorder.rules/src/main/java/org/openjdk/jmc/flightrecorder/rules/report/html/internal/RulesHtmlToolkit.java b/core/org.openjdk.jmc.flightrecorder.rules/src/main/java/org/openjdk/jmc/flightrecorder/rules/report/html/internal/RulesHtmlToolkit.java index 969a7cdbb8..ed4a9a5d60 100644 --- a/core/org.openjdk.jmc.flightrecorder.rules/src/main/java/org/openjdk/jmc/flightrecorder/rules/report/html/internal/RulesHtmlToolkit.java +++ b/core/org.openjdk.jmc.flightrecorder.rules/src/main/java/org/openjdk/jmc/flightrecorder/rules/report/html/internal/RulesHtmlToolkit.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2018, 2025, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2018, 2026, Oracle and/or its affiliates. All rights reserved. * * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * @@ -278,7 +278,7 @@ public int compare(IResult r1, IResult r2) { @Override public int compare(IResult r1, IResult r2) { - return r1.getSeverity().compareTo(r2.getSeverity()); + return Double.compare(r1.getSeverity().getLimit(), r2.getSeverity().getLimit()); } }; @@ -302,7 +302,7 @@ public static String generateSinglePageHtml(Collection results, boolean html.append("

"); //$NON-NLS-1$ } else { for (IResult result : resultList) { - boolean expand = result.getSeverity().compareTo(Severity.INFO) >= 0; + boolean expand = result.getSeverity().isAtLeast(Severity.INFO); html.append(createRuleHtml(result, expand, 0)); } } diff --git a/core/org.openjdk.jmc.flightrecorder.rules/src/main/java/org/openjdk/jmc/flightrecorder/rules/util/RulesToolkit.java b/core/org.openjdk.jmc.flightrecorder.rules/src/main/java/org/openjdk/jmc/flightrecorder/rules/util/RulesToolkit.java index 0185bdf2d9..b74001b72e 100644 --- a/core/org.openjdk.jmc.flightrecorder.rules/src/main/java/org/openjdk/jmc/flightrecorder/rules/util/RulesToolkit.java +++ b/core/org.openjdk.jmc.flightrecorder.rules/src/main/java/org/openjdk/jmc/flightrecorder/rules/util/RulesToolkit.java @@ -1423,7 +1423,7 @@ private static String getRuleDependencyName(IRule rule) { private static boolean shouldEvaluate(IRule rule, IResult depResult) { DependsOn dependency = rule.getClass().getAnnotation(DependsOn.class); if (dependency != null) { - if (depResult.getSeverity().compareTo(dependency.severity()) < 0) { + if (!depResult.getSeverity().isAtLeast(dependency.severity())) { return false; } } diff --git a/core/tests/org.openjdk.jmc.flightrecorder.rules.jdk.test/src/main/java/org/openjdk/jmc/flightrecorder/test/rules/jdk/TestRulesWithJfr.java b/core/tests/org.openjdk.jmc.flightrecorder.rules.jdk.test/src/main/java/org/openjdk/jmc/flightrecorder/test/rules/jdk/TestRulesWithJfr.java index 4355dfaf65..8eda435c5e 100644 --- a/core/tests/org.openjdk.jmc.flightrecorder.rules.jdk.test/src/main/java/org/openjdk/jmc/flightrecorder/test/rules/jdk/TestRulesWithJfr.java +++ b/core/tests/org.openjdk.jmc.flightrecorder.rules.jdk.test/src/main/java/org/openjdk/jmc/flightrecorder/test/rules/jdk/TestRulesWithJfr.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2018, 2025, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2018, 2026, Oracle and/or its affiliates. All rights reserved. * * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * @@ -257,7 +257,7 @@ private static boolean shouldEvaluate(Map, Severity> eval if (dependencyType != null) { while (true) { if (evaluatedRules.containsKey(dependencyType)) { - if (evaluatedRules.get(dependencyType).compareTo(dependency.severity()) < 0) { + if (!evaluatedRules.get(dependencyType).isAtLeast(dependency.severity())) { return false; } return true; @@ -289,7 +289,7 @@ private static Report generateReport(IOResource jfr, boolean verbose, Severity m } evaluatedRules.put(rule.getClass(), result.getSeverity()); rp.addResults(result); - if (minSeverity == null || result.getSeverity().compareTo(minSeverity) >= 0) { + if (minSeverity == null || result.getSeverity().isAtLeast(minSeverity)) { ItemSet itemSet = null; IItemQuery itemQuery = result.getResult(TypedResult.ITEM_QUERY); if (verbose && itemQuery != null && !itemQuery.getAttributes().isEmpty()) { diff --git a/core/tests/org.openjdk.jmc.flightrecorder.rules.test/src/main/java/org/openjdk/jmc/flightrecorder/rules/SeverityTest.java b/core/tests/org.openjdk.jmc.flightrecorder.rules.test/src/main/java/org/openjdk/jmc/flightrecorder/rules/SeverityTest.java new file mode 100644 index 0000000000..f6006a2928 --- /dev/null +++ b/core/tests/org.openjdk.jmc.flightrecorder.rules.test/src/main/java/org/openjdk/jmc/flightrecorder/rules/SeverityTest.java @@ -0,0 +1,157 @@ +/* + * Copyright (c) 2026, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2026, Datadog, Inc. All rights reserved. + * + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. + * + * The contents of this file are subject to the terms of either the Universal Permissive License + * v 1.0 as shown at https://oss.oracle.com/licenses/upl + * + * or the following license: + * + * Redistribution and use in source and binary forms, with or without modification, are permitted + * provided that the following conditions are met: + * + * 1. Redistributions of source code must retain the above copyright notice, this list of conditions + * and the following disclaimer. + * + * 2. Redistributions in binary form must reproduce the above copyright notice, this list of + * conditions and the following disclaimer in the documentation and/or other materials provided with + * the distribution. + * + * 3. Neither the name of the copyright holder nor the names of its contributors may be used to + * endorse or promote products derived from this software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS OR + * IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND + * FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR + * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, + * WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY + * WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + */ +package org.openjdk.jmc.flightrecorder.rules; + +import org.junit.Assert; +import org.junit.Test; + +public class SeverityTest { + + /** + * The severities in ascending order of severity. This is the order the rest of these tests, and + * the severity filtering throughout the code base, are written against. + */ + private static final Severity[] ASCENDING = {Severity.IGNORE, Severity.NA, Severity.OK, Severity.INFO, + Severity.WARNING}; + + /** + * The declaration order of the enum must agree with the severity scores, so that the natural + * ordering of the enum is also the severity ordering. IGNORE used to be declared last, which + * made compareTo rank it as the most severe value and let ignored results through every + * severity filter. + */ + @Test + public void declarationOrderMatchesScoreOrder() { + Assert.assertArrayEquals("Severity constants are no longer declared in ascending severity order", ASCENDING, + Severity.values()); + for (int i = 1; i < ASCENDING.length; i++) { + Assert.assertTrue(ASCENDING[i - 1] + " should score lower than " + ASCENDING[i], + ASCENDING[i - 1].getLimit() < ASCENDING[i].getLimit()); + } + } + + @Test + public void ignoreIsTheLeastSevere() { + for (Severity severity : Severity.values()) { + if (severity != Severity.IGNORE) { + Assert.assertFalse("IGNORE must not satisfy a minimum of " + severity, + Severity.IGNORE.isAtLeast(severity)); + Assert.assertTrue(severity + " must satisfy a minimum of IGNORE", severity.isAtLeast(Severity.IGNORE)); + } + } + } + + @Test + public void isAtLeastMatchesScoreOrder() { + for (int i = 0; i < ASCENDING.length; i++) { + for (int j = 0; j < ASCENDING.length; j++) { + Severity severity = ASCENDING[i]; + Severity minimum = ASCENDING[j]; + Assert.assertEquals(severity + ".isAtLeast(" + minimum + ")", i >= j, severity.isAtLeast(minimum)); + } + } + } + + @Test + public void isAtLeastIsReflexive() { + for (Severity severity : Severity.values()) { + Assert.assertTrue(severity + " should be at least itself", severity.isAtLeast(severity)); + } + } + + @Test + public void getMapsScoresToTheHighestMatchingSeverity() { + Assert.assertEquals(Severity.WARNING, Severity.get(100)); + Assert.assertEquals(Severity.WARNING, Severity.get(75)); + Assert.assertEquals(Severity.INFO, Severity.get(74)); + Assert.assertEquals(Severity.INFO, Severity.get(25)); + Assert.assertEquals(Severity.OK, Severity.get(24)); + Assert.assertEquals(Severity.OK, Severity.get(0)); + Assert.assertEquals(Severity.NA, Severity.get(-1)); + Assert.assertEquals(Severity.IGNORE, Severity.get(-3)); + } + + /** + * Every constant must be reachable from {@link Severity#get(double)}, which iterates a separate + * array that has to be kept in descending score order. + */ + @Test + public void getReturnsEverySeverity() { + for (Severity severity : Severity.values()) { + Assert.assertEquals("Severity.get should return " + severity + " for its own score", severity, + Severity.get(severity.getLimit())); + } + } + + /** + * Regression tests for + * {@link Severity#nextMax(Severity, Severity, java.util.function.Supplier)}, which drives the + * icon shown by rule result actions in the UI. A prior version of that logic recomputed the max + * severity when a result dropped below the current max, but never told the caller to refresh + * the icon, leaving a stale (too severe) icon displayed. + */ + @Test + public void moreSevereResultBecomesTheNewMax() { + Severity result = Severity.nextMax(Severity.OK, Severity.WARNING, () -> { + throw new AssertionError("recompute should not be needed when severity increases"); + }); + Assert.assertEquals(Severity.WARNING, result); + } + + @Test + public void lessSevereResultTriggersRecompute() { + Severity result = Severity.nextMax(Severity.WARNING, Severity.OK, () -> Severity.INFO); + Assert.assertEquals("the recomputed value must be used so the icon can be refreshed", Severity.INFO, result); + } + + @Test + public void sameSeverityKeepsCurrentMaxWithoutRecomputing() { + Severity result = Severity.nextMax(Severity.INFO, Severity.INFO, () -> { + throw new AssertionError("recompute should not be needed when severity is unchanged"); + }); + Assert.assertEquals(Severity.INFO, result); + } + + /** + * Pins the bug: when severity drops, the caller must be told the max changed (by getting back a + * different reference) so it knows to refresh the icon, even if the recomputed max happens to + * differ from the reported result's own severity. + */ + @Test + public void recomputedMaxCanDifferFromTheReportedResult() { + Severity result = Severity.nextMax(Severity.WARNING, Severity.IGNORE, () -> Severity.OK); + Assert.assertNotSame(Severity.IGNORE, result); + Assert.assertSame(Severity.OK, result); + } +}