Fix numeric sorts in `_cat/nodes` (#106189)

Some of the columns in the `GET _cat/nodes` output are numeric, but
formatted in a specific way, so we represent them as strings today which
makes them sort incorrectly. This commit combines the string
representation with the original numeric value so these values sort
correctly.

Closes #48070

docs/changelog/106189.yaml

@@ -0,0 +1,6 @@
+pr: 106189
+summary: Fix numeric sorts in `_cat/nodes`
+area: CAT APIs
+type: bug
+issues:
+ - 48070

server/src/main/java/org/elasticsearch/rest/action/cat/RestNodesAction.java

@@ -57,7 +57,6 @@ import org.elasticsearch.script.ScriptStats;
 import org.elasticsearch.search.suggest.completion.CompletionStats;
 
 import java.util.List;
-import java.util.Locale;
 import java.util.concurrent.atomic.AtomicReference;
 
 import static org.elasticsearch.rest.RestRequest.Method.GET;
@@ -375,14 +374,14 @@ public class RestNodesAction extends AbstractCatAction {
             ByteSizeValue diskTotal = null;
             ByteSizeValue diskUsed = null;
             ByteSizeValue diskAvailable = null;
-            String diskUsedPercent = null;
+            RestTable.FormattedDouble diskUsedPercent = null;
             if (fsInfo != null) {
                 diskTotal = fsInfo.getTotal().getTotal();
                 diskAvailable = fsInfo.getTotal().getAvailable();
                 diskUsed = ByteSizeValue.ofBytes(diskTotal.getBytes() - diskAvailable.getBytes());
 
                 double diskUsedRatio = diskTotal.getBytes() == 0 ? 1.0 : (double) diskUsed.getBytes() / diskTotal.getBytes();
-                diskUsedPercent = String.format(Locale.ROOT, "%.2f", 100.0 * diskUsedRatio);
+                diskUsedPercent = RestTable.FormattedDouble.format2DecimalPlaces(100.0 * diskUsedRatio);
             }
             table.addCell(diskTotal);
             table.addCell(diskUsed);
@@ -408,17 +407,17 @@ public class RestNodesAction extends AbstractCatAction {
             table.addCell(
                 hasLoadAverage == false || osStats.getCpu().getLoadAverage()[0] == -1
                     ? null
-                    : String.format(Locale.ROOT, "%.2f", osStats.getCpu().getLoadAverage()[0])
+                    : RestTable.FormattedDouble.format2DecimalPlaces(osStats.getCpu().getLoadAverage()[0])
             );
             table.addCell(
                 hasLoadAverage == false || osStats.getCpu().getLoadAverage()[1] == -1
                     ? null
-                    : String.format(Locale.ROOT, "%.2f", osStats.getCpu().getLoadAverage()[1])
+                    : RestTable.FormattedDouble.format2DecimalPlaces(osStats.getCpu().getLoadAverage()[1])
             );
             table.addCell(
                 hasLoadAverage == false || osStats.getCpu().getLoadAverage()[2] == -1
                     ? null
-                    : String.format(Locale.ROOT, "%.2f", osStats.getCpu().getLoadAverage()[2])
+                    : RestTable.FormattedDouble.format2DecimalPlaces(osStats.getCpu().getLoadAverage()[2])
             );
             table.addCell(jvmStats == null ? null : jvmStats.getUptime());
 

server/src/main/java/org/elasticsearch/rest/action/cat/RestTable.java

@@ -496,4 +496,24 @@ public class RestTable {
             return reverse;
         }
     }
+
+    /**
+     * A formatted number, such that it sorts according to its numeric value but captures a specific string representation too
+     */
+    record FormattedDouble(String displayValue, double numericValue) implements Comparable<FormattedDouble> {
+
+        static FormattedDouble format2DecimalPlaces(double numericValue) {
+            return new FormattedDouble(Strings.format("%.2f", numericValue), numericValue);
+        }
+
+        @Override
+        public int compareTo(FormattedDouble other) {
+            return Double.compare(numericValue, other.numericValue);
+        }
+
+        @Override
+        public String toString() {
+            return displayValue;
+        }
+    }
 }

server/src/test/java/org/elasticsearch/rest/action/cat/RestNodesActionTests.java

@@ -9,17 +9,24 @@
 package org.elasticsearch.rest.action.cat;
 
 import org.elasticsearch.action.admin.cluster.node.info.NodesInfoResponse;
+import org.elasticsearch.action.admin.cluster.node.stats.NodeStats;
 import org.elasticsearch.action.admin.cluster.node.stats.NodesStatsResponse;
 import org.elasticsearch.action.admin.cluster.state.ClusterStateResponse;
 import org.elasticsearch.cluster.ClusterName;
 import org.elasticsearch.cluster.ClusterState;
+import org.elasticsearch.cluster.node.DiscoveryNode;
 import org.elasticsearch.cluster.node.DiscoveryNodeUtils;
 import org.elasticsearch.cluster.node.DiscoveryNodes;
+import org.elasticsearch.monitor.fs.FsInfo;
+import org.elasticsearch.monitor.os.OsStats;
 import org.elasticsearch.test.ESTestCase;
 import org.elasticsearch.test.rest.FakeRestRequest;
 import org.junit.Before;
 
+import java.util.ArrayList;
 import java.util.Collections;
+import java.util.List;
+import java.util.Map;
 
 import static java.util.Collections.emptySet;
 import static org.mockito.Mockito.mock;
@@ -48,4 +55,70 @@ public class RestNodesActionTests extends ESTestCase {
 
         action.buildTable(false, new FakeRestRequest(), clusterStateResponse, nodesInfoResponse, nodesStatsResponse);
     }
+
+    public void testFormattedNumericSort() {
+        final var clusterState = ClusterState.builder(ClusterName.DEFAULT)
+            .nodes(DiscoveryNodes.builder().add(DiscoveryNodeUtils.create("node-1")).add(DiscoveryNodeUtils.create("node-2")))
+            .build();
+
+        final var nowMillis = System.currentTimeMillis();
+        final var rowOrder = RestTable.getRowOrder(
+            action.buildTable(
+                false,
+                new FakeRestRequest(),
+                new ClusterStateResponse(clusterState.getClusterName(), clusterState, false),
+                new NodesInfoResponse(clusterState.getClusterName(), List.of(), List.of()),
+                new NodesStatsResponse(
+                    clusterState.getClusterName(),
+                    List.of(
+                        // sorting 10 vs 9 in all relevant columns, since these sort incorrectly as strings
+                        getTrickySortingNodeStats(nowMillis, clusterState.nodes().get("node-1"), 10),
+                        getTrickySortingNodeStats(nowMillis, clusterState.nodes().get("node-2"), 9)
+                    ),
+                    Collections.emptyList()
+                )
+            ),
+            new FakeRestRequest.Builder(xContentRegistry()).withParams(
+                Map.of("s", randomFrom("load_1m", "load_5m", "load_15m", "disk.used_percent"))
+            ).build()
+        );
+
+        final var nodesList = new ArrayList<DiscoveryNode>();
+        for (final var node : clusterState.nodes()) {
+            nodesList.add(node);
+        }
+
+        assertEquals("node-2", nodesList.get(rowOrder.get(0)).getId());
+        assertEquals("node-1", nodesList.get(rowOrder.get(1)).getId());
+    }
+
+    private static NodeStats getTrickySortingNodeStats(long nowMillis, DiscoveryNode node, int sortValue) {
+        return new NodeStats(
+            node,
+            nowMillis,
+            null,
+            new OsStats(
+                nowMillis,
+                new OsStats.Cpu((short) sortValue, new double[] { sortValue, sortValue, sortValue }),
+                new OsStats.Mem(0, 0, 0),
+                new OsStats.Swap(0, 0),
+                null
+            ),
+            null,
+            null,
+            null,
+            new FsInfo(nowMillis, null, new FsInfo.Path[] { new FsInfo.Path("/foo", "/foo", 100, 100 - sortValue, 100 - sortValue) }),
+            null,
+            null,
+            null,
+            null,
+            null,
+            null,
+            null,
+            null,
+            null,
+            null,
+            null
+        );
+    }
 }

