fix(thold): make the unit suffix conversions inverses of each other

tests/TestCase.php

@@ -31,6 +31,7 @@ protected function setUp(): void {
 
 		CactiStubs::reset();
 		$GLOBALS['rpn_error'] = false;
+		$_SESSION = [];
 	}
 
 	/**

tests/Unit/TholdUnitSuffixTest.php

@@ -0,0 +1,176 @@
+<?php
+/*
+ +-------------------------------------------------------------------------+
+ | Copyright (C) 2004-2026 The Cacti Group                                 |
+ |                                                                         |
+ | This program is free software; you can redistribute it and/or           |
+ | modify it under the terms of the GNU General Public License             |
+ | as published by the Free Software Foundation; either version 2          |
+ | of the License, or (at your option) any later version.                  |
+ +-------------------------------------------------------------------------+
+ | Cacti: The Complete RRDtool-based Graphing Solution                     |
+ +-------------------------------------------------------------------------+
+ | http://www.cacti.net/                                                   |
+ +-------------------------------------------------------------------------+
+*/
+
+/**
+ * The pair that moves a threshold bound between what an operator types and
+ * what is stored.
+ *
+ * They have to be inverses. A threshold is read out of the database, rendered
+ * into the form, and written back on every save, so any disagreement between
+ * them compounds each time the form is opened.
+ */
+final class TholdUnitSuffixTest extends TestCase {
+	/**
+	 * @return void
+	 */
+	public static function setUpBeforeClass(): void {
+		self::loadPluginSource('thold_functions.php');
+	}
+
+	/**
+	 * @return array<string, array{0: string, 1: float}>
+	 */
+	public static function suffixProvider() {
+		return [
+			'yocto' => ['5y', 5.0e-24],
+			'zepto' => ['5z', 5.0e-21],
+			'atto'  => ['5a', 5.0e-18],
+			'femto' => ['5f', 5.0e-15],
+			'pico'  => ['5p', 5.0e-12],
+			'nano'  => ['5n', 5.0e-9],
+			'micro' => ['5u', 5.0e-6],
+			'milli' => ['5m', 5.0e-3],
+			'kilo'  => ['5K', 5.0e3],
+			'mega'  => ['5M', 5.0e6],
+			'giga'  => ['5G', 5.0e9],
+			'tera'  => ['5T', 5.0e12],
+			'peta'  => ['5P', 5.0e15],
+			'exa'   => ['5E', 5.0e18],
+			'zetta' => ['5Z', 5.0e21],
+			'yotta' => ['5Y', 5.0e24],
+		];
+	}
+
+	/**
+	 * @dataProvider suffixProvider
+	 *
+	 * @param string $typed
+	 * @param float  $stored
+	 *
+	 * @return void
+	 */
+	public function testEachSuffixScalesByItsSiFactor($typed, $stored): void {
+		$this->assertEqualsWithDelta($stored, thold_display_to_raw($typed, 'thold_hi'), abs($stored) * 1.0e-9);
+	}
+
+	/**
+	 * @dataProvider suffixProvider
+	 *
+	 * @param string $typed
+	 * @param float  $stored
+	 *
+	 * @return void
+	 */
+	public function testEachStoredValueRendersWithItsSiSuffix($typed, $stored): void {
+		$this->assertSame($typed, thold_raw_to_display($stored));
+	}
+
+	/**
+	 * Opening a threshold and saving it again must not change it. This is the
+	 * failure that mattered: a value stored at 1e-12 rendered as 5f, which
+	 * parsed back as 1e-15, so every visit to the form divided it by a
+	 * thousand.
+	 *
+	 * @dataProvider suffixProvider
+	 *
+	 * @param string $typed
+	 * @param float  $stored
+	 *
+	 * @return void
+	 */
+	public function testAValueSurvivesBeingDisplayedAndReEntered($typed, $stored): void {
+		$round_tripped = thold_display_to_raw(thold_raw_to_display($stored), 'thold_hi');
+
+		$this->assertEqualsWithDelta($stored, $round_tripped, abs($stored) * 1.0e-9);
+	}
+
+	/**
+	 * @return void
+	 */
+	public function testAPlainNumberIsLeftAlone(): void {
+		$this->assertSame('42', thold_display_to_raw('42', 'thold_hi'));
+		$this->assertSame('42', thold_raw_to_display(42));
+	}
+
+	/**
+	 * @return void
+	 */
+	public function testZeroIsLeftAlone(): void {
+		$this->assertSame('0', thold_raw_to_display(0));
+	}
+
+	/**
+	 * @return void
+	 */
+	public function testNegativeValuesKeepTheirSign(): void {
+		$this->assertSame('-5K', thold_raw_to_display(-5000));
+		$this->assertEqualsWithDelta(-5000, thold_display_to_raw('-5K', 'thold_hi'), 1.0e-6);
+	}
+
+	/**
+	 * @return array<string, array{0: string}>
+	 */
+	public static function rejectedInputProvider() {
+		return [
+			'unknown suffix' => ['5x'],
+			'letters only'   => ['abc'],
+			'empty'          => [''],
+		];
+	}
+
+	/**
+	 * @dataProvider rejectedInputProvider
+	 *
+	 * @param string $typed
+	 *
+	 * @return void
+	 */
+	public function testUnusableInputIsRejectedAndFlagged($typed): void {
+		$this->assertFalse(thold_display_to_raw($typed, 'thold_hi'));
+		$this->assertArrayHasKey('thold_hi', $_SESSION['sess_error_fields']);
+	}
+
+	/**
+	 * @return void
+	 */
+	public function testNonNumericInputHasNoDisplayForm(): void {
+		$this->assertFalse(thold_raw_to_display('abc'));
+	}
+
+	/**
+	 * Beyond the largest and smallest suffix there is nothing left to index,
+	 * and the old code read past the end of the pattern and dropped the
+	 * magnitude entirely.
+	 *
+	 * @return void
+	 */
+	public function testMagnitudesBeyondTheLargestSuffixKeepTheirScale(): void {
+		$rendered = thold_raw_to_display(5.0e27);
+
+		$this->assertNotSame('5', $rendered);
+		$this->assertEqualsWithDelta(5.0e27, (float) thold_display_to_raw($rendered, 'thold_hi'), 5.0e18);
+	}
+
+	/**
+	 * @return void
+	 */
+	public function testMagnitudesBelowTheSmallestSuffixKeepTheirScale(): void {
+		$rendered = thold_raw_to_display(5.0e-18);
+
+		$this->assertNotSame('5', $rendered);
+		$this->assertEqualsWithDelta(5.0e-18, (float) thold_display_to_raw($rendered, 'thold_hi'), 5.0e-27);
+	}
+}

