HYPERFLEET-390 | refactor: refator evaluator to support jsonPATH and cel expression. #20
Conversation
WalkthroughThis PR adds unified value extraction and capture modes across config loading, validation, criteria evaluation, and executors. It introduces ValueDef and extends CaptureField to support either a JSONPath field or a CEL expression (mutually exclusive), adds ParseValueDef, implements ExtractField and ExtractValue with FieldResult/ExtractValueResult types, and enforces capture validation (name + exactly one of field/expression, optional CEL compile). Executors (precondition and post-action) use ParseValueDef/ExtractValue; legacy nested-field traversal and several CEL helper functions were removed. Tests and README/docs updated accordingly. Sequence Diagram(s)sequenceDiagram
participant ConfigLoader as Config Loader
participant Validator as Validator
participant Executor as Executor
participant Evaluator as Evaluator
participant FieldExtractor as Field Extractor
participant CELEvaluator as CEL Evaluator
ConfigLoader->>Validator: provide parsed config (CaptureField, ValueDef)
rect rgb(240,248,255)
Note over Validator: Validation Phase
Validator->>Validator: validateCaptureFields()
loop per capture
Validator->>Validator: ensure Name present
Validator->>Validator: ensure exactly one of Field or Expression
alt Expression & CEL available
Validator->>CELEvaluator: compile/validate expression
CELEvaluator-->>Validator: compile result
end
end
end
rect rgb(245,255,250)
Note over Executor: Execution (Precondition)
Executor->>Evaluator: init with API response
loop per capture
alt capture.field set
Executor->>FieldExtractor: ExtractField(response, field)
FieldExtractor-->>Executor: FieldResult{Value, Error}
else capture.expression set
Executor->>CELEvaluator: Evaluate(expression, context)
CELEvaluator-->>Executor: CELResult{Value, Error}
end
Executor->>Executor: store captured value in exec context
end
end
rect rgb(255,250,240)
Note over Executor: Post-Action / Payload Building
Executor->>Evaluator: ParseValueDef(valueDef)
Evaluator->>Evaluator: choose ExtractValue(field, expression)
alt field path
Evaluator->>FieldExtractor: ExtractField(...)
else expression path
Evaluator->>CELEvaluator: Evaluate(...)
end
Evaluator-->>Executor: ExtractValueResult{Value, Source, Error}
alt Value nil & Default present
Executor->>Executor: use Default
else
Executor->>Executor: use Value
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
internal/criteria/field_evaluator.go (1)
51-68: Inconsistent value handling betweenExecuteandFindResults.The function executes the JSONPath query twice: first with
Execute()(storing a string inresult.Value), then withFindResults()to get typed values. IfFindResultsfails on line 65-68,result.Valueretains the string from line 58 whileresult.Erroris set, leading to potentially confusing state.Consider restructuring to use only
FindResultsfor value extraction:🔎 Proposed refactor
- // Execute the query to check if we get any result - var buf bytes.Buffer - if err := jp.Execute(&buf, data); err != nil { - result.Error = fmt.Errorf("failed to execute JSONPath: %w", err) - return result, nil - } - - result.Value = strings.TrimSpace(buf.String()) - if result.Value == "" { - return result, nil - } - - // For complex queries that might return multiple values, use FindResults + // Use FindResults to get typed values results, err := jp.FindResults(data) if err != nil { - result.Error = fmt.Errorf("failed to find results for '%s': %w", originalField, err) + result.Error = fmt.Errorf("failed to extract field '%s': %w", originalField, err) return result, nil }internal/criteria/types.go (1)
115-126: Consider removing duplicate method.
GetData()(lines 117-126) is functionally identical to the existingData()method (lines 210-218). Both return a shallow copy of the internal data map. This duplication could lead to maintenance burden.🔎 Proposed fix
Either remove
GetData()and useData()consistently, or have one delegate to the other:// GetData returns a shallow copy of the context data for direct field extraction. // This method is safe for concurrent use. func (c *EvaluationContext) GetData() map[string]interface{} { - c.mu.RLock() - defer c.mu.RUnlock() - // Return shallow copy to prevent external modification - result := make(map[string]interface{}, len(c.data)) - for k, v := range c.data { - result[k] = v - } - return result + return c.Data() }internal/criteria/cel_evaluator_test.go (2)
164-165: Redundant assertions.Lines 164 and 165 assert the same condition with opposite boolean checks:
assert.True(t, !result.HasError())assert.False(t, result.HasError())Consider keeping only one assertion for clarity.
🔎 Proposed fix
- assert.True(t, !result.HasError()) assert.False(t, result.HasError())
175-175: Confusing double-negative assertion.
assert.False(t, !result.HasError())is equivalent toassert.True(t, result.HasError()). The latter is more readable, especially since line 174 already correctly usesassert.True(t, result.HasError()).🔎 Proposed fix
- assert.False(t, !result.HasError()) + // Line 174 already asserts HasError() is true; this line is redundantinternal/criteria/README.md (1)
137-137: Format the bare URL as a Markdown link.Static analysis flagged a bare URL. Consider formatting it as a proper Markdown link for consistency.
🔎 Suggested fix
-See: https://kubernetes.io/docs/reference/kubectl/jsonpath/ +See: [Kubernetes JSONPath Reference](https://kubernetes.io/docs/reference/kubectl/jsonpath/)internal/criteria/evaluator.go (1)
175-226: Consider handling whitespace-only field inputs.The
ExtractValuemethod trims the expression (line 199) but doesn't trim the field parameter before checking if it's empty. Iffieldis whitespace-only (e.g.," "), it will be passed toEvaluateField, which delegates toExtractField. Looking atExtractFieldin the snippet (line 27), it does trim the field, so this is handled downstream. However, for consistency and to match the expression handling, consider trimming field here as well.Also, the error message on line 225 could be clearer - "neither field nor expression defined" might confuse users if they passed whitespace strings.
🔎 Optional: Trim field for consistency
func (e *Evaluator) ExtractValue(field, expression string) (*ExtractValueResult, error) { result := &ExtractValueResult{} + field = strings.TrimSpace(field) + expression = strings.TrimSpace(expression) if expression != "" { // CEL expression mode - celResult, err := e.EvaluateCEL(strings.TrimSpace(expression)) + celResult, err := e.EvaluateCEL(expression) if err != nil {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
configs/adapter-config-template.yamlinternal/config_loader/README.mdinternal/config_loader/types.gointernal/config_loader/validator.gointernal/config_loader/validator_test.gointernal/criteria/README.mdinternal/criteria/cel_evaluator.gointernal/criteria/cel_evaluator_test.gointernal/criteria/evaluator.gointernal/criteria/evaluator_scenarios_test.gointernal/criteria/evaluator_test.gointernal/criteria/evaluator_version_test.gointernal/criteria/field_evaluator.gointernal/criteria/types.gointernal/executor/README.mdinternal/executor/post_action_executor.gointernal/executor/post_action_executor_test.gointernal/executor/precondition_executor.gotest/integration/config-loader/config_criteria_integration_test.go
💤 Files with no reviewable changes (1)
- internal/executor/post_action_executor_test.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: xueli181114
Repo: openshift-hyperfleet/hyperfleet-adapter PR: 13
File: internal/executor/post_action_executor.go:198-205
Timestamp: 2025-12-04T14:06:51.656Z
Learning: Logger refactor is tracked in HYPERFLEET-304 for the hyperfleet-adapter repository, which will address how CEL evaluation failures and similar errors are handled and logged.
📚 Learning: 2025-12-11T02:06:33.975Z
Learnt from: xueli181114
Repo: openshift-hyperfleet/hyperfleet-adapter PR: 17
File: internal/config_loader/validator_schema.go:126-133
Timestamp: 2025-12-11T02:06:33.975Z
Learning: In the hyperfleet-adapter repository (Go), environment variable validation for required parameters (those with `env.*` source, `Required=true`, and no default) occurs at config load time in `internal/config_loader/validator_schema.go`. This is intentional design - environment variables must be set before deployment, and the validation ensures fail-fast behavior with clear error messages.
Applied to files:
internal/config_loader/validator.gointernal/config_loader/validator_test.go
🧬 Code graph analysis (9)
internal/criteria/types.go (1)
internal/criteria/field_evaluator.go (1)
ExtractField(24-91)
internal/executor/post_action_executor.go (1)
internal/config_loader/types.go (1)
ParseValueDef(33-56)
internal/executor/precondition_executor.go (3)
internal/criteria/types.go (1)
NewEvaluationContext(75-80)internal/criteria/evaluator.go (1)
NewEvaluator(55-70)internal/criteria/cel_evaluator.go (1)
CELResult(26-40)
internal/config_loader/validator_test.go (4)
internal/config_loader/types.go (13)
AdapterConfig(59-64)Metadata(67-71)AdapterConfigSpec(74-82)AdapterInfo(85-87)HyperfleetAPIConfig(90-95)KubernetesConfig(98-100)Condition(189-193)Precondition(151-158)Parameter(104-111)APICall(161-169)Resource(229-236)DiscoveryConfig(239-243)CaptureField(182-186)internal/criteria/types.go (1)
Operator(10-10)internal/k8s_client/discovery.go (2)
Discovery(14-29)DiscoveryConfig(32-42)internal/config_loader/loader.go (1)
Parse(105-121)
internal/criteria/cel_evaluator.go (1)
pkg/errors/cel_error.go (1)
NewCELEvalError(67-77)
internal/criteria/evaluator_scenarios_test.go (1)
internal/criteria/types.go (1)
OperatorGreaterThan(24-24)
internal/criteria/evaluator.go (2)
internal/criteria/field_evaluator.go (2)
FieldResult(11-14)ExtractField(24-91)internal/criteria/types.go (1)
Operator(10-10)
internal/criteria/evaluator_test.go (3)
internal/criteria/field_evaluator.go (1)
ExtractField(24-91)internal/criteria/types.go (3)
OperatorEquals(14-14)OperatorGreaterThan(24-24)OperatorIn(18-18)internal/config_loader/constants.go (1)
FieldValue(72-72)
test/integration/config-loader/config_criteria_integration_test.go (1)
internal/criteria/types.go (2)
OperatorExists(28-28)OperatorGreaterThan(24-24)
🪛 markdownlint-cli2 (0.18.1)
internal/criteria/README.md
137-137: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (33)
configs/adapter-config-template.yaml (1)
115-135: LGTM!The documentation for capture modes is clear and accurately reflects the new mutual exclusivity constraint between
fieldandexpression. The examples demonstrate both JSONPath and CEL usage patterns appropriately.internal/criteria/cel_evaluator.go (1)
23-40: LGTM!The simplified
CELResultstructure withErrorinstead ofErrorReasonis cleaner. TheHasError()method provides a clear API for checking error state, and the updated comments accurately document the behavior.internal/config_loader/README.md (1)
95-118: LGTM!The documentation for
CaptureFieldis comprehensive, with clear examples showing simple dot notation, JSONPath, and CEL expression modes. The mutual exclusivity constraint is well-documented.internal/criteria/evaluator_version_test.go (1)
26-27: LGTM!The test assertions correctly use
HasError()to check for evaluation success, aligning with the simplifiedCELResultAPI that removedIsSuccess().internal/criteria/types.go (1)
130-144: LGTM!The refactored
GetNestedFieldcorrectly delegates toExtractFieldwith proper error handling. The nil check on line 137 is defensive but harmless.internal/executor/post_action_executor.go (1)
162-177: LGTM!The value definition handling is clean and well-structured:
- Properly uses
ParseValueDefto detect value definitions- Correctly handles extraction errors as parse errors (fail-fast)
- Implements default value fallback with appropriate debug logging
internal/criteria/evaluator_scenarios_test.go (1)
1-373: LGTM! Test assertions correctly updated to use structured results.All tests have been properly updated to access
result.Matchedinstead of raw boolean values, aligning with the newEvaluationResultreturn type fromEvaluateConditionandEvaluateConditions. The test coverage is comprehensive and the changes are consistent throughout.internal/config_loader/validator.go (1)
176-211: LGTM! Well-structured capture field validation.The validation logic correctly enforces:
- Name is required for each capture
- Mutual exclusivity between
fieldandexpression(exactly one must be set)- CEL expression syntax validation when an expression is provided
The implementation follows existing validation patterns in the codebase.
internal/config_loader/validator_test.go (2)
11-24: Good refactoring to programmatic config construction.The
baseConfig()helper provides a clean, minimal valid configuration that tests can modify for specific scenarios. This approach is more maintainable than large YAML blocks and enables type-safe test setup.
534-579: Comprehensive capture field validation tests.The tests properly cover all validation scenarios:
- Valid capture with field only
- Valid capture with expression only
- Invalid: both field and expression set
- Invalid: neither field nor expression set
- Invalid: missing capture name
internal/criteria/README.md (1)
101-176: Well-documented JSONPath and value extraction APIs.The new documentation sections clearly explain:
ExtractFieldfor JSONPath extraction with supported syntax tableExtractValuefor unified field/expression extraction- Error handling semantics (parse errors vs. missing fields)
FieldResultandExtractValueResultstructuresThis will help developers understand the new extraction capabilities.
internal/config_loader/types.go (2)
33-56: Consider whether silent error handling is intentional.
ParseValueDefreturns(nil, false)for all error cases (marshal failure, unmarshal failure, not a map). This makes it impossible for callers to distinguish between "not a value definition" and "malformed value definition".If malformed definitions should be caught during validation, this is fine since the caller will treat it as a literal value. However, if debugging is needed, consider logging or returning the error.
Is this silent error handling intentional? If the expectation is that validation catches structural issues elsewhere, then this is acceptable. Otherwise, consider returning a third value or logging on parse failures.
177-186: LGTM! CaptureField now supports both extraction modes.The updated
CaptureFieldstruct properly supports mutually exclusiveField(JSONPath) andExpression(CEL) modes withomitemptytags, aligning with the validator enforcement added invalidator.go.internal/executor/precondition_executor.go (2)
130-148: Evaluator creation failure silently skips all captures.When
NewEvaluatorfails (line 130-132), a warning is logged but all captures are skipped without further indication. This could lead to missing data that downstream conditions or post-actions depend on.Consider whether this should be an error that aborts the precondition, or at minimum ensure the warning message clearly indicates that all captures were skipped.
Is silently skipping all captures on evaluator creation failure the intended behavior? If captures are required for subsequent conditions, this could cause confusing failures later.
135-146: Good error handling pattern for value extraction.The distinction between parse errors (return immediately) and field-missing errors (warn and continue) is appropriate:
- Parse errors indicate configuration issues that should fail the operation
- Missing fields may be intentional and can use defaults
The debug logging of captured values with their source is helpful for troubleshooting.
internal/executor/README.md (2)
162-184: Clear documentation of capture modes.The examples effectively demonstrate all three capture approaches:
- Simple dot notation (
status.phase)- JSONPath for complex extraction (
{.items[?(@.adapter=='landing-zone-adapter')]...})- CEL expression for computed values (
items.filter(i, i.status == 'active').size())The note about mutual exclusivity between
fieldandexpressionis helpful.
283-287: Payload value types well documented.The documentation clearly explains the three payload value patterns:
- Direct string with Go template support
- Field extraction with JSONPath and optional default
- CEL expression with optional default
This aligns with the
ValueDeftype implementation.test/integration/config-loader/config_criteria_integration_test.go (3)
179-182: Appropriate handling for missing field edge case.The assertion
assert.True(t, err != nil || !result.Matched, ...)correctly handles the case where the field doesn't exist - the behavior could be either an error or a non-match depending on implementation. This defensive assertion is appropriate for integration tests that shouldn't over-specify internal behavior.
268-298: LGTM! ExtractValue and EvaluateCondition usage updated correctly.The tests properly:
- Call
ExtractValueand accessresult.Valuefor extracted data- Handle errors from extraction appropriately
- Use
EvaluateConditionwith structured result access
331-358: Good null safety test coverage.The tests verify proper error handling when:
- A resource path leads to a nil value (
clusterController: nil)- A deeply nested path has a null intermediate (
status: nil)Both cases correctly expect errors from
ExtractValue, which aligns with the documented behavior that missing fields return errors rather than nil values.internal/criteria/evaluator_test.go (10)
600-600: LGTM!The assertion correctly accesses the new
result.Matchedfield from the structuredEvaluationResultreturn type, aligning with the API refactor.
652-652: LGTM!Consistent with the API change to return
*ConditionsResultwith aMatchedfield.
658-722: LGTM!The
TestExtractFieldfunction properly tests the newExtractFieldAPI with various path scenarios. The error handling pattern on lines 711-713 correctly accounts for errors being returned either as the function error or captured inresult.Error.
724-815: Comprehensive JSONPath extraction tests.Good coverage of JSONPath features including:
- Wildcard extraction (
{.items[*].adapter})- Index access (
{.items[0].adapter})- Filter expressions (
{.items[?(@.adapter=='landing-zone-adapter')]...})- Auto-wrapping of paths without braces
This validates the new JSONPath capabilities well.
817-843: LGTM!Good convenience function tests covering simple extraction, filter expressions, and wildcard extraction patterns.
917-945: LGTM!The
TestExtractValuecorrectly validates the unified extraction interface. The test on lines 941-944 appropriately verifies that non-existent fields returnnilvalue without an error, allowing callers to apply defaults.
947-982: LGTM!Thorough tests for
EvaluateConditioncovering multiple operators and verifying the structured result fields (Matched,FieldValue,Field,Operator,ExpectedValue).
984-1028: LGTM!Good coverage of
EvaluateConditionsincluding verification of:
FailedConditionindex trackingExtractedFieldsmap population even when conditions fail- Multiple condition evaluation flow
1064-1081: LGTM!Null handling tests correctly verify the new behavior where accessing fields on null parents returns
nilvalue (not error), enabling default value fallback.
1095-1103: LGTM!Deep null path test validates that deeply nested null paths return
nilvalue without parse errors, consistent with the new extraction semantics.internal/criteria/evaluator.go (3)
95-106: LGTM!Clean delegation to
ExtractFieldwith good documentation covering both simple dot notation and JSONPath syntax examples.
108-146: LGTM!The
EvaluateConditionmethod properly structures the evaluation flow:
- Retrieves field value with error wrapping
- Builds result with all context fields populated
- Handles
OperatorExistsspecially (no expected value)- Uses the operator function map for other operators
Good error handling with the custom
EvaluationErrortype.
148-173: LGTM!The
EvaluateConditionscorrectly:
- Initializes
FailedConditionto -1 (no failure)- Records the first failed condition index
- Populates
ExtractedFieldsregardless of match result- Short-circuits the
Matchedflag but continues evaluating all conditions
xueli181114
force-pushed
the
HYPERFLEET-390
branch
from
3772d7d to
a5dd796
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/criteria/evaluator_test.go (1)
917-945: Add test coverage for missing ExtractValue scenarios.The
TestExtractValuetest only covers field extraction mode. Missing test cases for:
- CEL expression mode: Test
ExtractValue("", "expression")path- Mutual exclusivity: Test behavior when both field and expression are provided (should this error?)
- Neither parameter: Test
ExtractValue("", "")returns expected errorThese gaps should be addressed, especially test case #2 which relates to the mutual exclusivity validation issue flagged in
evaluator.go.🔎 Suggested additional test cases
// Test CEL expression mode result, err := evaluator.ExtractValue("", "cluster.metadata.name == 'test-cluster'") require.NoError(t, err) assert.NotNil(t, result.Value) // Test neither field nor expression - should error result, err = evaluator.ExtractValue("", "") assert.Error(t, err) assert.Contains(t, err.Error(), "neither field nor expression") // Test both field and expression - should error (if validation added) result, err = evaluator.ExtractValue("cluster.metadata.name", "cluster.metadata.name == 'test'") assert.Error(t, err) assert.Contains(t, err.Error(), "mutually exclusive")
🧹 Nitpick comments (1)
internal/criteria/cel_evaluator_test.go (1)
201-201: Assertion pattern is valid but slightly verbose.Using
!result.HasError()works correctly. Consider usingassert.False(t, result.HasError())for slightly clearer intent, though the current form is functionally equivalent.Also applies to: 209-209
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
configs/adapter-config-template.yamlinternal/config_loader/README.mdinternal/config_loader/types.gointernal/config_loader/validator.gointernal/config_loader/validator_test.gointernal/criteria/README.mdinternal/criteria/cel_evaluator.gointernal/criteria/cel_evaluator_test.gointernal/criteria/evaluator.gointernal/criteria/evaluator_scenarios_test.gointernal/criteria/evaluator_test.gointernal/criteria/evaluator_version_test.gointernal/criteria/field_evaluator.gointernal/criteria/types.gointernal/executor/README.mdinternal/executor/post_action_executor.gointernal/executor/post_action_executor_test.gointernal/executor/precondition_executor.gotest/integration/config-loader/config_criteria_integration_test.go
💤 Files with no reviewable changes (1)
- internal/executor/post_action_executor_test.go
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/config_loader/README.md
- internal/config_loader/types.go
- internal/criteria/README.md
- internal/criteria/field_evaluator.go
- internal/criteria/evaluator_scenarios_test.go
- configs/adapter-config-template.yaml
- internal/config_loader/validator_test.go
- internal/criteria/evaluator_version_test.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: xueli181114
Repo: openshift-hyperfleet/hyperfleet-adapter PR: 13
File: internal/executor/post_action_executor.go:198-205
Timestamp: 2025-12-04T14:06:51.656Z
Learning: Logger refactor is tracked in HYPERFLEET-304 for the hyperfleet-adapter repository, which will address how CEL evaluation failures and similar errors are handled and logged.
📚 Learning: 2025-12-11T02:06:33.975Z
Learnt from: xueli181114
Repo: openshift-hyperfleet/hyperfleet-adapter PR: 17
File: internal/config_loader/validator_schema.go:126-133
Timestamp: 2025-12-11T02:06:33.975Z
Learning: In the hyperfleet-adapter repository (Go), environment variable validation for required parameters (those with `env.*` source, `Required=true`, and no default) occurs at config load time in `internal/config_loader/validator_schema.go`. This is intentional design - environment variables must be set before deployment, and the validation ensures fail-fast behavior with clear error messages.
Applied to files:
internal/config_loader/validator.go
🧬 Code graph analysis (7)
internal/config_loader/validator.go (2)
internal/config_loader/constants.go (4)
FieldSpec(9-9)FieldPreconditions(19-19)FieldCapture(49-49)FieldExpression(51-51)internal/config_loader/types.go (1)
CaptureField(182-186)
internal/executor/post_action_executor.go (1)
internal/config_loader/types.go (1)
ParseValueDef(33-56)
internal/criteria/evaluator_test.go (3)
internal/criteria/field_evaluator.go (1)
ExtractField(24-93)internal/criteria/types.go (3)
OperatorEquals(14-14)OperatorGreaterThan(24-24)OperatorIn(18-18)internal/config_loader/constants.go (1)
FieldValue(72-72)
test/integration/config-loader/config_criteria_integration_test.go (1)
internal/criteria/types.go (2)
OperatorExists(28-28)OperatorGreaterThan(24-24)
internal/executor/precondition_executor.go (3)
internal/criteria/types.go (1)
NewEvaluationContext(75-80)internal/criteria/evaluator.go (1)
NewEvaluator(55-70)internal/criteria/cel_evaluator.go (1)
CELResult(26-40)
internal/criteria/evaluator.go (2)
internal/criteria/field_evaluator.go (2)
FieldResult(11-14)ExtractField(24-93)internal/criteria/types.go (1)
Operator(10-10)
internal/criteria/types.go (1)
internal/criteria/field_evaluator.go (1)
ExtractField(24-93)
🔇 Additional comments (23)
internal/executor/README.md (2)
162-184: Documentation updates look accurate and helpful.The capture modes documentation clearly explains the mutually exclusive
fieldvsexpressionoptions, with good examples showing simple dot notation, JSONPath, and CEL expression usage. This aligns well with the validation logic added invalidator.go.
283-287: Payload value types documentation is clear.The three value types (direct string, field extraction with default, CEL expression with default) are well-documented and match the
ParseValueDefimplementation inconfig_loader/types.go.internal/criteria/types.go (1)
120-131: Clean delegation to new ExtractField API.The refactored implementation properly handles all error cases:
- Function-level errors from
ExtractField- Nil result handling
- Result-level errors via
result.ErrorThis maintains backward compatibility while leveraging the new extraction infrastructure.
internal/criteria/cel_evaluator_test.go (1)
176-176: Test assertions correctly updated for new error representation.The assertions now use
result.Error.Error()to access error messages, aligning with the simplifiedCELResultstruct that removed theErrorReasonfield in favor of the singleErrorfield.Also applies to: 194-194
internal/executor/post_action_executor.go (1)
162-177: Value definition handling is well-structured.The new flow correctly:
- Detects value definitions via
ParseValueDef- Uses
ExtractValuefor unified field/expression extraction- Fails fast on parse errors (configuration bugs)
- Falls back to default when extraction yields nil
One consideration: the debug log at line 172-173 logs the default value. Ensure sensitive defaults aren't used in production configs, though debug-level logging is typically acceptable.
internal/config_loader/validator.go (1)
180-211: Capture field validation is comprehensive and well-implemented.The validation correctly enforces:
- Required
namefield- Mutual exclusivity of
fieldandexpression(exactly one must be set)- CEL syntax validation when
expressionis providedThe error messages with path context will help users quickly identify configuration issues.
internal/executor/precondition_executor.go (3)
130-148: Capture evaluation flow looks correct, but consider evaluator creation failure impact.When
captureEvaluatorcreation fails (line 131-132), all captures are silently skipped with only a warning log. This could mask configuration issues. Consider whether this should be a harder failure, or at minimum ensure the warning is prominent enough for operators to notice.The individual capture handling is well-designed:
- Parse errors (line 136-138) cause immediate failure (correct - configuration bug)
- Missing fields (line 140-143) log a warning and continue (correct - valid runtime scenario)
171-171: API migration to EvaluateConditions looks correct.The switch from
EvaluateConditionsWithResulttoEvaluateConditionsaligns with the broader API refactoring in this PR.
238-240: CEL error formatting correctly uses Error field.The formatting now uses
result.CELResult.Errordirectly with%v, which aligns with the removal of the separateErrorReasonfield fromCELResult.internal/criteria/cel_evaluator.go (2)
24-40: CELResult struct simplification is a good cleanup.Removing the redundant
ErrorReasonfield in favor of the singleErrorfield simplifies the API. Callers can useHasError()to check for errors andError.Error()to get the message when needed.
160-166: Error result construction is consistent with simplified struct.The evaluation error result now correctly initializes only the necessary fields. The explicit
nilforValueandfalseforMatchedmake the error state clear.test/integration/config-loader/config_criteria_integration_test.go (4)
85-89: Integration tests correctly updated for new evaluator API.The migration from
EvaluateConditionWithResult/EvaluateConditionsWithResulttoEvaluateCondition/EvaluateConditionsis consistent throughout the test file.Also applies to: 111-111
268-276: ExtractValue usage correctly accesses result.Value.The test properly uses the new
ExtractValueAPI and accesses the extracted value viaresult.Value. This aligns with theExtractValueResultstruct design.
179-182: Flexible assertion handles both error and non-match cases.The assertion
err != nil || !result.Matchedcorrectly handles the case where a missing field could either return an error or a non-matched result, depending on the implementation path. This is appropriate for testing theexistsoperator on a missing field.
331-343: Null safety tests correctly verify ExtractValue semantics.The tests verify that
ExtractValuereturns a nil value (not an error) for null paths, which allows callers to apply defaults. The condition evaluation test at line 337-342 properly checks that null paths fail evaluation.internal/criteria/evaluator.go (4)
95-106: LGTM!The
EvaluateFieldmethod is well-documented with clear examples of both simple dot notation and full JSONPath syntax. The delegation toExtractFieldis clean and maintains separation of concerns.
108-173: LGTM!The refactored
EvaluateConditionandEvaluateConditionsmethods properly aggregate evaluation results and extract field values. The new signature forEvaluateConditiontaking individual parameters (field, operator, value) improves API clarity.
175-180: LGTM!The
ExtractValueResultstructure provides a clear contract for extraction outcomes, bundling the value, source identifier, and any errors encountered.
215-220: Verify the field extraction error semantics are clearly understood.The
ExtractValuemethod has nuanced error handling: parse errors (invalid JSONPath syntax) return an error, but field-not-found conditions return a result withnilValue and no error. This allows callers to apply default values when fields are missing.While this design is documented in comments and tested, it differs from typical Go error handling patterns where "not found" might also be an error. Ensure this behavior is clearly communicated in public documentation and that callers understand when to check
result.Value == nilversuserr != nil.Are there user-facing docs or examples that explain when
ExtractValuereturns an error versus a nil Value? This distinction is critical for proper default value handling.internal/criteria/evaluator_test.go (4)
600-600: LGTM!Test correctly updated to access
result.Matchedfrom the newEvaluationResultstructure.
658-843: Excellent test coverage for field extraction!The new tests comprehensively cover:
- Simple and nested field extraction
- Full JSONPath syntax including wildcards and filters
- Array handling with single vs multiple results
- Error handling for invalid/missing fields
The dual error checking pattern (
err != nil || result.Error != nil) correctly handles both parse errors and extraction errors.
947-1028: LGTM!The tests correctly validate the refactored
EvaluateConditionandEvaluateConditionsmethods, including proper verification of:
- Individual condition matching
- Extracted field values
- Failed condition tracking
- Multiple condition aggregation
1046-1104: LGTM!The null handling tests properly validate the extraction semantics where:
- Accessing fields on null parents returns
nilValue without error (allowing defaults)- Existing fields still extract correctly
- Deep null paths are handled gracefully
These tests confirm the error handling design discussed in the
ExtractValueimplementation.
…cel expression. In this commit I - Removed useless code and the testing code - Refatored evaluator to support cel expression and jsonPATH field - Supported default value when field and cel expression match failure - Supported expression and complex field in capture
xueli181114
force-pushed
the
HYPERFLEET-390
branch
from
a5dd796 to
74b71dd
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
internal/criteria/field_evaluator.go (1)
42-56: Clarify "backward compatible" comment.Line 43 mentions "backward compatible," but since this is a new file introducing
ExtractField, the comment is misleading. Consider revising to: "AllowMissingKeys(false) ensures missing fields are treated as errors during validation."🔎 Suggested comment revision
- // AllowMissingKeys(false) ensures we get errors for missing fields (backward compatible) + // AllowMissingKeys(false) ensures missing fields produce errors during validationinternal/config_loader/types.go (1)
30-56: Consider adding mutual exclusivity validation.While
ParseValueDefvalidates that at least one offieldorexpressionis set, it doesn't enforce that both cannot be set simultaneously. This could allow invalid configurations to pass parsing, even though they're documented as mutually exclusive.🔎 Proposed fix to add mutual exclusivity check
// Must have at least one of field or expression if valueDef.Field == "" && valueDef.Expression == "" { return nil, false } + + // Enforce mutual exclusivity + if valueDef.Field != "" && valueDef.Expression != "" { + return nil, false + } return &valueDef, trueNote: If the config loader validator already enforces this constraint at configuration load time, this additional check provides defense-in-depth for runtime usage outside the config loader.
internal/executor/README.md (1)
193-211: Minor: Consider rewording to remove word repetition.The documentation is excellent and comprehensive. However, static analysis detected a minor stylistic issue on line 199 where "only" appears twice: "API Response only | Only the parsed JSON response". Consider rephrasing for better readability.
🔎 Suggested rewording
-| **Capture** (`field`/`expression`) | API Response only | Only the parsed JSON response (e.g., `status.phase`, `items[0].name`) | +| **Capture** (`field`/`expression`) | API Response data | The parsed JSON response (e.g., `status.phase`, `items[0].name`) |
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
configs/adapter-config-template.yamlinternal/config_loader/README.mdinternal/config_loader/types.gointernal/config_loader/validator.gointernal/config_loader/validator_test.gointernal/criteria/README.mdinternal/criteria/cel_evaluator.gointernal/criteria/cel_evaluator_test.gointernal/criteria/evaluator.gointernal/criteria/evaluator_scenarios_test.gointernal/criteria/evaluator_test.gointernal/criteria/evaluator_version_test.gointernal/criteria/field_evaluator.gointernal/criteria/types.gointernal/executor/README.mdinternal/executor/post_action_executor.gointernal/executor/post_action_executor_test.gointernal/executor/precondition_executor.gotest/integration/config-loader/config_criteria_integration_test.go
💤 Files with no reviewable changes (1)
- internal/executor/post_action_executor_test.go
✅ Files skipped from review due to trivial changes (1)
- internal/config_loader/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/criteria/evaluator_version_test.go
- internal/config_loader/validator.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: xueli181114
Repo: openshift-hyperfleet/hyperfleet-adapter PR: 13
File: internal/executor/post_action_executor.go:198-205
Timestamp: 2025-12-04T14:06:51.656Z
Learning: Logger refactor is tracked in HYPERFLEET-304 for the hyperfleet-adapter repository, which will address how CEL evaluation failures and similar errors are handled and logged.
📚 Learning: 2025-12-11T02:06:33.975Z
Learnt from: xueli181114
Repo: openshift-hyperfleet/hyperfleet-adapter PR: 17
File: internal/config_loader/validator_schema.go:126-133
Timestamp: 2025-12-11T02:06:33.975Z
Learning: In the hyperfleet-adapter repository (Go), environment variable validation for required parameters (those with `env.*` source, `Required=true`, and no default) occurs at config load time in `internal/config_loader/validator_schema.go`. This is intentional design - environment variables must be set before deployment, and the validation ensures fail-fast behavior with clear error messages.
Applied to files:
internal/config_loader/validator_test.gointernal/criteria/evaluator.go
🧬 Code graph analysis (9)
internal/executor/post_action_executor.go (1)
internal/config_loader/types.go (1)
ParseValueDef(33-56)
internal/executor/precondition_executor.go (3)
internal/criteria/types.go (1)
NewEvaluationContext(75-80)internal/criteria/evaluator.go (1)
NewEvaluator(55-70)internal/criteria/cel_evaluator.go (1)
CELResult(22-36)
internal/config_loader/validator_test.go (4)
internal/config_loader/types.go (13)
AdapterConfig(59-64)Metadata(67-71)AdapterConfigSpec(74-82)AdapterInfo(85-87)HyperfleetAPIConfig(90-95)KubernetesConfig(98-100)Condition(189-193)Precondition(151-158)Parameter(104-111)APICall(161-169)Resource(229-236)DiscoveryConfig(239-243)CaptureField(182-186)internal/criteria/types.go (1)
Operator(10-10)internal/k8s_client/discovery.go (2)
Discovery(14-29)DiscoveryConfig(32-42)internal/config_loader/loader.go (1)
Parse(105-121)
internal/criteria/field_evaluator.go (2)
pkg/errors/error.go (1)
New(107-123)internal/config_loader/loader.go (1)
Parse(105-121)
test/integration/config-loader/config_criteria_integration_test.go (1)
internal/criteria/types.go (2)
OperatorExists(28-28)OperatorGreaterThan(24-24)
internal/criteria/evaluator_test.go (1)
internal/criteria/field_evaluator.go (1)
ExtractField(24-93)
internal/criteria/types.go (1)
internal/criteria/field_evaluator.go (2)
FieldResult(11-14)ExtractField(24-93)
internal/criteria/evaluator_scenarios_test.go (3)
internal/criteria/types.go (3)
OperatorGreaterThan(24-24)NewEvaluationContext(75-80)OperatorContains(22-22)internal/criteria/evaluator.go (1)
NewEvaluator(55-70)pkg/logger/test_support.go (1)
NewTestLogger(17-32)
internal/criteria/evaluator.go (3)
internal/criteria/field_evaluator.go (2)
FieldResult(11-14)ExtractField(24-93)internal/criteria/types.go (1)
Operator(10-10)internal/config_loader/constants.go (1)
FieldValue(72-72)
🪛 LanguageTool
internal/executor/README.md
[style] ~199-~199: This adverb was used twice in the sentence. Consider removing one of them or replacing them with a synonym.
Context: ...ld/expression) | API Response only | Only the parsed JSON response (e.g., status...
(ADVERB_REPETITION_PREMIUM)
🔇 Additional comments (31)
internal/criteria/types.go (1)
115-121: LGTM! Clean delegation to the new extraction API.The rename from
GetNestedFieldtoGetFieldand the delegation toExtractFieldalign well with the broader API migration to structured result types.internal/config_loader/validator_test.go (2)
11-24: LGTM! Clean test helper for baseline configuration.The
baseConfig()helper provides a minimal valid configuration that tests can mutate for specific scenarios, making the test setup clearer and more maintainable than YAML-based approaches.
534-579: Excellent capture field validation coverage.These tests thoroughly validate the mutual exclusivity of
fieldandexpression, ensuring that capture definitions follow the expected structure. The test cases cover all edge cases: field-only, expression-only, both set (invalid), neither set (invalid), and missing name.internal/criteria/cel_evaluator_test.go (2)
12-20: LGTM! Constructor signature simplified.The removal of the context and logger parameters from
newCELEvaluatorsimplifies the API and aligns with the refactored evaluator architecture.
169-177: LGTM! Error handling aligned with CELResult structure.The change from
result.ErrorReasontoresult.Error.Error()correctly accesses the error through the Error field, consistent with the refactored CELResult type.internal/criteria/field_evaluator.go (2)
33-40: Auto-conversion enhances usability.The automatic conversion from simple dot notation (e.g.,
metadata.name) to JSONPath format (e.g.,{.metadata.name}) improves user experience by accepting both formats. This aligns with the decision from past review discussions to maintain flexibility.
52-70: Double execution pattern is intentional and necessary.The code executes the JSONPath twice: first with
Executeto validate the path and check for empty results, then withFindResultsto retrieve typed values. This pattern is necessary because:
Executevalidates the path exists and produces output (string representation)FindResultsretrieves the actual typed values (not just strings)While this may seem redundant, it's the correct approach for distinguishing between "field not found" and "field exists but empty."
internal/criteria/cel_evaluator.go (3)
19-36: LGTM! Simplified CELResult structure.The removal of
ErrorReasonandIsSuccess()simplifies the API. Error state is now determined solely by checkingError != nilviaHasError(), which is cleaner and more idiomatic.
43-58: LGTM! Streamlined constructor signature.Removing the context and logger parameters from
newCELEvaluatorsimplifies the API and removes unnecessary dependencies. The evaluator now focuses solely on CEL evaluation with the provided evaluation context.
148-160: LGTM! Clean error handling in EvaluateSafe.The error handling is well-structured:
- Parse errors are returned as errors (fail fast for configuration bugs)
- Evaluation errors are captured in
CELResult.Error(safe handling for missing data)The comment at line 153 correctly notes that callers should handle logging based on the result, providing flexibility for different logging strategies.
internal/criteria/README.md (1)
101-176: Excellent documentation for the new extraction APIs.The documentation clearly explains:
- JSONPath support with both simple dot notation and full JSONPath syntax
ExtractFieldandExtractValueusage with comprehensive examples- Error handling distinction between parse errors (returned) and missing fields (nil value)
- Supported JSONPath syntax with a helpful reference table
This will greatly assist developers using the new APIs.
internal/executor/post_action_executor.go (1)
159-203: LGTM! Well-structured value processing with default support.The refactored
processValuemethod cleanly handles value definitions:
- Detects value definitions via
ParseValueDef- Extracts values using the unified
ExtractValueAPI- Fails fast on parse errors (line 166-168)
- Falls back to defaults when extraction yields nil (lines 170-174)
The error handling strategy is appropriate: parse errors indicate configuration bugs and should fail immediately, while missing values gracefully use defaults.
internal/criteria/evaluator_scenarios_test.go (2)
259-345: Excellent edge case coverage for the contains operator.The new
TestContainsOperatorEdgeCasestest thoroughly validates:
- String substring matching (found/not found)
- Slice element containment with various types
- Integer slice handling
- Error cases for unsupported types (int field)
- Nil and missing field error handling
This comprehensive test coverage will help catch regressions in contains operator behavior.
427-460: LGTM! Node count validation uses structured results correctly.The test correctly combines
result1.Matched && result2.Matchedto validate range conditions, which is more explicit and maintainable than relying on raw boolean logic. The test coverage includes boundary cases (minimum, maximum, below/above range).configs/adapter-config-template.yaml (2)
101-170: LGTM! Excellent documentation improvements.The expanded documentation clearly distinguishes between capture scope (API response only) and conditions scope (full execution context). The examples for both field-based and CEL-based captures are helpful and comprehensive.
265-265: LGTM! Simplified timestamp field.The change from a nested value structure to a direct Go template string is appropriate for timestamp fields, which are rendered at template evaluation time rather than extracted from the execution context.
internal/config_loader/types.go (2)
177-186: LGTM! Well-documented capture field modes.The updated
CaptureFieldstruct properly supports both JSONPath and CEL modes with clear documentation. Theomitemptytags ensure clean YAML serialization.
3-7: LGTM! Necessary import addition.The
gopkg.in/yaml.v3import is required for theParseValueDefimplementation and is a standard, well-maintained library.internal/executor/precondition_executor.go (3)
121-123: LGTM! Enables full API response access in conditions.Storing the complete API response under the precondition name allows conditions to access any part of the response (e.g.,
checkClusterStatus.status.phase), which is a valuable feature documented in the configuration template.
128-153: LGTM! Robust capture logic with appropriate error handling.The new capture implementation properly distinguishes between:
- Parse errors (invalid field path or CEL expression) → fails fast
- Extraction errors (field not found or CEL evaluation null) → logs warning and continues
This allows optional captures that may not always be present in API responses, while still catching configuration errors.
171-224: LGTM! Clean API migration to new evaluator interface.The condition evaluation has been properly updated to use the new
EvaluateConditionsAPI. The result mapping and error handling are correct, and the execution context tracking is maintained.internal/executor/README.md (1)
313-339: LGTM! Clear post-action payload examples.The documentation clearly shows the three types of payload values (direct string, field extraction, CEL expression) with concrete examples. This aligns well with the
ValueDefimplementation.test/integration/config-loader/config_criteria_integration_test.go (2)
76-94: LGTM! Tests properly migrated to new evaluator API.The test correctly uses the new
EvaluateConditionsignature and accesses results through the structuredresultobject. The assertions validate both matching status and field values.
267-360: LGTM! Comprehensive testing of extraction and null handling.The tests correctly validate:
- Field extraction via
ExtractValuereturning values throughresult.Value- Null path handling returning
nilvalue without parse errors- Condition evaluation on null paths properly failing
The test coverage aligns with the documented behavior of the new extraction APIs.
internal/criteria/evaluator.go (4)
95-106: LGTM! Clean field extraction interface.The new
EvaluateFieldfunction provides a clear public API for JSONPath-based field extraction with comprehensive documentation and examples.
108-142: LGTM! Properly updated to use new field extraction API.The
EvaluateConditionfunction correctly uses the newGetFieldmethod and handles results through the structuredFieldResultobject. The operator evaluation logic remains robust.
191-233: LGTM! Well-implemented unified extraction with proper validation.The
ExtractValuefunction correctly implements:
- Mutual exclusivity validation (lines 196-198) - addresses the previous review concern
- Two extraction modes - CEL expression or JSONPath field
- Appropriate error handling - distinguishing parse errors (fast fail) from extraction errors (log + allow defaults)
The warning logs (lines 209, 225) provide visibility into extraction failures while allowing callers to use default values, which is the intended behavior for optional fields.
144-169: LGTM! Proper multi-condition evaluation with field extraction.The
EvaluateConditionsfunction correctly:
- Uses the new
EvaluateConditionAPI- Aggregates individual results
- Populates the
ExtractedFieldsmap for downstream use- Tracks the first failed condition index
internal/criteria/evaluator_test.go (3)
38-91: LGTM! Tests properly validate new GetField semantics.The updated tests correctly validate that:
- Parse errors are returned as errors
- Missing fields return
nilvalue without error- Valid fields return expected values
The
wantNilflag provides clear test case documentation.
658-843: LGTM! Comprehensive ExtractField test coverage.The test suite thoroughly validates:
- Simple and nested field extraction
- JSONPath syntax (wildcards, filters, array access)
- Single vs. multiple result handling
- Error conditions
The tests ensure the extraction logic works correctly for all supported use cases.
917-1104: LGTM! Excellent test coverage for value extraction and null handling.The tests comprehensively validate:
- Field-based and CEL-based extraction via
ExtractValue- Graceful handling of missing fields (nil value, no error)
- Proper behavior when conditions evaluate null paths
- Deep null path traversal
This ensures the extraction APIs handle edge cases correctly and align with documented behavior.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 86254860 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
bot
merged commit 2acf251
into
openshift-hyperfleet:main
2 participants
In this commit I
Summary by CodeRabbit
New Features
Documentation
Configuration Changes
✏️ Tip: You can customize this high-level summary in your review settings.