ISSUE-433: Behavior of rules sharing conditions within :or is incorrect

CHANGELOG.md

@@ -3,6 +3,7 @@ This is a history of changes to clara-rules.
 # 0.20.0 SNAPSHOT
 * Add a flag to omit compilation context (used by the durability layer) after Session compilation to save space when not needed. Defaults to true. [issue 422](https://github.com/cerner/clara-rules/issues/422)
 * Correct duplicate bindings within the same condition. See [issue 417](https://github.com/cerner/clara-rules/issues/417)
+* Correct sharing of nodes with different parents. See [issue 433](https://github.com/cerner/clara-rules/issues/433) 
 
 # 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.

src/main/clojure/clara/rules/compiler.clj

@@ -1136,20 +1136,33 @@
             ;; all of the old children for a prior match, for each additional child added we incur the assoc but the hash code
             ;; has already been cached by prior iterations of add-conjunctions. In these cases we have seen that the hashing
             ;; will out perform equivalence checks.
-            update-node->id (fn [m id]
-                              (let [node (get id-to-condition-nodes id)
-                                    prev (get m node)]
-                                (if prev
-                                  (assoc! m node (min prev id))
-                                  (assoc! m node id))))
-
-            node->id (-> (reduce update-node->id
-                                 (transient {})
-                                 forward-edges)
-                         persistent!)
+            update-node->ids (fn [m id]
+                               (let [node (get id-to-condition-nodes id)
+                                     prev (get m node)]
+                                 (if prev
+                                   ;; There might be nodes with multiple representation under the same parent, this would
+                                   ;; likely be due to nodes that have the same condition but do not share all the same
+                                   ;; parents, see Issue 433 for more information
+                                   (assoc! m node (conj prev id))
+                                   (assoc! m node [id]))))
+
+            node->ids (-> (reduce update-node->ids
+                                  (transient {})
+                                  forward-edges)
+                          persistent!)
+
+            backward-edges (:backward-edges beta-graph)
+
+            parent-ids-set (set parent-ids)
 
             ;; Use the existing id or create a new one.
-            node-id (or (get node->id node)
+            node-id (or (when-let [common-nodes (get node->ids node)]
+                          ;; We need to validate that the node we intend on sharing shares the same parents as the
+                          ;; current node we are creating. See Issue 433 for more information
+                          (some #(when (= (get backward-edges %)
+                                          parent-ids-set)
+                                   %)
+                                common-nodes))
                         (create-id-fn))
 
             graph-with-node (add-node beta-graph parent-ids node-id node)]

src/test/common/clara/test_simple_rules.cljc

@@ -194,3 +194,31 @@
     (is (has-fact? (:cold @side-effect-holder) (->Temperature -10 "MCI") ))
 
     (is (has-fact? (:subzero @side-effect-holder) (->Temperature -10 "MCI")))))
+
+
+;; A test to validate that nodes of different parents will not be shared.
+;; See issue 433 for more information
+(def-rules-test test-or-sharing-same-condition
+  {:rules [or-rule [[[:or
+                      [::a]
+                      [::b]]
+                     [::d]]
+                    (insert! {:fact-type ::c})]
+
+           other-rule [[[::a]
+                        [::d]]
+                       (insert! {:fact-type ::e})]]
+   :queries [c-query [[] [[?c <- ::c]]]
+             e-query [[] [[?e <- ::e]]]]
+
+   :sessions [empty-session [or-rule other-rule c-query e-query] {:fact-type-fn :fact-type}]}
+  (let [session (-> empty-session
+                    (insert {:fact-type ::b})
+                    (insert {:fact-type ::d})
+                    fire-rules)]
+
+    (is (= (set (query session c-query))
+           #{{:?c {:fact-type ::c}}}))
+
+    (is (= (set (query session e-query))
+           #{}))))