sergehuber commented on code in PR #775:
URL: https://github.com/apache/unomi/pull/775#discussion_r3428515420
##########
services/src/main/java/org/apache/unomi/services/impl/rules/RulesServiceImpl.java:
##########
@@ -677,10 +683,29 @@ protected void setRule(Rule rule, boolean
allowInvalidRules) {
}
}
-
if (rule.getCondition() != null) {
+ // Start validation operation in tracer
+ if (tracerService != null) {
+ RequestTracer tracer = tracerService.getCurrentTracer();
+ if (tracer != null && tracer.isEnabled()) {
+ tracer.startOperation("rule-condition-validation",
"Validating rule condition: " + rule.getItemId(), rule.getCondition());
+ }
+ }
+
+ // Validate condition (skips parameters with references/scripts)
+ // Validation service will auto-resolve types if needed
List<ValidationError> validationErrors =
definitionsService.getConditionValidationService().validate(rule.getCondition());
+ // Add validation info to tracer
+ if (tracerService != null) {
+ RequestTracer tracer = tracerService.getCurrentTracer();
+ if (tracer != null && tracer.isEnabled()) {
+ tracer.addValidationInfo(validationErrors,
"rule-condition-validation");
+ tracer.endOperation(!validationErrors.isEmpty(),
String.format("Rule validation completed with %d errors",
validationErrors.size()));
+ }
Review Comment:
Fixed: Changed `!validationErrors.isEmpty()` to `validationErrors.isEmpty()`
in the `endOperation` call.
##########
services/src/main/java/org/apache/unomi/services/impl/goals/GoalsServiceImpl.java:
##########
@@ -188,8 +198,119 @@ public void setGoal(Goal goal) {
LOGGER.warn("Trying to save null goal, aborting...");
return;
}
- ParserHelper.resolveConditionType(definitionsService,
goal.getStartEvent(), "goal "+goal.getItemId()+" start event");
- ParserHelper.resolveConditionType(definitionsService,
goal.getTargetEvent(), "goal "+goal.getItemId()+" start event");
+ boolean isValid = true;
+ if (goal.getStartEvent() != null) {
+ // Resolve condition type (skips parameter resolution - happens
on-demand in query builders/evaluators)
+ TypeResolutionService typeResolutionService =
getTypeResolutionService();
+ boolean startValid = typeResolutionService != null &&
typeResolutionService.resolveCondition("goals", goal, goal.getStartEvent(),
"goal "+goal.getItemId()+" start event");
+ isValid = isValid && startValid;
+ // Start validation operation in tracer for start event
+ if (tracerService != null) {
+ RequestTracer tracer = tracerService.getCurrentTracer();
+ if (tracer != null && tracer.isEnabled()) {
+ tracer.startOperation("goal-start-event-validation",
"Validating goal start event: " + goal.getItemId(), goal.getStartEvent());
+ }
+ }
+
+ // Validate condition (skips parameters with references/scripts)
+ // Validation service will auto-resolve types if needed
+ List<ValidationError> validationErrors =
definitionsService.getConditionValidationService().validate(goal.getStartEvent());
+
+ // Add validation info to tracer
+ if (tracerService != null) {
+ RequestTracer tracer = tracerService.getCurrentTracer();
+ if (tracer != null && tracer.isEnabled()) {
+ tracer.addValidationInfo(validationErrors,
"goal-start-event-validation");
+ tracer.endOperation(!validationErrors.isEmpty(),
String.format("Goal start event validation completed with %d errors",
validationErrors.size()));
+ }
Review Comment:
Fixed: Changed `!validationErrors.isEmpty()` to `validationErrors.isEmpty()`
for the start-event validation block.
##########
services/src/main/java/org/apache/unomi/services/impl/goals/GoalsServiceImpl.java:
##########
@@ -188,8 +198,119 @@ public void setGoal(Goal goal) {
LOGGER.warn("Trying to save null goal, aborting...");
return;
}
- ParserHelper.resolveConditionType(definitionsService,
goal.getStartEvent(), "goal "+goal.getItemId()+" start event");
- ParserHelper.resolveConditionType(definitionsService,
goal.getTargetEvent(), "goal "+goal.getItemId()+" start event");
+ boolean isValid = true;
+ if (goal.getStartEvent() != null) {
+ // Resolve condition type (skips parameter resolution - happens
on-demand in query builders/evaluators)
+ TypeResolutionService typeResolutionService =
getTypeResolutionService();
+ boolean startValid = typeResolutionService != null &&
typeResolutionService.resolveCondition("goals", goal, goal.getStartEvent(),
"goal "+goal.getItemId()+" start event");
+ isValid = isValid && startValid;
+ // Start validation operation in tracer for start event
+ if (tracerService != null) {
+ RequestTracer tracer = tracerService.getCurrentTracer();
+ if (tracer != null && tracer.isEnabled()) {
+ tracer.startOperation("goal-start-event-validation",
"Validating goal start event: " + goal.getItemId(), goal.getStartEvent());
+ }
+ }
+
+ // Validate condition (skips parameters with references/scripts)
+ // Validation service will auto-resolve types if needed
+ List<ValidationError> validationErrors =
definitionsService.getConditionValidationService().validate(goal.getStartEvent());
+
+ // Add validation info to tracer
+ if (tracerService != null) {
+ RequestTracer tracer = tracerService.getCurrentTracer();
+ if (tracer != null && tracer.isEnabled()) {
+ tracer.addValidationInfo(validationErrors,
"goal-start-event-validation");
+ tracer.endOperation(!validationErrors.isEmpty(),
String.format("Goal start event validation completed with %d errors",
validationErrors.size()));
+ }
+ }
+
+ // Separate errors and warnings
+ List<ValidationError> errors = validationErrors.stream()
+ .filter(error -> error.getType() !=
ValidationErrorType.MISSING_RECOMMENDED_PARAMETER)
+ .collect(Collectors.toList());
+
+ List<ValidationError> warnings = validationErrors.stream()
+ .filter(error -> error.getType() ==
ValidationErrorType.MISSING_RECOMMENDED_PARAMETER)
+ .collect(Collectors.toList());
+
+ // Log warnings but don't block the operation
+ if (!warnings.isEmpty()) {
+ StringBuilder warningMessage = new StringBuilder("Goal start
event has warnings:");
+ for (ValidationError warning : warnings) {
+ warningMessage.append("\n- ").append(warning.getMessage());
+ }
+ LOGGER.warn(warningMessage.toString());
+ }
+
+ // Only throw exception for actual errors
+ if (!errors.isEmpty()) {
+ StringBuilder errorMessage = new StringBuilder("Invalid goal
start event:");
+ for (ValidationError error : errors) {
+ errorMessage.append("\n- ").append(error.getMessage());
+ }
+ if (typeResolutionService != null) {
+ typeResolutionService.markInvalid("goals",
goal.getItemId(), "Start event validation errors: " + errorMessage.toString());
+ }
+ throw new IllegalArgumentException(errorMessage.toString());
+ }
+ }
+ if (goal.getTargetEvent() != null) {
+ // Resolve condition type (skips parameter resolution - happens
on-demand in query builders/evaluators)
+ TypeResolutionService typeResolutionService =
getTypeResolutionService();
+ boolean targetValid = typeResolutionService != null &&
typeResolutionService.resolveCondition("goals", goal, goal.getTargetEvent(),
"goal "+goal.getItemId()+" target event");
+ isValid = isValid && targetValid;
+ // Start validation operation in tracer for target event
+ if (tracerService != null) {
+ RequestTracer tracer = tracerService.getCurrentTracer();
+ if (tracer != null && tracer.isEnabled()) {
+ tracer.startOperation("goal-target-event-validation",
"Validating goal target event: " + goal.getItemId(), goal.getTargetEvent());
+ }
+ }
+
+ // Validate condition (skips parameters with references/scripts)
+ // Validation service will auto-resolve types if needed
+ List<ValidationError> targetValidationErrors =
definitionsService.getConditionValidationService().validate(goal.getTargetEvent());
+
+ // Add validation info to tracer
+ if (tracerService != null) {
+ RequestTracer tracer = tracerService.getCurrentTracer();
+ if (tracer != null && tracer.isEnabled()) {
+ tracer.addValidationInfo(targetValidationErrors,
"goal-target-event-validation");
+ tracer.endOperation(!targetValidationErrors.isEmpty(),
String.format("Goal target event validation completed with %d errors",
targetValidationErrors.size()));
+ }
Review Comment:
Fixed: Changed `!targetValidationErrors.isEmpty()` to
`targetValidationErrors.isEmpty()` for the target-event validation block.
##########
persistence-spi/src/main/java/org/apache/unomi/persistence/spi/conditions/ConditionContextHelper.java:
##########
@@ -204,17 +219,19 @@ private static Object parseParameterWithValidation(
* @param value the value to resolve
* @param scriptExecutor executor for script expressions
* @param parameterDefs optional parameter definitions for validation
+ * @param tracerService optional tracer service for warnings
* @param conditionTypeId condition type ID for context
* @param resolutionChain set of parameter keys/scripts already being
resolved (for cycle detection)
* @param depth current resolution depth
- * @return resolved value, or {@code RESOLUTION_ERROR} sentinel if cycle
detected or max depth exceeded
+ * @return resolved value, or null if cycle detected or max depth exceeded
*/
Review Comment:
The `RESOLUTION_ERROR` sentinel is an internal implementation detail of
`parseParameterWithValidation`. Before returning to public callers,
`getContextualCondition` converts it to `null` (via the `if (rawValues == null
|| rawValues == RESOLUTION_ERROR) { return null; }` guard). So the public
Javadoc correctly documents the `null` return for cycle/depth errors. Agreed
the private-method Javadoc could be more explicit about the sentinel — will
track as a separate documentation cleanup.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
