[rest] Do not report a missing store strategy when another config stores the items

[ML19821]

Description

Fixes #5777.

httpGetPersistenceHealth() reports PERSISTENCE_SERVICE_ITEMS_NO_STORE_STRATEGY for every configuration entry whose only strategy is restoreOnStartup, judging each entry in isolation. Persistence application is additive — PersistenceManagerImpl.storeItem() streams over all matching configurations — so the split configuration below is correct and still permanently red:

items: ["*"],                  strategies: ["everyChange"]
items: ["gRestoreOnStartup*"], strategies: ["restoreOnStartup"]

The additive rule is documented behaviour, not an implementation detail: configuration/persistence.md says "The entries are additive. This means if one Item appears in more than one <itemlist> … all the strategies … listed on all those lines apply to that Item." The check therefore contradicts the documented semantics of the format it checks, and states something factually wrong about the user's configuration.

The problem is now reported only when no other entry of the same service both carries a store strategy and provably covers the same items.

Conservative on purpose. Suppressing a warning that ought to fire means silently losing data, which is far worse than one warning too many, so coverage has to be provable from the configuration alone:

• Store strategies are identified positively, not by exclusion: only everyUpdate, everyChange and cron strategies count, because those are the ones PersistenceManagerImpl actually acts on (schedulePersistJobs() turns cron strategies into ordinary persist jobs). A file-based configuration may name an arbitrary strategy, which PersistenceModelManager turns into a plain PersistenceStrategy instance that nothing ever executes — treating "neither restore nor forecast" as storing would let such a strategy silence a warning that should fire.
• Coverage may itself be additive, matching the rule the fix is about: a selector only has to be found in some storing entry, not in one single entry that covers them all. A restore-only entry selecting ItemA, ItemB is therefore covered by two separate entries that store ItemA and ItemB respectively — but it keeps warning if only one of the two is covered.
• An entry carrying any exclude selector never silences anything, because appliesToItem() drops the whole entry for the excluded items, so nothing about it is provable. A * entry with !Item or !Group* is not evidence of coverage.
• Selectors are compared structurally rather than with equals(), because the PersistenceConfig implementations do not override it. Comparison is by concrete type plus identifying field, and any implementation this method does not know is treated as not equal — calling two unknown selectors equal would silence a warning. PersistenceAllConfig, the one type where the class alone is decisive, is handled by the caller before that point.

Testing

PersistenceResourceTest had no coverage of httpGetPersistenceHealth at all; these are the first tests for it. Ten cases, deliberately weighted towards the ones that must keep warning:

Case	Expectation
the standard split (* + everyChange, restore-only group)	not reported
a restore-only entry with nothing storing those items	still reported
a * store entry carrying an exclude selector	still reported
covered by an entry with a cron strategy	not reported
covered only by an entry that itself has just forecast	still reported
covered only by an entry with an arbitrary named strategy	still reported
store entry in service A, restore-only entry in service B	still reported (no bleed between services)
exact-selector coverage (same group selector + everyUpdate)	not reported
coverage split across two store entries, one selector each	not reported
the same, but only one of the two selectors covered	still reported

Bundle build green, static analysis clean.

The cases that require the new behaviour (standard split, cron coverage, exact-selector coverage, split coverage) were verified to fail against the unmodified code — the ones asserting that the warning is still raised pass under both versions by construction, which is the point of including them.

Verified on a live openHAB 5.2.1 installation as well: the reported items carried several thousand datapoints per 48 hours in InfluxDB while the health page insisted they had no store strategy.

Note for completeness: a configuration can be rewritten so that every entry carries its own store strategy (["*", "!gRestoreOnStartup*"] plus ["gRestoreOnStartup*"] with both store and restore strategies), and that is arguably tidier. It does not make the report correct, though — the documented additive form stays valid, and users following the documentation should not be told their configuration is broken.

Signed-off-by (in commit): Martin Littkovsky 2018turtle@proton.me

[ML19821]

Two practical additions, in case they help judging the change.

How the configuration that triggered this looks once worked around. For anyone finding this issue with a red health page, the warning can be avoided today by giving every entry its own store strategy instead of relying on the additive rule:

items: ["*", "!gRestoreOnStartup*"],  strategies: ["everyUpdate", "everyMinute"]
items: ["gRestoreOnStartup*"],        strategies: ["everyUpdate", "everyMinute", "restoreOnStartup"]

