diff --git a/CHANGELOG.md b/CHANGELOG.md index f9be5524..8dc1f01d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,8 @@ This is a history of changes to clara-rules. +# 0.20.0-SNAPSHOT +* Correct implementation surrrouning bindings in ExpressionJoinNodes. See [Issue 426](https://github.com/cerner/clara-rules/issues/426) + # 0.19.1 * Added a new field to the clara.rules.engine/Accumulator record. This could be a breaking change for any user durability implementations with low-level performance optimizations. See [PR 410](https://github.com/cerner/clara-rules/pull/410) for details. * Performance improvements for :exists conditions. See [issue 298](https://github.com/cerner/clara-rules/issues/298). diff --git a/src/main/clojure/clara/macros.clj b/src/main/clojure/clara/macros.clj index 44355986..c5ddfef8 100644 --- a/src/main/clojure/clara/macros.clj +++ b/src/main/clojure/clara/macros.clj @@ -174,8 +174,10 @@ ;; that a rule wouldn't, or at least shouldn't for ;; clarity, start the names of other locals or vars ;; with "?". - (mapv (comp symbol name) all-bindings))] - (com/compile-action all-bindings + (mapv (comp symbol name) (:bindings beta-node)))] + ;; using the :bindings defined on the beta-node rather than `all-bindings`, as all-bindings + ;; does not account for bindings that reside within expression join nodes + (com/compile-action (:bindings beta-node) ;; Using private function for now as a workaround. (if (:ns-name production) (if (com/compiling-cljs?) diff --git a/src/main/clojure/clara/rules/compiler.clj b/src/main/clojure/clara/rules/compiler.clj index 773d32f1..bc7d2ef0 100644 --- a/src/main/clojure/clara/rules/compiler.clj +++ b/src/main/clojure/clara/rules/compiler.clj @@ -9,7 +9,6 @@ [clojure.core.reducers :as r] [clojure.reflect :as reflect] [clojure.set :as s] - [clojure.set :as set] [clojure.string :as string] [clojure.walk :as walk] [schema.core :as sc] @@ -803,8 +802,8 @@ ;; Unsatisfied conditions remain, so find ones we can satisfy. (let [satisfied? (fn [classified-condition] - (clojure.set/subset? (:unbound classified-condition) - bound-variables)) + (s/subset? (:unbound classified-condition) + bound-variables)) ;; Find non-accumulator conditions that are satisfied. We defer ;; accumulators until later in the rete network because they @@ -812,8 +811,8 @@ ;; in the network are satisfied. satisfied-non-accum? (fn [classified-condition] (and (not (:is-accumulator classified-condition)) - (clojure.set/subset? (:unbound classified-condition) - bound-variables))) + (s/subset? (:unbound classified-condition) + bound-variables))) has-satisfied-non-accum (some satisfied-non-accum? remaining-conditions) @@ -825,16 +824,16 @@ (remove satisfied-non-accum? remaining-conditions) (remove satisfied? remaining-conditions)) - updated-bindings (apply clojure.set/union bound-variables + updated-bindings (apply s/union bound-variables (map :bound newly-satisfied))] ;; If no existing variables can be satisfied then the production is invalid. (when (empty? newly-satisfied) ;; Get the subset of variables that cannot be satisfied. - (let [unsatisfiable (clojure.set/difference - (apply clojure.set/union (map :unbound still-unsatisfied)) - bound-variables)] + (let [unsatisfiable (s/difference + (apply s/union (map :unbound still-unsatisfied)) + bound-variables)] (throw (ex-info (str "Using variable that is not previously bound. This can happen " "when an expression uses a previously unbound variable, " "or if a variable is referenced in a nested part of a parent " @@ -855,11 +854,30 @@ (defn- non-equality-unifications "Returns a set of unifications that do not use equality-based checks." [constraints] - (let [[bound-variables unbound-variables] (classify-variables constraints)] - (into #{} - (for [constraint constraints - :when (non-equality-unification? constraint bound-variables)] - constraint)))) + (let [[bound-variables _] (classify-variables constraints)] + (loop [[cur & more] constraints + constraints #{} + bound-variables bound-variables] + (if cur + (let [non-equity? (non-equality-unification? cur bound-variables) + + ;; In the event that the unification is also a binding, then we will need to remove + ;; the bound variable from the previously bound variables. This prevents further constraints + ;; that use this variable from being considered "equality-unifications". + ;; See https://github.com/cerner/clara-rules/issues/426 for more info. + bound-variables (if (and non-equity? + (seq cur) + (equality-expression? cur)) + (s/difference bound-variables + (into #{} + (filter is-variable?) + cur)) + bound-variables) + constraints (if non-equity? + (conj constraints cur) + constraints)] + (recur more constraints bound-variables)) + constraints)))) (sc/defn condition-to-node :- schema/ConditionNode "Converts a condition to a node structure." @@ -917,7 +935,7 @@ constraint-bindings) new-bindings (s/difference (variables-as-keywords (:constraints condition)) - parent-bindings) + parent-bindings) join-filter-bindings (if join-filter-expressions (variables-as-keywords join-filter-expressions) @@ -1018,7 +1036,7 @@ ;; have the necessary bindings. ;; See https://github.com/cerner/clara-rules/issues/304 for more details ;; and a case that behaves incorrectly without this check. - ancestor-bindings-in-negation-expr (set/intersection + ancestor-bindings-in-negation-expr (s/intersection (variables-as-keywords negation-expr) ancestor-bindings) @@ -1224,8 +1242,8 @@ ;; for use in descendent nodes. {:beta-graph (:beta-graph new-result) :new-ids (into (:new-ids previous-result) (:new-ids new-result)) - :bindings (set/union (:bindings previous-result) - (:bindings new-result))})) + :bindings (s/union (:bindings previous-result) + (:bindings new-result))})) ;; Initial reduce value, combining previous graph, parent ids, and ancestor variable bindings. {:beta-graph beta-with-negations diff --git a/src/main/clojure/clara/rules/testfacts.cljc b/src/main/clojure/clara/rules/testfacts.cljc index 41fff1d7..3ce5fa73 100644 --- a/src/main/clojure/clara/rules/testfacts.cljc +++ b/src/main/clojure/clara/rules/testfacts.cljc @@ -5,6 +5,7 @@ ;; place them here as leiningen won't AOT compile test resources. (defrecord Temperature [temperature location]) (defrecord WindSpeed [windspeed location]) +(defrecord WindChill [temperature location wind-chill]) (defrecord Cold [temperature]) (defrecord Hot [temperature]) (defrecord ColdAndWindy [temperature windspeed]) diff --git a/src/test/common/clara/test_bindings.cljc b/src/test/common/clara/test_bindings.cljc index 64df8d1f..1aaf4f03 100644 --- a/src/test/common/clara/test_bindings.cljc +++ b/src/test/common/clara/test_bindings.cljc @@ -12,7 +12,7 @@ query]] [clara.rules.testfacts :refer [->Temperature ->Cold ->WindSpeed - ->ColdAndWindy]] + ->ColdAndWindy ->WindChill]] [clojure.test :refer [is deftest run-tests testing use-fixtures]] [clara.rules.accumulators :as acc] [schema.test :as st]) @@ -20,6 +20,7 @@ Temperature Cold WindSpeed + WindChill ColdAndWindy])) :cljs @@ -33,6 +34,7 @@ [clara.rules.testfacts :refer [->Temperature Temperature ->Cold Cold ->WindSpeed WindSpeed + ->WindChill WindChill ->ColdAndWindy ColdAndWindy]] [clara.rules.accumulators :as acc] [cljs.test] @@ -369,3 +371,70 @@ fire-rules (query cold-query))) "The query results should not be empty for a matching location")) + +;; https://github.com/cerner/clara-rules/issues/426 +;; A join binding used in the same condition but different constraint will fail to compile. +(defn calc-wind-chill + [temp wind-speed] + (+ 35.74 + (- (* 0.6215 temp) + (* 35.75 + (Math/pow wind-speed 0.16))) + (* 0.4275 temp (Math/pow wind-speed 0.16)))) + +(def-rules-test test-join-binding-followed-by-usage + {:rules [cold-windy-rule [[[Temperature + (= ?t temperature) + (= ?loc location)] + [WindSpeed + (= location ?loc) + (= ?wc (calc-wind-chill ?t windspeed)) + (<= ?wc 32)]] + (insert! (->WindChill ?t ?loc ?wc))]] + :queries [wind-chill-query [[] + [[WindChill + (= ?t temperature) + (= ?wind-chill wind-chill) + (= ?loc location)]]]] + :sessions [empty-session [cold-windy-rule wind-chill-query] {}]} + ;; The overall intent of this test is to prove that the rules above compile and + ;; operate as expected. + (is (= [{:?loc "LHR" + :?t 36 + :?wind-chill (calc-wind-chill 36 7)}] + (-> empty-session + (insert (->Temperature 36 "LHR")) + (insert (->WindSpeed 7 "LHR")) + fire-rules + (query wind-chill-query))) + ;; Not the intent of the test, but a distinct message in the event the test fails. + "The query should return the wind-chill for the given location since the wind chill is under 32 degrees")) + +;; https://github.com/cerner/clara-rules/issues/426 +;; Due to the way we compile the beta-network for cljs this test would fail for cljs because the binding +;; expression join node(?wc) would not be recognised correctly in the production node. Thus ?wc would be +;; assumed to be a variable defined in this ns and would resolve to nil at runtime. +(def-rules-test test-join-binding-used-in-a-produciton-node + {:rules [cold-windy-rule [[[Temperature + (= ?t temperature) + (= ?loc location)] + [WindSpeed + (= location ?loc) + (= ?wc (calc-wind-chill ?t windspeed))]] + (insert! (->WindChill ?t ?loc ?wc))]] + :queries [wind-chill-query [[] + [[WindChill + (= ?t temperature) + (= ?wind-chill wind-chill) + (= ?loc location)]]]] + :sessions [empty-session [cold-windy-rule wind-chill-query] {}]} + (is (= [{:?loc "LHR" + :?t 36 + :?wind-chill (calc-wind-chill 36 7)}] + (-> empty-session + (insert (->Temperature 36 "LHR")) + (insert (->WindSpeed 7 "LHR")) + fire-rules + (query wind-chill-query))) + ;; Not the intent of the test, but a distinct message in the event the test fails. + "The query should return the wind-chill for the given location since there is a Temperature and WindSpeed")) \ No newline at end of file