From 1997691de0220d15816869ec0697cca0f1e5cda9 Mon Sep 17 00:00:00 2001 From: TheWitness Date: Fri, 16 Oct 2026 11:23:07 -0400 Subject: [PATCH] Render RRDtool Tune Info as balanced div boxes and POST the Data Source repair actions (#8412) * data_sources: render RRDtool Tune Info as balanced div boxes rrdtool_tune() emitted an unbalanced mix of html_header() and html_end_box() calls wrapped around raw table rows inside the single box opened by data_sources.php. The premature and extra closing tags corrupted the DOM, which (depending on theme) clipped the Save button at the bottom of the Data Source edit page while in Data Source Info mode. - Separate the execute, CLI and HTML code paths in rrdtool_tune() and, for HTML, render each section as a self contained, balanced div box. - Extract and rename the helper print_leaves() to the standalone rrdtool_tune_print_leaves(), matching the rrdtool_tune_* naming convention, and emit div rows instead of table rows. - Switch the data_sources.php wrapper box to div mode to match. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * data_debug: require POST for the Data Source repair actions Applying RRDfile repairs and rerunning the debugger are state changing operations, so they should not be reachable with a GET. Driving them through a GET also meant the follow up redirect was pulled into the content area as a full page, losing the theme and navigation on rerender. Register data_debug.php run_repair and run_debug in the cactiIsPostActionUrl() allowlist so loadUrl() upgrades them to a token-bearing POST, and add the matching csrf_require_post() guard so the browser transport and server side guard stay in sync. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * rrd: split rrdtool_tune into testable render and execute helpers Break rrdtool_tune() into small single-purpose helpers so each path can be exercised directly by unit tests (the web display path is otherwise unreachable under CACTI_CLI): - rrdtool_tune_print_leaves() takes an explicit $html flag instead of branching on CACTI_CLI, so both the div and plain-text branches are testable. - rrdtool_tune_execute() applies the tune/resize commands for a repair. - rrdtool_tune_print_cli() prints the source commands as plain text. - rrdtool_tune_print_html() renders the balanced div boxes. - rrdtool_tune() becomes a thin dispatcher over the three. Add tests/Unit/RrdTuneRenderTest.php covering the leaf printer, the HTML and CLI printers, the execute path and the dispatcher, asserting the output is balanced div markup with no table rows. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * review: post Data Source Troubleshooter actions via cactiPostAction Address PR review feedback: - Switch the Apply Suggested Fixes and Rerun Analysis links to the existing cactiPostAction/data-url idiom (as data_sources.php already does for ds_repair) instead of upgrading a loadUrl() GET through the cactiIsPostActionUrl allowlist. The allowlist path replayed the query-free mutation URL into browser history, so a refresh reopened the list instead of the selected Data Source; a full cactiPostAction submission follows the server redirect to the read-only view URL and records that in history. The run_repair / run_debug csrf_require_post guards are retained, and the data_debug.php allowlist entry (and its now redundant click handlers) are removed. - Add a callable $executor test seam to rrdtool_tune_execute() (matching rrdtool_dump_document) so the unit tests assert the tune and resize commands are actually executed, with resize.rrd produced only when the resize command runs, rather than passing on the rename alone. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * review: move RrdTuneRenderTest into the Unit/Core/Rrd category folder Per AGENTS.md, new unit tests belong in a category subfolder rather than directly under tests/Unit. Move RrdTuneRenderTest there and update the UnitStubs include depth accordingly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: TheWitness Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- data_debug.php | 20 +- data_sources.php | 8 +- lib/rrd.php | 182 ++++++++++++------- tests/Unit/Core/Rrd/RrdTuneRenderTest.php | 212 ++++++++++++++++++++++ 4 files changed, 334 insertions(+), 88 deletions(-) create mode 100644 tests/Unit/Core/Rrd/RrdTuneRenderTest.php diff --git a/data_debug.php b/data_debug.php index b0d6a8f14..f195957f6 100644 --- a/data_debug.php +++ b/data_debug.php @@ -37,6 +37,10 @@ set_default_action(); draw_data_debug_filter(false); +if (in_array(grv('action'), ['run_repair', 'run_debug'], true)) { + csrf_require_post(); +} + switch (grv('action')) { case 'actions': form_actions(); @@ -769,7 +773,7 @@ function debug_view() : void { } elseif ($debug_status == 'analysis') { html_start_box(__('Data Source Troubleshooter [ Auto Refreshing till RRDfile Update ] %s', ''), '100%', false, 3, 'center', ''); } else { - html_start_box(__('Data Source Troubleshooter [ Analysis Complete! %s ]', '' . __('Rerun Analysis') . ''), '100%', false, 3, 'center', ''); + html_start_box(__('Data Source Troubleshooter [ Analysis Complete! %s ]', '' . __('Rerun Analysis') . ''), '100%', false, 3, 'center', ''); } html_header( @@ -859,7 +863,7 @@ function debug_view() : void { $path = get_data_source_path($id, true); if (is_writeable($path)) { - html_start_box(__('Repair Steps [ %s ]', '' . __('Apply Suggested Fixes') . ''), '100%', false, 3, 'center', ''); + html_start_box(__('Repair Steps [ %s ]', '' . __('Apply Suggested Fixes') . ''), '100%', false, 3, 'center', ''); } else { html_start_box(__('Repair Steps [ Run Fix from Command Line ]', $path), '100%', false, 3, 'center', ''); } @@ -889,22 +893,10 @@ function debug_view() : void { ?> ' . __('Apply Suggested Fixes') . ''), '100%', false, 3, 'center', ''); + html_start_box(__('RRDtool Tune Info [ %s ]', '' . __('Apply Suggested Fixes') . ''), '100%', true, 3, 'center', ''); } elseif ($has_tune) { - html_start_box(__('RRDtool Tune Info [ %s ]', __('Run Fix from Command Line')), '100%', false, 3, 'center', ''); + html_start_box(__('RRDtool Tune Info [ %s ]', __('Run Fix from Command Line')), '100%', true, 3, 'center', ''); } else { - html_start_box(__('RRDtool Tune Info'), '100%', false, 3, 'center', ''); + html_start_box(__('RRDtool Tune Info'), '100%', true, 3, 'center', ''); } rrdtool_tune($rrd_info['filename'], $diff, true); - html_end_box(); + html_end_box(true, true); } } diff --git a/lib/rrd.php b/lib/rrd.php index 0c9a97a70..00f20a027 100644 --- a/lib/rrd.php +++ b/lib/rrd.php @@ -4802,102 +4802,144 @@ function rrdtool_info2html(array $info_array, array $diff = []) : void { } /** - * print_leaves - A function used by the rrdtool_tune function to output data + * rrdtool_tune_print_leaves - a helper used by rrdtool_tune_print_cli() and + * rrdtool_tune_print_html() to recursively output the leaf nodes of an rrdtool + * 'diff' array * * @param array $array The array of leaves + * @param bool $html Emit div rows for the web UI, or plain text for the CLI * * @return void */ -function print_leaves(array $array) : void { +function rrdtool_tune_print_leaves(array $array, bool $html = true) : void { foreach ($array as $key => $line) { if (!is_array($line)) { - if (CACTI_CLI) { - print $line . PHP_EOL; + if ($html) { + print "
" . $line . '
'; } else { - print '' . $line . ''; + print $line . PHP_EOL; } } else { - if ($key === 'tune') { + if ($key === 'tune' || $key === 'resize') { continue; } - if ($key === 'resize') { - continue; - } - print_leaves($line); + rrdtool_tune_print_leaves($line, $html); } } } +/** + * rrdtool_tune_execute - apply the rrdtool tune/resize commands for a repair + * + * @param string $rrd_file The rrd file name + * @param array $diff The array of discrepancies between cacti settings and rrd file info + * @param callable|null $executor Test seam for RRDtool execution + * + * @return void + */ +function rrdtool_tune_execute(string $rrd_file, array $diff, ?callable $executor = null) : void { + $executor ??= 'rrdtool_execute'; + + if (isset($diff['tune']) && cacti_sizeof($diff['tune'])) { + foreach ($diff['tune'] as $line) { + $executor("tune $line", true, RRDTOOL_OUTPUT_STDOUT); + } + } + + if (isset($diff['resize']) && cacti_sizeof($diff['resize'])) { + foreach ($diff['resize'] as $line) { + $executor("resize $line", true, RRDTOOL_OUTPUT_STDOUT); + rename(dirname($rrd_file) . '/resize.rrd', $rrd_file); + } + } +} + +/** + * rrdtool_tune_print_cli - print the rrdtool tune/resize source commands as plain text + * + * @param string $rrd_file The rrd file name + * @param array $diff The array of discrepancies between cacti settings and rrd file info + * + * @return void + */ +function rrdtool_tune_print_cli(string $rrd_file, array $diff) : void { + $rrd_path = read_config_option('path_rrdtool'); + + if (cacti_sizeof($diff)) { + rrdtool_tune_print_leaves($diff, false); + } + + if (isset($diff['tune']) && cacti_sizeof($diff['tune'])) { + foreach ($diff['tune'] as $line) { + print $rrd_path . ' tune ' . $line . PHP_EOL; + } + } + + if (isset($diff['resize']) && cacti_sizeof($diff['resize'])) { + foreach ($diff['resize'] as $line) { + print $rrd_path . ' resize ' . $line . PHP_EOL; + print __('rename %s to %s', dirname($rrd_file) . '/resize.rrd', $rrd_file) . PHP_EOL; + } + } +} + +/** + * rrdtool_tune_print_html - render the rrdtool tune/resize output as balanced div boxes + * + * @param string $rrd_file The rrd file name + * @param array $diff The array of discrepancies between cacti settings and rrd file info + * + * @return void + */ +function rrdtool_tune_print_html(string $rrd_file, array $diff) : void { + $rrd_path = read_config_option('path_rrdtool'); + + if (cacti_sizeof($diff)) { + html_start_box(__('Errors Found'), '100%', true, 3, 'center', ''); + + rrdtool_tune_print_leaves($diff, true); + + html_end_box(true, true); + } + + if ((isset($diff['tune']) && cacti_sizeof($diff['tune'])) || + (isset($diff['resize']) && cacti_sizeof($diff['resize']))) { + html_start_box(__('Optional Tuning Recommendations'), '100%', true, 3, 'center', ''); + + if (isset($diff['tune'])) { + foreach ($diff['tune'] as $line) { + print "
" . $rrd_path . ' tune ' . $line . '
'; + } + } + + if (isset($diff['resize'])) { + foreach ($diff['resize'] as $line) { + print "
" . $rrd_path . ' resize ' . $line . '
'; + print "
" . __('rename %s to %s', dirname($rrd_file) . '/resize.rrd', $rrd_file) . '
'; + } + } + + html_end_box(true, true); + } +} + /** * rrdtool_tune - create rrdtool tune/resize commands html+cli enabled * * @param string $rrd_file The rrd file name * @param array $diff The array of discrepancies between cacti settings and rrd file info - * @param bool $show_source The only show text+commands or execute all commands, execute is for cli mode only! + * @param bool $show_source Only show text+commands, or execute all commands; execute is for cli mode only! * * @return void */ function rrdtool_tune(string $rrd_file, array $diff, bool $show_source = true) : void { - $rrd_path = read_config_option('path_rrdtool'); - - $cmd = []; - - if ($show_source && cacti_sizeof($diff)) { - if (!CACTI_CLI) { - html_header([__('Errors Found')]); - } - - print_leaves($diff); - - if (!CACTI_CLI) { - html_end_box(); - } - } - - if (isset($diff['tune']) || isset($diff['resize'])) { - if (!CACTI_CLI) { - html_header([__('Optional Tuning Recommendations')]); - } - } - - if (isset($diff['tune']) && cacti_sizeof($diff['tune'])) { - // create tune commands - foreach ($diff['tune'] as $line) { - if ($show_source == true) { - if (CACTI_CLI) { - print $rrd_path . ' tune ' . $line . PHP_EOL; - } else { - print '' . $rrd_path . ' tune ' . $line . ''; - } - } else { - rrdtool_execute("tune $line", true, RRDTOOL_OUTPUT_STDOUT); - } - } - } - - if (isset($diff['resize']) && cacti_sizeof($diff['resize'])) { - // each resize goes into an extra line - foreach ($diff['resize'] as $line) { - if ($show_source == true) { - if (CACTI_CLI) { - print read_config_option('path_rrdtool') . ' resize ' . $line . PHP_EOL; - print __('rename %s to %s', dirname($rrd_file) . '/resize.rrd', $rrd_file) . PHP_EOL; - } else { - print '' . read_config_option('path_rrdtool') . ' resize ' . $line . ''; - print '' . __('rename %s to %s', dirname($rrd_file) . '/resize.rrd', $rrd_file) . ''; - } - } else { - rrdtool_execute("resize $line", true, RRDTOOL_OUTPUT_STDOUT); - rename(dirname($rrd_file) . '/resize.rrd', $rrd_file); - } - } - } - - if (isset($diff['tune']) || isset($diff['resize'])) { - if (!CACTI_CLI) { - html_end_box(); - } + if (!$show_source) { + rrdtool_tune_execute($rrd_file, $diff); + } elseif (CACTI_CLI) { + rrdtool_tune_print_cli($rrd_file, $diff); + } else { + rrdtool_tune_print_html($rrd_file, $diff); } } diff --git a/tests/Unit/Core/Rrd/RrdTuneRenderTest.php b/tests/Unit/Core/Rrd/RrdTuneRenderTest.php new file mode 100644 index 000000000..e79f35e34 --- /dev/null +++ b/tests/Unit/Core/Rrd/RrdTuneRenderTest.php @@ -0,0 +1,212 @@ + ['/rra/x.rrd DEL:timeafter'], + 'resize' => ['/rra/x.rrd 0 GROW 10'], + ]; + + $output = rrd_tune_capture(function () use ($diff) { + rrdtool_tune_print_leaves($diff, true); + }); + + expect($output)->toContain("
The Graph requires 1 Data Sources but the RRDfile includes 2.
"); + // the tune and resize branches are intentionally skipped by the leaf printer + expect($output)->not->toContain('DEL:timeafter'); + expect(substr_count($output, 'toBe(substr_count($output, '')); +}); + +test('rrdtool_tune_print_leaves emits plain text for the CLI', function () { + $diff = [ + 'Errors were found', + 'nested' => ['Deeper error'], + 'tune' => ['ignored'], + ]; + + $output = rrd_tune_capture(function () use ($diff) { + rrdtool_tune_print_leaves($diff, false); + }); + + expect($output)->toContain('Errors were found' . PHP_EOL); + expect($output)->toContain('Deeper error' . PHP_EOL); + expect($output)->not->toContain('not->toContain('ignored'); +}); + +test('rrdtool_tune_print_html renders self contained, balanced div boxes', function () { + $diff = [ + 'Data Source timeafter is included but is not required', + 'tune' => ['/rra/x.rrd DEL:timeafter'], + 'resize' => ['/rra/x.rrd 0 GROW 10'], + ]; + + $output = rrd_tune_capture(function () use ($diff) { + rrdtool_tune_print_html('/rra/x.rrd', $diff); + }); + + expect($output)->toContain('Errors Found'); + expect($output)->toContain('Optional Tuning Recommendations'); + expect($output)->toContain('tune /rra/x.rrd DEL:timeafter'); + expect($output)->toContain('resize /rra/x.rrd 0 GROW 10'); + expect($output)->toContain('rename'); + // the panel must not emit raw table markup any more + expect($output)->not->toContain(''); + expect($output)->not->toContain(''); + // every div that opens must close + expect(substr_count($output, 'toBe(substr_count($output, '')); +}); + +test('rrdtool_tune_print_html is empty when there is nothing to report', function () { + $output = rrd_tune_capture(function () { + rrdtool_tune_print_html('/rra/x.rrd', []); + }); + + expect($output)->toBe(''); +}); + +test('rrdtool_tune_print_cli prints the rrdtool source commands', function () { + $diff = [ + 'An error leaf', + 'tune' => ['/rra/x.rrd DEL:timeafter'], + 'resize' => ['/rra/x.rrd 0 GROW 10'], + ]; + + $output = rrd_tune_capture(function () use ($diff) { + rrdtool_tune_print_cli('/rra/x.rrd', $diff); + }); + + expect($output)->toContain('An error leaf' . PHP_EOL); + expect($output)->toContain('tune /rra/x.rrd DEL:timeafter' . PHP_EOL); + expect($output)->toContain('resize /rra/x.rrd 0 GROW 10' . PHP_EOL); + expect($output)->toContain('rename'); + expect($output)->not->toContain(' [cacti_escapeshellarg($target) . ' DEL:timeafter'], + 'resize' => [cacti_escapeshellarg($target) . ' 0 GROW 10'], + ]; + + // a recording executor stands in for rrdtool and, like the real resize, + // only produces resize.rrd when the resize command actually runs + $commands = []; + $executor = function ($command) use (&$commands, $dir) { + $commands[] = $command; + + if (strpos($command, 'resize ') === 0) { + file_put_contents($dir . '/resize.rrd', 'resized'); + } + + return true; + }; + + rrdtool_tune_execute($target, $diff, $executor); + + // both commands must have been executed, not just the rename + expect($commands)->toHaveCount(2); + expect($commands[0])->toBe('tune ' . cacti_escapeshellarg($target) . ' DEL:timeafter'); + expect($commands[1])->toBe('resize ' . cacti_escapeshellarg($target) . ' 0 GROW 10'); + + // the resize step renames resize.rrd over the target + expect(file_exists($dir . '/resize.rrd'))->toBeFalse(); + expect(file_get_contents($target))->toBe('resized'); + + @unlink($target); + @rmdir($dir); +}); + +test('rrdtool_tune_execute skips the resize rename when no resize is produced', function () { + $dir = sys_get_temp_dir() . '/rrd_tune_noresize_' . getmypid(); + @mkdir($dir); + + $target = $dir . '/data.rrd'; + file_put_contents($target, 'target'); + + $commands = []; + $executor = function ($command) use (&$commands) { + $commands[] = $command; + + return true; + }; + + rrdtool_tune_execute($target, ['tune' => [cacti_escapeshellarg($target) . ' DEL:timeafter']], $executor); + + expect($commands)->toBe(['tune ' . cacti_escapeshellarg($target) . ' DEL:timeafter']); + expect(file_get_contents($target))->toBe('target'); + + @unlink($target); + @rmdir($dir); +}); + +test('rrdtool_tune dispatches to the execute path when not showing the source', function () { + $dir = sys_get_temp_dir() . '/rrd_tune_disp_' . getmypid(); + @mkdir($dir); + + $target = $dir . '/data.rrd'; + file_put_contents($target, 'target'); + file_put_contents($dir . '/resize.rrd', 'resized'); + + $diff = [ + 'resize' => [cacti_escapeshellarg($target) . ' 0 GROW 10'], + ]; + + // no executor is supplied, so the real rrdtool_execute is invoked; it returns + // without side effects in this environment while the resize rename still runs + rrdtool_tune($target, $diff, false); + + expect(file_get_contents($target))->toBe('resized'); + + @unlink($target); + @rmdir($dir); +}); + +test('rrdtool_tune dispatches to the CLI printer under CACTI_CLI', function () { + expect(CACTI_CLI)->toBeTrue(); + + $diff = [ + 'tune' => ['/rra/x.rrd DEL:timeafter'], + ]; + + $output = rrd_tune_capture(function () use ($diff) { + rrdtool_tune('/rra/x.rrd', $diff, true); + }); + + expect($output)->toContain('tune /rra/x.rrd DEL:timeafter' . PHP_EOL); + expect($output)->not->toContain('