refactor(dsl): fail-loud contract, function registry, e2e tests; bump 26.05.08#15
Closed
ancongui wants to merge 3 commits into
Closed
refactor(dsl): fail-loud contract, function registry, e2e tests; bump 26.05.08#15ancongui wants to merge 3 commits into
ancongui wants to merge 3 commits into
Conversation
…gistry, end-to-end tests; bump to 26.05.08 Removes silent-failure pockets across the parser, evaluator, and action executor; adds a pluggable function-registry extension point and three new DSL primitives; brings docs in line with the actual codebase; and bumps the project version to 26.05.08. Core correctness fixes - Parser: complex map-shaped action handling now throws ASTException with the original-map + reconstructed-syntax context instead of catch-and- return-null. Sub-rule action lists now route through the same parseActionList as the top level, so YAML-collapsed forEach / while / do actions parse the same way in either context. - Lexer/parser: ExpressionParser now attaches the parsed array index to VariableExpression (was parsed then discarded). - Numeric coercion: toNumberSafe and toBigDecimal converge on one contract: null treated as ZERO (financial-aggregation convention), non-numeric strings raise IllegalArgumentException with operand type info. - Conditions/actions: evaluateConditions, evaluateConditionalBlock, and executeActions all propagate RuntimeExceptions wrapped in RuleEvaluationException; the outer evaluateRules catch converts these to success=false with the action/condition index, debug string, and cause. - Action executor: unknown function names in `call` actions and unknown ArithmeticOperationType branches throw IllegalArgumentException with the registry-aware diagnostic. Arithmetic actions on non-numeric operands throw rather than silently no-op'ing. - Expression evaluator: matches() raises on bad regex pattern; getPropertyValue throws on missing bean accessor (maps still get null on missing key, matching json_get semantics); is_valid rejects unknown validation types with a list of supported types. - 16+ financial / formatting / utility functions (calculate_loan_payment, compound_interest, amortization, debt_to_income_ratio, credit_utilization, loan_to_value, calculate_apr, calculate_credit_score, calculate_risk_score, payment_history_score, format_currency, format_percentage, distance_between, time_hour, in_range, calculate_debt_ratio, calculate_ltv, calculate_payment_schedule): catch-and-return-null replaced with throws via a shared wrapFunctionError helper that prefixes the message with the function name and preserves already-good diagnostics. - dateadd / datediff / toLong: bad inputs and unknown units now throw with the list of supported units. Reactive correctness - ASTRulesEvaluationEngine.evaluateRulesReactive wraps the synchronous visitor in Mono.fromCallable(...).subscribeOn(Schedulers.boundedElastic()) so REST/JSON built-ins can block safely without stalling the Netty event loop. - CacheServiceImpl fire-and-forget writes now end with .onErrorComplete() before .subscribe(); errors are still logged via the existing doOnError. Variable-store safety - EvaluationContext switched from ConcurrentHashMap (rejects null values with NPE) to Collections.synchronizedMap(LinkedHashMap). json_get returning null on a missing path no longer NPEs when stored; iteration order is now insertion-stable. - @DaTa replaced with @Getter + selective @Setter; the three variable maps are final, so the auto-generated bulk setters that would bypass the typed setters' validateVariableName guard rails no longer exist. Extension point (new) - org.fireflyframework.rules.core.dsl.function.RuleFunction: functional interface, Object apply(Object[] args). - org.fireflyframework.rules.core.dsl.function.CustomFunctionRegistry: Spring @component holding registered functions. Case-insensitive lookup. Checked before the built-in catalog, so custom functions may shadow built-ins. Wired through ASTRulesEvaluationEngine -> ActionExecutor -> ExpressionEvaluator via optional constructor parameters; existing callers keep working without changes. - ActionExecutor's default branch for `call <fn>` delegates to the expression evaluator, so the same registered function is reachable from both expression (run / calculate / condition) and action (call) contexts. New built-in functions - coalesce(a, b, c, ...): returns the first non-null argument. - if_else(condition, thenValue, elseValue): inline ternary expression for use inside run / calculate / output contexts. - is_in_range(value, low, high): function form of the between operator. - calculate_age(birthDate[, asOfDate]), format_date(date[, pattern]), validate_email(value), validate_phone(value): function forms that complement the existing operator equivalents. Dead-code / stub removal - Deleted orphan AST classes never produced by the parser: AssignmentAction, AssignmentOperator, ArithmeticExpression, ArithmeticOperation. Removed visitor methods across ASTVisitor, ActionExecutor, ExpressionEvaluator, ValidationVisitor, PythonCodeGenerator, YamlDslValidator, and ASTRulesEvaluationEngine. - Deleted DSLParser.validateAST() (was a stub with no callers). - Deleted the commented-out HealthIndicator TODO block in DatabaseConfig (actuator wired in via the web module; a proper indicator belongs there). - Deleted the permanently @disabled AuditTrailIntegrationTest (no testcontainers infrastructure was present; logic is exercised by the unit-level AuditHelperTest and AuditTrailServiceTest). - Cleaned two stale "this was the TODO that's now implemented" breadcrumbs. Test coverage - 345 tests, 0 failures, 0 errors, 0 skipped (was 323/0/5 before this work). - New: CustomFunctionRegistryTest (7), DslPrimitivesTest (9), DoWhileAnd- ConditionFunctionTest (3), EndToEndScenarioTest (5). - EndToEndScenarioTest exercises the full pipeline in one realistic loan- eligibility rule across approval / decline / tier-cutoff / empty-debt / circuit-breaker scenarios. - testCallAction split into a happy-path test against the `log` built-in and a typed-error test for unknown function names. - testDateFunctionsErrorHandling split into a happy path + two fail-loud assertions, matching the new contract. Documentation (updated to match the codebase) - README.md: features list rewritten; quick-start YAML uses canonical when/then/else syntax that actually parses; new Custom Functions and Error Contract sections. - docs/yaml-dsl-reference.md: new functions documented in their right sections; examples that used `calculate` for function calls corrected to `run` (calculate is pure-math-only); new Custom Functions extension-point section; Error Behavior Reference table contrasting old vs new contract for 11 situations; REST chain-friendly contract called out explicitly as the deliberate exception to fail-loud. - docs/developer-guide.md: all references to the four removed AST classes removed from the file-tree diagram, AST hierarchy diagram, visitor interface example, and visitor-implementation walkthrough; replaced with the actual current AST plus the new function/ package. - docs/architecture.md: Error Handling section expanded into a 12-row reference table. Version - 26.05.07 -> 26.05.08 across all five module poms and the parent.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } catch (Exception e) { | ||
| log.warn("Failed to parse reconstructed loop action: {}", actionString, e); | ||
| // Fall through to complex action parsing | ||
| log.debug("Reconstructed loop parse failed for '{}', retrying as structured map", actionString); |
added 2 commits
May 24, 2026 20:12
…t guard Audits every YAML example in the docs against the actual parser source-of- truth, fixes a flurry of documentation bugs, and locks the docs to the implementation at build time via a new parameterised validation test. What was wrong -------------- - 12 examples in yaml-dsl-reference.md and 4 elsewhere used `calculate ... as <function-call>(...)`. The DSL restricts `calculate` to pure-math expressions and raises IllegalArgumentException for function/REST/JSON calls; these examples couldn't actually be evaluated. Corrected to use `run`. - Arithmetic-action grammar was documented backwards. The parser is `<keyword> <value> <preposition> <target-variable>`, so the correct form is `multiply 1.5 by risk_factor`, not `multiply risk_factor by 1.5`. Updated the operator table and examples; added an explicit grammar note. - "Validation operators in expressions" example used C-style ternary `(... ? X : Y)` -- a syntax the parser doesn't have. Rewritten using the `if_else(cond, then, else)` built-in (which is documented in the same doc). - Several complete examples in docs/yaml-dsl-reference.md, common-patterns- guide.md, b2b-credit-scoring-tutorial.md, and quick-start-guide.md used unquoted YAML strings containing colons (e.g., `"Loan approved: " + amount`) or other patterns that the YAML / DSL parser rejects. Fixed where the rewrite was mechanical; tagged the rest with TODO skip markers (see below). New: DocExamplesValidationTest ------------------------------ - Extracts every fenced ```yaml block from README.md, docs/yaml-dsl- reference.md, docs/quick-start-guide.md, docs/common-patterns-guide.md, and docs/b2b-credit-scoring-tutorial.md (60 blocks total). - Skips blocks that don't look like complete rules (missing every top-level key), and skips blocks explicitly tagged with `<!-- doc-test:skip -->` in the surrounding markdown (with an optional trailing rationale parenthetical). - Parses each remaining block through the real `ASTRulesDSLParser` and fails the build with the file:line of the offending block if parsing throws. - 49 documented rule examples are now actively validated at every build. Future doc drift -- a renamed function, a removed operator, a typo, a syntactic restriction -- is caught immediately with a precise message. - 11 blocks are deliberately marked skip: schema sketches with `[placeholder]` text and template snippets used to describe DSL shape, plus a small set of legacy walkthrough examples carrying explicit TODO notes for future rewrite. Source-of-truth catalogue ------------------------- A parallel audit confirmed the doc now correctly enumerates every operator, action keyword, and built-in function the parser accepts -- including synonyms (`equals`/`==`, `at_least`/`>=`, `in`/`in_list`, etc.), 30+ comparison operators, 33 unary operators, the 16 action keywords, and the ~70 built-in functions in the ExpressionEvaluator switch. No documented feature is missing from the parser; no parser feature is undocumented. Tests ----- - 394 tests, 0 failures, 0 errors, 0 skipped (was 345 before this commit). +49 from the new parameterised DocExamplesValidationTest. Version ------- No version change in this commit -- still on 26.05.08 from the previous commit on this branch.
…d dep; symmetric arithmetic grammar This is the modernisation pass. It removes every piece of "we kept it around" DSL surface that wasn't actually serving users, and fixes the one real arithmetic-grammar coherence issue. The result is a smaller, more uniform, and more honest DSL. Three parallel audits drove this commit: 1. dead-code/deprecated-API audit 2. DSL-surface inconsistency audit 3. deep design-coherence audit What's removed -------------- - **`JsonPathExpression`, `RestCallExpression` AST classes** -- zero `new` callers anywhere in the codebase; every visitor across `ASTVisitor`, `ActionExecutor`, `ExpressionEvaluator`, `ValidationVisitor`, `PythonCodeGenerator`, `YamlDslValidator.ValidationVariableCollector`, and `ASTRulesEvaluationEngine. VariableReferenceCollector` had a method for them, but the parser never produced them. JSON path / REST functionality is reached through the ordinary `FunctionCallExpression` path (`json_get`, `rest_get`, etc.) -- unchanged for users. Removes 200 lines of dark code. - **Top-level `circuit_breaker:` YAML config block** (`ASTCircuitBreakerConfig` inner record + `convertToCircuitBreakerConfig` parser branch + ASTRulesDSL field + `validateCircuitBreakerConfig` validator stub). Parsed by the YAML layer, stored in the model, "validated" by an empty validator method, but **never read by the evaluator at runtime**. Resilience is already provided by the `circuit_breaker "MESSAGE"` action, which is unchanged. - **`commons-math3` dependency** -- zero `import org.apache.commons.math*` anywhere in core. Was used by the now-removed `ArithmeticExpression` (deleted in an earlier commit). Dead weight. - **`@Deprecated` annotation on `parseRules(String)`** -- the method is a legitimate synchronous convenience wrapper, used by 9 callers (5 tests, 4 production). Removing the tag and updating the JavaDoc honestly. The evaluator's `evaluateRules(String, Map)` was never `@Deprecated` and is treated the same way. What's improved --------------- - **Arithmetic grammar is now symmetric for `multiply` and `divide`.** The parser previously accepted only `multiply VALUE by VARIABLE` (e.g., `multiply 1.5 by risk_factor`) -- the English-natural reading is `multiply VARIABLE by VALUE`, so users would write `multiply risk_factor by 1.5` and get a "Expected variable name after 'by'" error. Both forms are now accepted and produce the same `ArithmeticAction`. `add` and `subtract` remain unchanged (their value-first English form -- `add 5 to score`, `subtract penalty from total` -- is already natural). - New `ArithmeticActionSymmetryTest` (5 cases) locks in the symmetry contract and exercises both forms for both `multiply` and `divide`, plus the unchanged `add`/`subtract` behaviour, plus a complex-value-expression case. Documentation ------------- - `docs/yaml-dsl-reference.md` -- removes the documentation of the top-level `circuit_breaker:` block, replacing it with an explicit note that the only circuit-breaker surface is the *action*, with an example. - `docs/developer-guide.md` -- the AST file-tree, visitor interface, and hierarchy diagram drop their references to `JsonPathExpression` and `RestCallExpression`. - `docs/governance-guidelines.md` -- removes the now-invalid `circuit_breaker:` config example, replaces with the action-form equivalent. Tests ----- - 398 tests, 0 failures, 0 errors, 0 skipped. - +5 from `ArithmeticActionSymmetryTest`. - `DocExamplesValidationTest` continues to actively validate 49 documented rule examples at every build; the deletion of the `circuit_breaker:` block documentation removed it from validation (it would have failed under the new parser anyway).
Contributor
Author
|
Closing in favour of a fresh PR that supersedes this one with the additional DSL modernisation work (orphan AST removal, dead-config removal, arithmetic grammar symmetry). The branch is the same; opening a new PR with a comprehensive description. |
Merged
8 tasks
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
2 participants
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
Hardens the rule-engine DSL with a fail-loud error contract, adds a pluggable custom-function-registry extension point, closes several real bugs (sub-rule action parsing, dead array-index handling, ConcurrentHashMap rejecting nulls), removes orphan AST classes and stub methods, and brings the docs in line with the actual codebase. Tests: 345 passed, 0 failed, 0 skipped (up from 323/0/5; +22 net, –5 skipped). Bumps version to 26.05.08.
Highlights
Correctness
success=falsewith precise diagnostics — replacing 30+catch(Exception) → log.warn + return nullsites.forEachworks in either context);ExpressionParserattaches the parsed array index toVariableExpression(was parsed then discarded);EvaluationContextno longer NPEs when storing legitimate nulls (e.g.,json_geton a missing path) because the variable maps switched fromConcurrentHashMaptosynchronizedMap(LinkedHashMap).Mono.fromCallable(...).subscribeOn(Schedulers.boundedElastic())so REST/JSON built-ins can block safely without stalling the Netty event loop.Extensibility
org.fireflyframework.rules.core.dsl.functionpackage:RuleFunction(functional interface) +CustomFunctionRegistry(Spring@Component). Custom functions checked before built-ins (so they can shadow), case-insensitive, reachable from bothrun/calculate/condition (expression) andcall(action) contexts.New DSL primitives
coalesce(a, b, c, ...)— first non-null winsif_else(condition, then, else)— inline ternary expressionis_in_range(value, low, high)— function form of thebetweenoperatorcalculate_age(birth[, asOf]),format_date(date[, pattern]),validate_email(value),validate_phone(value)— closes documented-but-missing functionsDead code & stubs removed
AssignmentAction,AssignmentOperator,ArithmeticExpression,ArithmeticOperation) with all their visitor methods across 7 files.DSLParser.validateAST()(was a stub with zero callers).@DisabledAuditTrailIntegrationTest(no testcontainers infra; logic covered byAuditHelperTestandAuditTrailServiceTest).HealthIndicatorTODO block inDatabaseConfig.Documentation aligned to reality
README.md: features list rewritten; Quick-Start YAML uses canonicalwhen/then/elsesyntax that actually parses; new Custom Functions and Error Contract sections.docs/yaml-dsl-reference.md: new functions documented; examples that usedcalculatefor function calls corrected torun(sincecalculateis restricted to pure-math); Custom Functions extension-point section; Error Behavior Reference table contrasting old vs new contract for 11 situations.docs/developer-guide.md: all references to the four removed AST classes scrubbed from file-tree, AST hierarchy, visitor interface, and visitor-impl walkthroughs; replaced with the actual current AST plus the newfunction/package.docs/architecture.md: Error Handling section expanded into a 12-row reference table.Test Plan
mvn clean verifygreen across all 5 modules — 345 passed, 0 failed, 0 errors, 0 skippedEndToEndScenarioTestexercises the full pipeline (constants, sub-rules, loops, conditions, custom functions, circuit breaker) across approval / decline / tier-cutoff / empty-debt / circuit-breaker scenariosCustomFunctionRegistryTestcovers registration, case-insensitive lookup, built-in shadowing, unknown-function error path, re-registration, unregister, input validationDslPrimitivesTestcoverscoalesce,if_else,is_in_range,calculate_age,format_date,validate_email,is_validunknown-type error pathDoWhileAndConditionFunctionTestcloses two gaps the audit identified:do-whilesemantics and custom-function-in-condition (not just action)testCallActionsplit into happy-path + typed-error variants;testDateFunctionsErrorHandlingsplit into happy path + two fail-loud assertionsfireflyframework-utils/-validatorsreference${project.version}and must publish at the same version)Version Bump
26.05.07→26.05.08across the root pom + all 5 child module parent refs. The line-10 reference tofireflyframework-parentis intentionally left at 26.05.07 (external framework parent).What's intentionally deferred
ConstantServiceImpl.getConstantsByCodesper-code.onErrorResume,AuditTrailServiceImplJSON-serialization fallback,YamlDslValidatorcatch-all warning) — internal-only; documented inarchitecture.mdas design choices.ExpressionEvaluatorsplitting (2400-line file) — large refactor; safer as a dedicated PR with reviewer eyes.Notes for reviewers
The single biggest behaviour change is the fail-loud error contract: before this PR, many error situations silently produced
nullorfalseand the rule continued to its conclusion with a wrong but plausible-looking output. The new behaviour returnssuccess=falsefromASTRulesEvaluationResultwith the failing action/condition index + debug string + cause. The deliberate exception is REST functions, which still return a structured{success:false, error:true, message}map to support rule-driven retry/circuit-break chains — this is called out explicitly in the new docs section.