UNOMI-883: Add condition validation and type resolution#773
Merged
Conversation
Introduce ConditionValidationService and TypeResolutionService with ParserHelper and evaluator/query-builder wiring so parent conditions resolve correctly in Karaf. Includes standardized condition JSON, null-safe evaluators, and integration-test fixes.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR backports UNOMI-883 from unomi-3-dev, introducing centralized condition validation and type resolution, plus invalid-object tracking, to make rule/segment condition handling more robust across Karaf runtime evaluation, query building, and indexing.
Changes:
- Added
TypeResolutionService+ implementation to resolve condition/action/value types and track invalid objects (with a new dev shell command to list them). - Added
ConditionValidationService+ value-type validators and wired validator discovery via OSGi Blueprint (built-ins + plugin-provided validators). - Updated rule/segment/query flows and baseplugin condition metadata/evaluators to use effective-condition resolution, validation, and more null-safe comparisons.
Reviewed changes
Copilot reviewed 58 out of 62 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/shell-dev-commands/src/main/java/org/apache/unomi/shell/dev/commands/ListInvalidObjects.java | Adds Karaf dev command to print tracked invalid objects from type resolution. |
| tools/shell-dev-commands/pom.xml | Adjusts provided dependency metadata. |
| services/src/test/java/org/apache/unomi/services/TestHelper.java | Wires built-in validators into test-created DefinitionsServiceImpl and adds helper factory. |
| services/src/test/java/org/apache/unomi/services/impl/TypeResolutionServiceImplTest.java | Adds unit tests for type resolution + invalid tracking. |
| services/src/test/java/org/apache/unomi/services/impl/TestConditionEvaluators.java | Extends test parameter creation to include new validation metadata. |
| services/src/test/java/org/apache/unomi/services/impl/rules/TestSetEventOccurrenceCountAction.java | Validates nested condition when present. |
| services/src/main/resources/OSGI-INF/blueprint/blueprint.xml | Wires built-in validators and dynamic plugin validators into Definitions/Validation services. |
| services/src/main/java/org/apache/unomi/services/impl/validation/validators/AbstractNumericValueTypeValidator.java | Adds base numeric validator implementation. |
| services/src/main/java/org/apache/unomi/services/impl/validation/validators/StringValueTypeValidator.java | Adds string validator. |
| services/src/main/java/org/apache/unomi/services/impl/validation/validators/IntegerValueTypeValidator.java | Adds integer validator. |
| services/src/main/java/org/apache/unomi/services/impl/validation/validators/LongValueTypeValidator.java | Adds long validator. |
| services/src/main/java/org/apache/unomi/services/impl/validation/validators/FloatValueTypeValidator.java | Adds float validator. |
| services/src/main/java/org/apache/unomi/services/impl/validation/validators/DoubleValueTypeValidator.java | Adds double validator. |
| services/src/main/java/org/apache/unomi/services/impl/validation/validators/BooleanValueTypeValidator.java | Adds boolean validator. |
| services/src/main/java/org/apache/unomi/services/impl/validation/validators/DateValueTypeValidator.java | Adds date validator (legacy + modern Java time types). |
| services/src/main/java/org/apache/unomi/services/impl/validation/validators/ComparisonOperatorValueTypeValidator.java | Adds operator validator for known comparison operators. |
| services/src/main/java/org/apache/unomi/services/impl/validation/validators/ConditionValueTypeValidator.java | Adds validator for nested Condition-typed parameters. |
| services/src/main/java/org/apache/unomi/services/impl/validation/validators/NumericValueTypeValidator.java | Adds generic numeric validator (unused in blueprint wiring). |
| services/src/main/java/org/apache/unomi/services/impl/validation/ConditionValidationServiceImpl.java | Implements condition parameter validation (required/recommended/exclusive/allowed values/tags). |
| services/src/main/java/org/apache/unomi/services/impl/TypeResolutionServiceImpl.java | Implements type resolution + invalid object tracking (conditions/actions/value types). |
| services/src/main/java/org/apache/unomi/services/impl/segments/SegmentServiceImpl.java | Applies type resolution + validation when setting segment definitions and throws richer errors. |
| services/src/main/java/org/apache/unomi/services/impl/rules/RulesServiceImpl.java | Replaces ad-hoc invalid tracking with TypeResolutionService and adds validation hooks. |
| services/src/main/java/org/apache/unomi/services/impl/queries/QueryServiceImpl.java | Switches query condition preparation from resolve-only to validation (auto-resolves types). |
| services/src/main/java/org/apache/unomi/services/impl/profiles/ProfileServiceImpl.java | Switches query condition preparation to validation. |
| services/src/main/java/org/apache/unomi/services/impl/goals/GoalsServiceImpl.java | Switches query/report condition preparation to validation. |
| services/src/main/java/org/apache/unomi/services/impl/definitions/DefinitionsServiceImpl.java | Hosts internal TypeResolutionServiceImpl + ConditionValidationServiceImpl and exposes them via API. |
| services/src/main/java/org/apache/unomi/services/impl/AbstractServiceImpl.java | Switches metadata query condition preparation to validation. |
| services/pom.xml | Minor formatting change. |
| plugins/baseplugin/src/main/resources/META-INF/cxs/conditions/profilePropertyCondition.json | Adds description and parameter validation/exclusive groups; adds double value params. |
| plugins/baseplugin/src/main/resources/META-INF/cxs/conditions/pastEventCondition.json | Adds description and parameter validation/recommended/allowed values; reorders params. |
| plugins/baseplugin/src/main/resources/META-INF/cxs/conditions/nestedCondition.json | Standardizes type casing + adds required validation; adds description. |
| plugins/baseplugin/src/main/resources/META-INF/cxs/conditions/booleanCondition.json | Standardizes type casing + adds required/allowed-values validation; adds description. |
| plugins/baseplugin/src/main/resources/META-INF/cxs/conditions/eventPropertyCondition.json | Adds double value params and formatting normalization. |
| plugins/baseplugin/src/main/resources/META-INF/cxs/conditions/sessionPropertyCondition.json | Adds double value params. |
| plugins/baseplugin/src/main/resources/META-INF/cxs/conditions/notCondition.json | JSON formatting normalization. |
| plugins/baseplugin/src/main/resources/META-INF/cxs/conditions/matchAllCondition.json | JSON formatting normalization. |
| plugins/baseplugin/src/main/resources/META-INF/cxs/conditions/IdsCondition.json | JSON formatting normalization. |
| plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/conditions/PropertyConditionEvaluator.java | Hardens comparisons (null-safe numeric/date) and normalizes enum/string collection comparisons. |
| plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/conditions/NotConditionEvaluator.java | Adds null guard for sub-condition. |
| plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/conditions/IdsConditionEvaluator.java | Adds empty-ids guard (but currently changes semantics; see PR comments). |
| plugins/baseplugin/src/main/java/org/apache/unomi/plugins/baseplugin/conditions/BooleanConditionEvaluator.java | Adds null/empty guard for sub-conditions (but currently wrong for OR; see PR comments). |
| persistence-spi/src/test/java/org/apache/unomi/persistence/spi/conditions/ConditionContextHelperTest.java | Adds comprehensive tests for contextual parameter/script resolution, chains, cycles, and depth. |
| persistence-spi/src/main/java/org/apache/unomi/persistence/spi/conditions/evaluator/impl/ConditionEvaluatorDispatcherImpl.java | Restores optional DefinitionsService wiring and uses effective-condition resolution before eval. |
| persistence-spi/src/main/java/org/apache/unomi/persistence/spi/conditions/DateUtils.java | Expands date parsing to support legacy formats and modern Java time types with fallbacks. |
| persistence-spi/src/main/java/org/apache/unomi/persistence/spi/conditions/ConditionContextHelper.java | Enhances contextual parameter/script resolution (chains, cycles, depth limit) and adds type warnings. |
| persistence-opensearch/core/src/main/resources/OSGI-INF/blueprint/blueprint.xml | Wires DefinitionsService into OS query builder dispatcher via listener. |
| persistence-opensearch/core/src/main/java/org/apache/unomi/persistence/opensearch/ConditionOSQueryBuilderDispatcher.java | Resolves condition types/effective condition; adds graceful fallback behavior. |
| persistence-elasticsearch/core/src/main/resources/OSGI-INF/blueprint/blueprint.xml | Wires DefinitionsService into ES query builder dispatcher via listener. |
| persistence-elasticsearch/core/src/main/java/org/apache/unomi/persistence/elasticsearch/ConditionESQueryBuilderDispatcher.java | Resolves condition types/effective condition; adds graceful fallback behavior. |
| api/src/main/java/org/apache/unomi/api/utils/ParserHelper.java | Adds effective-condition resolution utilities and delegates some resolution to TypeResolutionService. |
| api/src/main/java/org/apache/unomi/api/services/ValueTypeValidator.java | Adds public SPI for value-type validators. |
| api/src/main/java/org/apache/unomi/api/services/TypeResolutionService.java | Adds public API for type resolution + invalid object tracking. |
| api/src/main/java/org/apache/unomi/api/services/InvalidObjectInfo.java | Adds API model for detailed invalid object info. |
| api/src/main/java/org/apache/unomi/api/services/DefinitionsService.java | Deprecates resolveConditionType and exposes new resolution/validation services. |
| api/src/main/java/org/apache/unomi/api/services/ConditionValidationService.java | Adds public API for condition validation and error modeling. |
| api/src/main/java/org/apache/unomi/api/Parameter.java | Adds validation metadata to parameters and YAML export. |
| api/src/main/java/org/apache/unomi/api/exceptions/BadSegmentConditionException.java | Adds constructors and class-level documentation. |
| api/src/main/java/org/apache/unomi/api/conditions/ConditionValidation.java | Adds model for parameter validation metadata and YAML output. |
| api/pom.xml | Removes explicit SLF4J dependency versions (inherits from parent). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- BooleanConditionEvaluator: empty OR returns false, not true - IdsConditionEvaluator: null-safe unboxing for match parameter - ConditionValidationServiceImpl: null guard in unbindValidator - ComparisonOperatorValueTypeValidator: LinkedHashSet for stable description order - TypeResolutionServiceImplTest: relax timestamp assertions to >= to avoid CI flakiness
…review findings - New BooleanConditionEvaluatorTest (11 tests, Mockito same() for identity matching) - New IdsConditionEvaluatorTest (9 tests) - ParserHelper regression guard: context must use putAll so parameter:: refs resolve - Add mockito-core test dependency to baseplugin - Improve TypeResolutionServiceImplTest with real assertions - Add @nested to PartialValidationTests in ConditionValidationServiceImplTest - Fix TOCTOU in TypeResolutionServiceImpl.markValid() via ConcurrentHashMap.compute() - Gate segment validation on isEnabled() in SegmentServiceImpl - Null guard for subConditionType in ConditionValidationServiceImpl - Delete dead NumericValueTypeValidator (no references, duplicate of abstract base)
…y, error handling) - InvalidObjectInfo: replace volatile int/long with AtomicInteger/AtomicLong and synchronize updateEncounter() to prevent data races under concurrent evaluation - ConditionContextHelper: throw ExceptionInInitializerError on missing fold-mapping file instead of silently degrading; fix French error message - TypeResolutionServiceImpl: WARN log in resolveValueType on null lookup; WARN in resolveActionType null guard; throw IAE in setDefinitionsService(null); remove two restatement inline comments - ConditionValidationServiceImpl: unknown validator type now WARN+skip (backward compat) instead of INVALID_VALUE error; null-guard getValueTypeId() in unbindValidator to prevent NPE in OSGi lifecycle - ConditionValidation: make customType transient for OSGi cross-bundle serialization - ConditionValidationService: rename conditionTypeName -> conditionTypeId (field stores machine ID); keep deprecated getConditionTypeName() alias - ParserHelper: WARN log when getParentChain returns null; add deprecated single-arg resolveConditionEventTypes overload for API compatibility - Tests: concurrent updateEncounter safety, setDefinitionsService IAE, resolveValueType null, AND/OR short-circuit verify(never()), null subConditionType, list contextual param skip, unbindValidator null typeId, built-in validator survives unbind
…oncurrency, error propagation) A second review of PR #773 identified several issues introduced or exposed by the new condition validation infrastructure: Behavioral regressions: - SegmentServiceImpl: restore persistenceService.isValidCondition() guard that was removed when validation was moved to ConditionValidationService. Without it, conditions that pass type-metadata validation but produce invalid ES/OS queries are now silently accepted and fail at query time. - ProfileServiceImpl.doSearch(): restore the gate that skips persistence queries when the condition type could not be resolved. The old code returned no results; the new code was passing unresolved conditions to the persistence layer and causing query errors. Concurrency: - TypeResolutionServiceImpl.markInvalid(): replace get/put sequence with putIfAbsent to eliminate the TOCTOU window where two concurrent callers for the same object ID could both see null and overwrite each other, losing encounter data and emitting duplicate WARN logs. Semantics: - TypeResolutionServiceImpl.resolveActions(): null or empty actions on a rule are a structural error, not a missing-plugin error. Only set missingPlugins=true when action type IDs are present but unresolvable, so rules with structural problems are not retried as if their plugins are simply not yet loaded. Error propagation: - ConditionValidationServiceImpl.validate(): replace IllegalStateException with WARN + empty list when builtInValidators is null, so read-path callers (GoalsService, ProfileService, QueryService) do not throw during the startup window before Blueprint wiring completes. - ConditionValidationServiceImpl.unbindValidator(): null-guard each built-in validator's getValueTypeId() in the stream to prevent NPE when any built-in validator returns null from that method. - ConditionContextHelper: Map and List branches now consistently return RESOLUTION_ERROR on failure (Map was returning null, masking the error). getContextualCondition() intercepts RESOLUTION_ERROR before the unchecked Map cast to convert it to null for callers. PastEventCondition ES/OS query builders now guard against a null result from getContextualCondition() and throw IllegalArgumentException rather than silently adding null to the sub-conditions list. - ConditionEvaluatorDispatcherImpl: elevate unresolved condition type log from DEBUG to WARN so operators see it in default logging configuration. - GoalsServiceImpl: log WARN when validate() returns issues rather than silently discarding the result. Tests: add concurrent read-side test for InvalidObjectInfo, assertSame for markInvalid idempotent-insert, and validate that an uninitialized ConditionValidationService returns empty list instead of throwing.
…rollback, null guards, tests) - Fix double-validation of nested condition parameters (H2): skip bottom-loop re-validation for parameters already handled by the typed-parameter loop - Fix parent conditionType not rolled back on child resolution failure (M1) - Guard validate() calls behind null check on conditionValidationService (M3) in SegmentServiceImpl, GoalsServiceImpl, and ProfileServiceImpl - Resolve condition types via ParserHelper before calling validate() (H1) to avoid relying on the optional TypeResolutionService OSGi wiring - Guard ConditionContextHelper.loadMappingFile() against null InputStream (M4) - Guard setBuiltInValidators() and getParameters() against null inputs (M2, M5) - Warn and skip allowedValues check for non-String parameter values (L1) - Add TC1-TC8 covering all changed paths across four test classes
…ull safety, tests) - InvalidObjectInfo: replace ArrayList/HashSet with CopyOnWriteArrayList/Set so reads are safe during concurrent updateEncounter calls (no more synchronized needed) - TypeResolutionServiceImpl: use ConcurrentHashMap.newKeySet() for atomic log-spam suppression; mark definitionsService volatile; use SLF4J parameterized logging in markInvalid; roll back all newly-resolved sibling conditions on child failure - ConditionOSQueryBuilderDispatcher.buildFilter: add missing parent-condition fallback (was present in ES counterpart but absent in OS, causing divergent behaviour) - ParserHelper.resolveEffectiveCondition: return null on cycle/max-depth failure instead of silently returning the unresolved original; all callers (ES/OS buildFilter+count, evaluator dispatcher) updated with explicit null guards - Both query-builder dispatchers: include conditionTypeId+queryBuilderKey in the "no matching query builder" WARN log - ConditionEvaluatorDispatcherImpl: outer catch now logs conditionTypeId and itemId - BooleanConditionEvaluator: guard subConditions cast with instanceof check; throw IllegalArgumentException on type mismatch instead of hiding it with @SuppressWarnings - Tests: add successful-eval dispatch test; add resolveRule(null), empty-actions, null-conditionTypeId, getAllInvalidObjectIds coverage; add untyped nested-condition validation test; fix weak || assertion to && in ConditionValidationServiceImplTest
- Thread safety: replace CopyOnWriteArrayList with CopyOnWriteArraySet in InvalidObjectInfo to eliminate TOCTOU on concurrent addAll() - TypeResolutionServiceImpl: fix TOCTOU on unresolved type logging, walk sub-condition tree to report leaf type ID (not root) in error messages, separate structural vs. missing-plugin action errors for missingPlugins flag - ConditionValidationServiceImpl: emit MISSING_RECOMMENDED_PARAMETER advisory when a required parameter uses a contextual reference; fix duplicate local variable declaration (compilation error) - ES/OS dispatchers: replace matchAll fallbacks with matchNone in buildFilter() and log null contextualCondition at WARN level - ConditionEvaluatorDispatcherImpl: remove dead secondary parent-chain walk that could cause unbounded recursion - Tests: add cycle, max-depth, sibling-rollback, concurrent-dedup, null-actions, and leaf-ID tests to TypeResolutionServiceImplTest; add parent-chain-cycle test to ConditionEvaluatorDispatcherImplTest; add IAE test to BooleanConditionEvaluatorTest; update ConditionValidationServiceImplTest to reflect new advisory behaviour - LogCheckerTest: fix flaky timing assertion by adding JVM warmup and raising threshold from 1ms to 50ms
- cancelScheduledTasks(), cleanupStaleNodes(), updateSystemStats() promoted from private to package-private so tests can call them directly - testCleanupStaleNodes: call cancelScheduledTasks() before creating nodes to prevent the background cleanup task (initialDelay=0) from racing with the precondition assertions and deleting the stale node first - testGetClusterNodesReturnsAllNodes: call updateSystemStats() directly - Remove java.lang.reflect.Method import (no more reflection in the test)
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.
Summary
Backport PR C from
unomi-3-dev: condition validation, type resolution, and condition standardization (without tracing — deferred to UNOMI-873 / PR D).ConditionValidationService,TypeResolutionService, and value-type validators; wire them in the services OSGi blueprint.ParserHelperandConditionContextHelperfor effective-condition resolution, parameter validation, and parent-chain handling.DefinitionsServicewiring on ES/OS query-builder dispatchers, parent fallbacks in evaluators/query builders, andTypeResolutionService-based rule indexing inRulesServiceImpl.unomi:list-invalid-objectsdev shell command and unit tests (ParserHelper, ConditionContextHelper, TypeResolutionService, ConditionValidationService).Intentionally out of scope for this PR: tracing/explain (PR D),
plugins/past-event→advanced-conditionsmodule move (PR C2), remaining UNOMI-881 service unit tests that depend on tracing (PR E).Test plan
mvn -pl api,persistence-spi,services,plugins/baseplugin,persistence-elasticsearch/core,persistence-opensearch/core,tools/shell-dev-commands -am testmvn -pl itests test(integration tests pass locally)unomi:list-invalid-objectslists unresolved items in Karaf