thold_functions.php

@@ -5370,6 +5370,40 @@ function thold_create_new_graph_from_template() {
  * @param mixed $number
  * @param mixed $field_name
  */
+/**
+ * SI suffixes thold accepts on a threshold bound, smallest first.
+ *
+ * thold_display_to_raw() and thold_raw_to_display() are inverses of each
+ * other, so they read the same table rather than each carrying their own
+ * copy. They previously disagreed: 'p' scaled by 1e-9 on the way in while
+ * 1e-12 rendered as 'f' on the way out, so a bound was divided by a thousand
+ * every time its form was opened and saved.
+ *
+ * @return array<string, float> Suffix to the factor it multiplies by.
+ */
+function thold_unit_suffixes() {
+	static $suffixes = [
+		'y' => 1e-24,
+		'z' => 1e-21,
+		'a' => 1e-18,
+		'f' => 1e-15,
+		'p' => 1e-12,
+		'n' => 1e-9,
+		'u' => 1e-6,
+		'm' => 1e-3,
+		'K' => 1e3,
+		'M' => 1e6,
+		'G' => 1e9,
+		'T' => 1e12,
+		'P' => 1e15,
+		'E' => 1e18,
+		'Z' => 1e21,
+		'Y' => 1e24,
+	];
+
+	return $suffixes;
+}
+
 function thold_display_to_raw($number, $field_name) {
 	$number = trim($number);
 
@@ -5382,96 +5416,19 @@ function thold_display_to_raw($number, $field_name) {
 		return $number;
 	}
 
-	$number = trim(substr($number, 0, -1));
+	$number   = trim(substr($number, 0, -1));
+	$suffixes = thold_unit_suffixes();
 
-	if (!is_numeric($number)) {
+	if (!is_numeric($number) || !isset($suffixes[$suffix])) {
 		$_SESSION['sess_error_fields'][$field_name] = $field_name;
 		raise_message(3);
 
 		return false;
 	}
 
-	switch($suffix) {
-		case 'f':
-			return $number * 1e-15;
-
-			break;
-		case 'p':
-			return $number * 1e-9;
-
-			break;
-		case 'u':
-			return $number * 1e-6;
-
-			break;
-		case 'm':
-			return $number * 1e-3;
-
-			break;
-		case 'K':
-			return $number * 1e3;
-
-			break;
-		case 'M':
-			return $number * 1e6;
-
-			break;
-		case 'G':
-			return $number * 1e9;
-
-			break;
-		case 'T':
-			return $number * 1e12;
-
-			break;
-		case 'P':
-			return $number * 1e15;
-
-			break;
-		case 'E':
-			return $number * 1e18;
-
-			break;
-		case 'Z':
-			return $number * 1e21;
-
-			break;
-		case 'Y':
-			return $number * 1e24;
-
-			break;
-		default:
-			$_SESSION['sess_error_fields'][$field_name] = $field_name;
-			raise_message(3);
-
-			return false;
-	}
+	return $number * $suffixes[$suffix];
 }
 
-/**
- * thold_display_to_raw - Converts a displayed number to a raw
- * numeric value.  This function converts number like '100M'
- * to the raw number 100,000,000, etc.
- *
- * Supported Units
- *
- * Unit  Expression
- * ----  -------------------------------------
- * f     Fermo (10e-12)
- * p     Pico  (10e-9)
- * u     Micro (10e-6)
- * m     Milli (10e-3)
- * K     Killo (10e3)
- * M     Mega  (10e6)
- * G     Giga  (10e9)
- * T     Terra (10e12)
- * P     Peta  (10e15)
- * E     Exa   (10e18)
- * Z     Zeta  (10e21)
- * Y     Yota  (10e24)
- *
- * @param mixed $number
- */
 function thold_raw_to_display($number) {
 	if ($number != '') {
 		$number = trim($number);
@@ -5485,42 +5442,27 @@ function thold_raw_to_display($number) {
 		return trim($number);
 	}
 
-	if ($number > 0) {
-		$multiplier = 1;
-	} else {
-		$multiplier = -1;
-	}
-
-	$number = abs($number);
-	$suffix = '';
+	$multiplier = $number > 0 ? 1 : -1;
+	$number     = abs($number);
 
-	if ($number > 1) {
-		$pattern = 'KMGTPEZY';
-		$count   = 0;
+	// The largest scale that still leaves a value of one or more, where the
+	// empty suffix stands for a scale of one. Factors ascend, so the ratio
+	// falls monotonically and the last match is the one wanted.
+	$scales = thold_unit_suffixes();
+	$scales[''] = 1.0;
+	asort($scales, SORT_NUMERIC);
 
-		while ($number >= 1e3) {
-			$count++;
-			$number /= 1e3;
-		}
-
-		if ($count > 0) {
-			$suffix = $pattern[$count - 1];
-		}
-	} else {
-		$pattern = 'mupf';
-		$count   = 0;
-
-		while ($number < 1) {
-			$count++;
-			$number *= 1e3;
-		}
+	$suffix = '';
+	$factor = 1.0;
 
-		if ($count > 0) {
-			$suffix = $pattern[$count - 1];
+	foreach ($scales as $candidate => $candidate_factor) {
+		if ($number / $candidate_factor >= 1) {
+			$suffix = $candidate;
+			$factor = $candidate_factor;
 		}
 	}
 
-	return trim(($number * $multiplier) . $suffix);
+	return trim((($number / $factor) * $multiplier) . $suffix);
 }
 
 function save_thold() {