Repository navigation
issue: Make the Listener and Stream filter-only and auto-materialize on save - #302
Conversation
…on save The Listener and Stream are part of what a Filter materializes, so changing them on the report page ran the report against data that was never materialized. Remove both dropdowns from the FlowView tab; they are set only in the Filter editor now, and run_flow_query() reads them straight from the saved Filter rather than from the page. The report page no longer sends, validates, seeds or saves device_id/ex_addr, and they are dropped from the session cache key. The New button now follows Edit. When a Filter is saved with a change that affects its cached results, from the editor or the FlowView tab, a materialization is now started in the background for it and a message says so, instead of a dialog asking the user to start one by hand. The old post-save dialog and its recommendation helper are removed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Saves can drop required rebuilds during active materialization, and graph drilldowns lose their listener/exporter scope.
2 open issues
What changed in this PR
This PR makes saved Filters the sole source of Listener and Stream settings for FlowView reports and automatically starts materialization after cache-affecting saves.
Changes:
- Removes Listener and Stream controls and request overrides from the report page, and moves New after Edit.
- Replaces manual rematerialization guidance with automatic background launches and updated notifications.
- Updates regression tests, shared test stubs, translation strings, and the changelog.
| File | Description |
|---|---|
| tests/Unit/FlowviewFilterControlsTest.php | Checks removed dropdowns and button order. |
| tests/Unit/FilterEditAndSaveTest.php | Tests automatic materialization and filter-only settings. |
| tests/Unit/CoreFunctionsTest.php | Updates dialog and saved-exporter behavior tests. |
| tests/bootstrap-unit.php | Adds background-execution and shell-escaping stubs. |
| locales/po/cacti.pot | Refreshes translated message templates. |
| includes/functions.php | Updates report controls, query scope, and save-triggered materialization. |
| flowview.php | Removes Listener and Stream request overrides and session seeding. |
| CHANGELOG.md | Documents filter-only controls and automatic materialization. |
Review effort: Balanced
# Conflicts: # CHANGELOG.md
Resolve two reviewer comments on the filter-only Listener/Stream PR. Queue a fresh materialization when a Filter is saved while one is already running for it. flowview_launch_materialize() leaves a Filter that is already being materialized to the active run, which works from the settings as they were, so the new settings went uncached. The save now detects that nothing was launched, records the Filter in a queue, and tells the user the rebuild is queued rather than started. The poller drains the queue with flowview_launch_queued_rematerializations() once no materialization is running, dropping any Filter that was deleted. Point the NetFlow graph drilldown at a saved Filter scoped to the graph's exporter. The report page now reads the Listener and Stream from the Filter, so flowview_graph_button()'s ex_addr and device_id overrides were ignored and the icon showed every exporter under the default Filter. It now links to the Filter matched to the graph's exporter and is omitted when none matches. Covered by new Pest tests for the queue helpers, the queued save paths and the existing suite (387 passing). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add Pest tests for the two early-return branches the changed-line coverage gate flagged: flowview_queue_rematerialize() ignoring a non-positive id, and flowview_launch_queued_rematerializations() returning early when nothing is queued. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
somethingwithproof
left a comment
somethingwithproof
left a comment
There was a problem hiding this comment.
Blocker: flowview_graph_button() still selects LIMIT 1 by device_id only. A listener can have saved Filters for multiple ex_addr values, so the new query-only URL can point at an arbitrary Filter for the right listener but the wrong stream; the removed request override can no longer correct it. Match the saved Filter on both listener and exporter, including the defined All-filter sentinels, make the choice deterministic, and cover two Filters on one listener.
Blocker: flowview_rematerialize_queue is a read-modify-write comma-separated setting with no synchronization. Two concurrent saves can overwrite one another, and the poller can clear a Filter queued after its stale read; that Filter will never be rebuilt under its saved settings. Persist queue entries with an atomic keyed or transactional protocol and add overlapping save-and-drain coverage.
Scope the NetFlow graph drilldown to both the graph host's Listener and Stream. flowview_graph_button() matched a saved Filter on device_id alone with LIMIT 1, so a Listener with Filters for several streams could drill into the wrong one and the removed request override could no longer correct it. The new flowview_graph_button_filter() helper matches on both the Listener and the Stream (including the All-listener and All-stream sentinels), prefers the most specific Filter deterministically, and returns nothing when none matches so the link is omitted rather than pointing at the wrong exporter. Make the rematerialize queue atomic. The queue was a comma-separated setting read, modified and written in place, so two concurrent saves could overwrite one another and the poller could clear a Filter queued after its stale read. Each Filter is now its own settings row keyed by id with a token value; the poller compare-and-deletes only the token it read, so concurrent saves never lose one another and a Filter re-queued while the poller drains is kept. Tests: rewrite the queue tests for the keyed protocol, add overlapping save-and-drain coverage, and cover the graph-button Filter scoping. Full suite 393 passing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Fixed in 5f198d0. Blocker 1 — graph drilldown matched the Listener only. WHERE (device_id = ? OR device_id = 0)
AND (ex_addr = ? OR ex_addr = ? OR ex_addr = ? OR ex_addr = ?) -- exporter, '-1', '', '0'
ORDER BY (device_id = ?) DESC, (ex_addr = ?) DESC, id ASC
LIMIT 1A Filter scoped to a different specific Stream is excluded by the Blocker 2 — the rematerialize queue was an unsynchronized read-modify-write. The single comma-separated
New Pest tests cover the keyed enqueue, the id-ordered listing, the compare-and-delete that survives an overlapping re-queue (the save-and-drain race), the still-running wait, and the deleted-Filter drop. Full suite: 393 passing. |
somethingwithproof
left a comment
somethingwithproof
left a comment
There was a problem hiding this comment.
The two prior blockers are resolved on this head: drilldown selection now scopes listener and stream deterministically, and the queue uses per-filter token rows with compare-and-delete coverage. I cannot provide an independent approval because this PR’s history includes my merge commit f9b5954. Current-head PHP jobs also stop before tests at Composer’s ownership/utime() failure; rebase after #304 or include that workflow fix, rerun green, and obtain an independent approval.
Issues are resolved now.
Composer runs as the runner user in a later CI step and calls touch() while generating the fixture autoloader; utime() only succeeds for the file owner, so the runner user must own the tree. www-data is retained as group and ownership is handed back before polling. Mirrors the fix in Cacti/plugin_evidence#63. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ion tokens The SQL LIKE that loads queued rematerialization settings can match case-insensitively, so flowview_get_queued_rematerialization_tokens() has a case-sensitive strncmp guard that skips rows which only match by case. Add a unit test feeding a case-variant settings row so that guard (the previously uncovered continue branch) is exercised, restoring 100% patch coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Summary
On the FlowView report tab, the Listener and Stream dropdowns are part of what a Filter materializes, so changing them there ran the report against data that was never materialized (always a cache miss / "fresh" scan). This removes both dropdowns so those settings live only in the Filter editor, reorders the New button to follow Edit, and makes a save that changes what is cached start a materialization automatically.
Changes
Listener / Stream are filter-only
device_id) and Stream (ex_addr) dropdowns from the report filter bar.run_flow_query()now reads the Listener and Stream straight from the saved Filter row, never from the page request.device_id/ex_addr: dropped fromflowview_request_vars()validation,flowview_seed_filter_request_vars(),load_session_for_filter(), the top-tab Save (save_filter_form), Save As, and the chart/table/apply JavaScript.device_idis also dropped from the session cache key.Button order
Auto-materialize on a cache-affecting save
flowview_launch_materialize()for that Filter and raises a message: "…a materialization of it has been started in the background. Its progress is shown on the Filters page…".flowview_show_saved_dialog/flowview_rematerialize_recommendationhelpers) is removed. The pre-save Continue/Cancel warning is reworded to say saving starts the materialization.Testing
run_flow_query/flowview_plan_filterassert the exporter address comes from the saved Filter, not the page. Added sharedexec_background/cacti_escapeshellargtest stubs.locales/po/cacti.potregenerated; the i18n gate passes locally.