I have been running exactly this since yesterday and can confirm it behaves: same restore scope, no item left unstored, and a reboot restored all 61 group members (verified item by item, including ones carrying a hand-set setting with no live source, which are the only ones where a broken restore would be silent).

It is arguably the tidier configuration, so it is worth asking whether the check is nudging users somewhere good. I still think the report is wrong as it stands, for two reasons: it makes a factual claim — these items have no store strategy — that is false for the additive form, and the additive form is what the documentation describes. A nudge would be worded differently from a problem.

A documentation gap this exposed, which I am happy to fix separately. Getting from "store everything, restore only a group" to a correct configuration is currently a two-step trap. configuration/persistence.md shows restoreOnStartup only alongside a store strategy on the same line, for two individual items. The obvious extension — putting the store strategies on the group line as well, while a * line already covers everything — persists those items twice, once per matching line. Removing the store strategies again then lands exactly in this report. I walked into both, in that order, before understanding the additive rule.

A short example for that case would probably save others the round trip. I would rather wait for the outcome here before writing it, since the shape worth recommending depends on whether this fix lands — no point documenting a workaround for something about to be fixed. Happy to open a docs PR either way if you think it is worth having; there is also a duplicated word in that same paragraph ("all the strategies strategies listed") that I would fix in passing.

(Edited: the first version of this comment lost three inline code spans to a shell quoting mistake on my side.)

[wborn]

AI review performed before manual maintainer review.

The change correctly addresses the false positive caused by additive persistence configurations, and the added health-check coverage is useful.

There are two correctness issues in the new coverage logic that should be addressed before merging:

1. hasStoreStrategy() currently treats any strategy other than restoreOnStartup and forecast as storing data. Generic named strategies can exist without actually being executed by PersistenceManagerImpl, so such a strategy could incorrectly suppress the warning.
2. Coverage can itself be additive across multiple store configurations. The current implementation requires a single other configuration to cover all selectors, so a restore configuration such as ItemA, ItemB is still reported when ItemA and ItemB are provably stored by two separate entries.

There is also a minor maintainability concern with isStructurallyEqual() treating every otherwise unknown PersistenceConfig subtype as structurally equal.

The CI checks are green.

[wborn]

AI found no further issues in the current changes. The previous review findings have been addressed.

The current CI workflow for this revision still needs to run before merge. A human maintainer should still review the PR before merge.

As a minor housekeeping item, the PR description should be updated to reflect the revised store-strategy detection and the additional tests added in d8f431a.

AI found no further blocking issues in the current changes. A human maintainer review is still needed before merge.

[ML19821]

While re-reviewing the coverage logic against PersistenceManagerImpl, I found two more gaps in isStoredByAnotherConfig(). I will push one additional commit on top of the current head addressing both; no changes to the existing commits.

Filtered entries counted as coverage. The store-entry candidates were filtered on strategy and exclude-freedom, but not on filters(). At runtime PersistenceManagerImpl.storeItem() drops every write for which a filter returns false, and a filter like PersistenceEqualsFilter can veto every single write — whether it does is not decidable from the configuration. Since the whole point of this check is that coverage must be provable from the configuration alone, a store entry carrying filters no longer counts. Trade-off, stated openly: a restore-only entry whose items are stored only by an entry with a harmless filter (say, discarding one noise value) is warned about again. That restores the status quo before this PR rather than introducing a regression, and it keeps the check failing in the safe direction — a spurious warning over a silenced real one.

Order of the all-items shortcut and the exclude-only guard. The early return for a * store entry came before the guard that an entry selecting nothing positively (only excludes) is never covered. So {items: ["!ItemX"], strategies: [restoreOnStartup]} was still suppressed whenever an all-items store entry existed — contradicting what the previous commit set out to do. The guard now runs before the shortcut.

One test per fix, and both fail against the previous head (d8f431a) without the fix, so they actually detect the two behaviours rather than merely passing alongside them. Full module test run is green (48 tests, 26 in PersistenceResourceTest).

[mherwege]

• Store strategies are identified positively, not by exclusion: only everyUpdate, everyChange and cron strategies count, because those are the ones PersistenceManagerImpl actually acts on (schedulePersistJobs() turns cron strategies into ordinary persist jobs). A file-based configuration may name an arbitrary strategy, which PersistenceModelManager turns into a plain PersistenceStrategy instance that nothing ever executes — treating "neither restore nor forecast" as storing would let such a strategy silence a warning that should fire.

That should be covered at the source, when parsing the DSL. I had created a PR #5775 for that.

