Repository navigation
fix(serviceMgr): change filter never fires for boolean-valued signals - #209
Merged
Merged
Conversation
evaluateChangeFilter decided whether compareValues should use the
'bool' or 'number' datatype branch by calling utils.IsBoolean on the
filter's diff parameter, not on the signal's own value. Per VISS
CORE's Change Filter Operation, a boolean signal's diff is
conventionally the numeric-looking string "0" (compareValues' bool
branch itself requires diff=="0"), so IsBoolean(diff) was always
false for a real boolean-signal subscription. That forced datatype to
"number", and compareValues then tried
strconv.ParseFloat("true"/"false", ...), which always fails and
silently returns false -- so a change-filter subscription on any
boolean path (e.g. Vehicle.Cabin.Door.Row1.DriverSide.IsOpen) never
fired a notification, regardless of logic-op (gt/lt/ne/eq).
This regressed in commit 9714661 ("Subscribe change/range
refactoring"), which replaced the previous utils.AnalyzeValueType
call on the signal's own latestValue (which correctly detected bool)
with the IsBoolean(diff) check.
Fix: classify datatype from the signal's current/latest value instead
of from diff.
Adds TestEvaluateChangeFilter_BoolWithConformantDiff, exercising the
exact gt/lt/ne/eq scenarios from the bug report with the
VISS-conformant diff="0", which the existing test suite did not
cover (the one prior bool-oriented test, TestEvaluateChangeFilter_
BoolNe, used an unrealistic diff="false" and asserted nothing on the
result).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Subscribing to a boolean-valued signal with a "change" filter never fires a subscription event, e.g.:
This was tested with
logic-opvaluesgt,ne, andltagainst a feeder (feederv4) simulating the signal fromtripdata.jsonwith alternatingtrue/falsevalues -- the underlying data does change, but no events are ever received, for any logic-op.Root cause
evaluateChangeFilter(server/vissv2server/serviceMgr/serviceMgr.go) decides whethercompareValuesshould use its"bool"or"number"comparison branch by callingutils.IsBoolean()on the filter'sdiffparameter, instead of on the signal's own value:Per VISS CORE's Change Filter Operation, a boolean signal's
diffis conventionally the numeric-looking string"0"--compareValues'"bool"branch itself requiresdiff == "0". Soutils.IsBoolean("0")is alwaysfalsefor a real boolean-signal subscription,datatypeis always forced to"number", andcompareValuesthen triesstrconv.ParseFloat("true"/"false", ...), which always fails and silently returnsfalse-- so every change-filter subscription on a boolean path never fires, regardless oflogic-op.This regressed in commit
9714661("Subscribe change/range refactoring"), which replaced a previousutils.AnalyzeValueType()call on the signal's own value (which correctly detectedbool) with theIsBoolean(diff)check above.The existing test suite did not catch this:
TestCompareValues_Bool*tests callcompareValuesdirectly withdatatype="bool"hardcoded, bypassing the buggy classification inevaluateChangeFilterentirely; and the one test that did go throughevaluateChangeFilterwith a boolean signal (TestEvaluateChangeFilter_BoolNe) used an unrealisticdiff="false"(which happens to makeIsBoolean(diff)true, masking the bug) and asserted nothing about the result (_ = ok // just ensure no panic).Fix
Classify
datatypefrom the signal'scurrent/latestvalue instead of fromdiff:Testing
TestEvaluateChangeFilter_BoolNeto actually assert on the outcome (it previously asserted nothing).TestEvaluateChangeFilter_BoolWithConformantDiff, a table-driven regression test coveringgt/lt/ne/eqwith the VISS-conformantdiff="0"ontrue/falsetransitions -- exactly the bug-report scenario. All 10 subcases pass with the fix (and would have failed before it).go test ./server/vissv2server/serviceMgrpasses in full.go vet ./server/vissv2server/...passes.