server/src/test/java/org/elasticsearch/rest/action/cat/RestTableTests.java

@@ -259,6 +259,26 @@ public class RestTableTests extends ESTestCase {
         assertEquals(Arrays.asList(1, 0, 2), rowOrder);
     }
 
+    public void testFormattedDouble() {
+        Table table = new Table();
+        table.startHeaders();
+        table.addCell("number");
+        table.endHeaders();
+        List<Integer> comparisonList = Arrays.asList(10, 9, 11);
+        for (int i = 0; i < comparisonList.size(); i++) {
+            table.startRow();
+            table.addCell(RestTable.FormattedDouble.format2DecimalPlaces(comparisonList.get(i)));
+            table.endRow();
+        }
+        restRequest.params().put("s", "number");
+        List<Integer> rowOrder = RestTable.getRowOrder(table, restRequest);
+        assertEquals(Arrays.asList(1, 0, 2), rowOrder);
+
+        restRequest.params().put("s", "number:desc");
+        rowOrder = RestTable.getRowOrder(table, restRequest);
+        assertEquals(Arrays.asList(2, 0, 1), rowOrder);
+    }
+
     public void testPlainTextChunking() throws Exception {
         final var cells = randomArray(8, 8, String[]::new, () -> randomAlphaOfLengthBetween(1, 5));
         final var expectedRow = String.join(" ", cells) + "\n";