[mherwege]

Did you look into adding extra public methods to PersistenceManager? There are already private methods to get all items for a strategy and to check if a configuration applies to an item. That may enable to do a real and precise check, at the cost of iterating all items with RestoreOnStartup.

[mherwege]

Filtered entries counted as coverage. The store-entry candidates were filtered on strategy and exclude-freedom, but not on filters(). At runtime PersistenceManagerImpl.storeItem() drops every write for which a filter returns false, and a filter like PersistenceEqualsFilter can veto every single write — whether it does is not decidable from the configuration. Since the whole point of this check is that coverage must be provable from the configuration alone, a store entry carrying filters no longer counts. Trade-off, stated openly: a restore-only entry whose items are stored only by an entry with a harmless filter (say, discarding one noise value) is warned about again. That restores the status quo before this PR rather than introducing a regression, and it keeps the check failing in the safe direction — a spurious warning over a silenced real one.

I am not convinced this should be covered in the checks. After all, if an item on has a everyChange strategy and never changes, it may not end in persistence either. And nothing to restore may be the right answer then.
There is another case that is missed, but impossible to validate. If someone uses a persistence extension to manually store, there may be values without a store strategy. And that can be OK.

[ML19821]

Thanks — both points taken.

You are right that the check is and stays a heuristic on intent: an everyChange item that never changes stores nothing either, and values written through a persistence extension from rules are invisible to any configuration-level check. The reason I singled out filters is narrower than it may have looked: with a filter, the configuration itself can veto every write even while change events do occur — it is the one case where the configuration contradicts the coverage assumption, rather than the world merely staying quiet. But I agree that does not make the check precise, only conservative — and I am happy to drop that clause again if you prefer intent to win here; the guard-order fix in the same commit is independent of it.

On your suggestion: exposing the manager's own applies-to logic would make the coverage computation precise, at the cost of iterating the items. Following that thought one step further: for restoreOnStartup specifically, the most direct check would be empirical — ask the configured persistence service whether a persisted value actually exists for each item carrying the strategy. That answers exactly the question this warning is about ("can the next restore work"), and it happens to cover both cases above: the never-changing item is rightly reported (there is genuinely nothing to restore), and manual stores through extensions become visible instead of unverifiable. Costs, stated openly: one query per restore item, and a freshly configured system warns until the first values arrive.

Happy to implement whichever direction you prefer — the manager-based precise coverage, the empirical stored-value check, or keeping the current conservative config-only shape — in this PR or as a follow-up.

[mherwege]

The reason I singled out filters is narrower than it may have looked: with a filter, the configuration itself can veto every write even while change events do occur — it is the one case where the configuration contradicts the coverage assumption, rather than the world merely staying quiet.

I understand that. Still, filters serve a different purpose then defining which item will be persisted. Its purpose is to filter what changes for an item state gets persisted. I don’t think we should overload the rules for the check as it will make it harder to understand why a warning is there in the first place.

On your suggestion: exposing the manager's own applies-to logic would make the coverage computation precise, at the cost of iterating the items. Following that thought one step further: for restoreOnStartup specifically, the most direct check would be empirical — ask the configured persistence service whether a persisted value actually exists for each item carrying the strategy. That answers exactly the question this warning is about ("can the next restore work"), and it happens to cover both cases above: the never-changing item is rightly reported (there is genuinely nothing to restore), and manual stores through extensions become visible instead of unverifiable. Costs, stated openly: one query per restore item, and a freshly configured system warns until the first values arrive.

My fear would be this is taken is too far from the potential configuration issue. It gets harder to find the consequence. After all, something might be wrong with the database itself as well. I think we should remain focussed on the item and strategy configuration side. Someone that manually persists (or even puts some values directly in the DB) would understand the warning may not apply to him.
By the way, there is a REST endpoint to get the information from the persistence service (getItemInfo) that should tell you if anything is persisted already. I did some cleanup to it recently.
I think the check should just look at the configuration, not if anything is actually already persisted. Anyway, a persistence call for 1000 items is most likely much more costly than iterating for all 1000 items over the configuration.

[ML19821]

Done — the filter clause and its test are dropped (bc5103b); the check looks at items and strategies only, as you suggested. The guard-order fix stays. 25/25 tests in PersistenceResourceTest (47 in the module).

And thanks for the getItemInfo pointer — agreed that the check should stay on the configuration side; I'll leave the empirical idea out of this PR.