Skip to content

Commit

Permalink
Issue446: Bean accessors don't account for IndexedPropertyDescriptors (
Browse files Browse the repository at this point in the history
  • Loading branch information
EthanEChristian authored Feb 18, 2020
1 parent 1bafa05 commit bda21fe
Show file tree
Hide file tree
Showing 4 changed files with 120 additions and 33 deletions.
4 changes: 3 additions & 1 deletion project.clj
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@
:dependencies [[org.clojure/clojure "1.7.0"]
[prismatic/schema "1.1.6"]]
:profiles {:dev {:dependencies [[org.clojure/math.combinatorics "0.1.3"]
[org.clojure/data.fressian "0.2.1"]]}
[org.clojure/data.fressian "0.2.1"]]
:java-source-paths ["src/test/java"]
:global-vars {*warn-on-reflection* true}}
:provided {:dependencies [[org.clojure/clojurescript "1.7.170"]]}
:recent-clj {:dependencies [^:replace [org.clojure/clojure "1.9.0"]
^:replace [org.clojure/clojurescript "1.9.946"]]}
Expand Down
60 changes: 28 additions & 32 deletions src/main/clojure/clara/rules/compiler.clj
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,11 @@
This is the Clara rules compiler, translating raw data structures into compiled versions and functions.
Most users should use only the clara.rules namespace."
(:require [clara.rules.engine :as eng]
[clara.rules.listener :as listener]
[clara.rules.platform :as platform]
[clara.rules.schema :as schema]
[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]
[schema.macros :as sm])
[schema.core :as sc])
(:import [clara.rules.engine
ProductionNode
QueryNode
Expand All @@ -27,7 +21,6 @@
AccumulateNode
AccumulateWithJoinFilterNode
LocalTransport
LocalSession
Accumulator
ISystemFact]
[java.beans
Expand Down Expand Up @@ -179,10 +172,13 @@
;; Iterate through the bean properties, returning tuples and the corresponding methods.
(for [^PropertyDescriptor property (seq (.. java.beans.Introspector
(getBeanInfo cls)
(getPropertyDescriptors)))]

[(symbol (string/replace (.. property (getName)) #"_" "-")) ; Replace underscore with idiomatic dash.
(symbol (str "." (.. property (getReadMethod) (getName))))])))
(getPropertyDescriptors)))
:let [read-method (.getReadMethod property)]
;; In the event that there the class has an indexed property without a basic accessor we will simply skip
;; the accessor as we will not know how to retrieve the value. see https://github.com/cerner/clara-rules/issues/446
:when read-method]
[(symbol (string/replace (.getName property) #"_" "-")) ; Replace underscore with idiomatic dash.
(symbol (str "." (.getName read-method)))])))

(defn effective-type [type]
(if (compiling-cljs?)
Expand Down Expand Up @@ -765,7 +761,7 @@
;; have any new facts as input. See:
;; https://github.com/cerner/clara-rules/issues/357
[#{}
(apply s/union (classify-variables constraints))]
(apply set/union (classify-variables constraints))]

;; It is not a negation, so simply classify variables.
(classify-variables constraints))
Expand All @@ -775,10 +771,10 @@
(:result-binding leaf-condition) (conj (symbol (name (:result-binding leaf-condition)))))

;; All variables bound in this condition.
all-bound (s/union bound bound-with-result-bindings)
all-bound (set/union bound bound-with-result-bindings)

;; Unbound variables, minus those that have been bound elsewhere in this condition.
all-unbound (s/difference (s/union unbound-variables unbound) all-bound)]
all-unbound (set/difference (set/union unbound-variables unbound) all-bound)]

{:bound all-bound
:unbound all-unbound
Expand Down Expand Up @@ -812,17 +808,17 @@

;; Unsatisfied conditions remain, so find ones we can satisfy.
(let [satisfied? (fn [classified-condition]
(clojure.set/subset? (:unbound classified-condition)
bound-variables))
(set/subset? (:unbound classified-condition)
bound-variables))

;; Find non-accumulator conditions that are satisfied. We defer
;; accumulators until later in the rete network because they
;; may fire a default value if all needed bindings earlier
;; 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)))
(set/subset? (:unbound classified-condition)
bound-variables)))

has-satisfied-non-accum (some satisfied-non-accum? remaining-conditions)

Expand All @@ -834,16 +830,16 @@
(remove satisfied-non-accum? remaining-conditions)
(remove satisfied? remaining-conditions))

updated-bindings (apply clojure.set/union bound-variables
updated-bindings (apply set/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 (set/difference
(apply set/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 "
Expand Down Expand Up @@ -925,7 +921,7 @@
(conj constraint-bindings (:fact-binding condition))
constraint-bindings)

new-bindings (s/difference (variables-as-keywords (:constraints condition))
new-bindings (set/difference (variables-as-keywords (:constraints condition))
parent-bindings)

join-filter-bindings (if join-filter-expressions
Expand All @@ -936,20 +932,20 @@
{:node-type node-type
:condition condition
:new-bindings new-bindings
:used-bindings (s/union cond-bindings join-filter-bindings)}
:used-bindings (set/union cond-bindings join-filter-bindings)}

(seq env) (assoc :env env)

;; Add the join bindings to join, accumulator or negation nodes.
(#{:join :negation :accumulator} node-type) (assoc :join-bindings (s/intersection cond-bindings parent-bindings))
(#{:join :negation :accumulator} node-type) (assoc :join-bindings (set/intersection cond-bindings parent-bindings))

accumulator (assoc :accumulator accumulator)

result-binding (assoc :result-binding result-binding)

join-filter-expressions (assoc :join-filter-expressions join-filter-expressions)

join-filter-bindings (assoc :join-filter-join-bindings (s/intersection join-filter-bindings parent-bindings)))))
join-filter-bindings (assoc :join-filter-join-bindings (set/intersection join-filter-bindings parent-bindings)))))

(sc/defn ^:private add-node :- schema/BetaGraph
"Adds a node to the beta graph."
Expand Down Expand Up @@ -1028,8 +1024,8 @@
;; 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
(variables-as-keywords negation-expr)
ancestor-bindings)
(variables-as-keywords negation-expr)
ancestor-bindings)

ancestor-bindings-insertion-form (into {}
(map (fn [binding]
Expand Down Expand Up @@ -1105,7 +1101,7 @@

{:keys [result-binding fact-binding]} expression

all-bindings (cond-> (s/union bindings (:used-bindings node))
all-bindings (cond-> (set/union bindings (:used-bindings node))
result-binding (conj result-binding)
fact-binding (conj fact-binding))

Expand Down Expand Up @@ -1626,9 +1622,9 @@
pending-id))

updated-edges (into {} (for [[dependent-id dependencies] node-deps]
[dependent-id (s/difference dependencies newly-satisfied-ids)]))]
[dependent-id (set/difference dependencies newly-satisfied-ids)]))]

(recur (s/difference pending-ids newly-satisfied-ids)
(recur (set/difference pending-ids newly-satisfied-ids)
updated-edges
(concat sorted-nodes newly-satisfied-ids)))))]

Expand Down
50 changes: 50 additions & 0 deletions src/test/clojure/clara/test_java_facts.clj
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
(ns clara.test-java-facts
(:require [clara.tools.testing-utils :as tu]
[clara.rules :as rules]
[clojure.test :refer [is deftest run-tests testing use-fixtures]]
[clara.rules.compiler :as com])
(:import [clara.test.facts
BeanTestFact]))

;; A test to demonstrate that a Pojo with indexed property accessors can be used as an alpha root in a session,
;; see https://github.com/cerner/clara-rules/issues/446
(tu/def-rules-test test-basic-rule
{:rules [kansas-rule [[[BeanTestFact
(= ?locs locations)
(some #(= "Kansas" %) ?locs)]]
(rules/insert! "Kansas Exists")]]
:queries [string-query [[] [[?s <- String]]]]

:sessions [empty-session [kansas-rule string-query] {}]}

(let [locs (doto (make-array String 2)
(aset 0 "Florida")
(aset 1 "Kansas"))]
(let [session-strings (map :?s
(-> empty-session
(rules/insert (BeanTestFact. locs))
(rules/fire-rules)
(rules/query string-query)))]
(is (= ["Kansas Exists"] session-strings)))))

;; Using an indexed property accessor that doesn't have a standard accessor will throw exception as clara cannot resolve
;; the usage of the accessor. See https://github.com/cerner/clara-rules/issues/446
(deftest test-indexed-property-accessor
(let [rule-using-unsupported-accessor {:ns-name (ns-name *ns*)
:lhs [{:type BeanTestFact :constraints [`(contains? ~'roadConditions "Slippery")]}]
:rhs `(rules/insert! "doesn't matter")
:name "rule-using-unsupported-accessor"}]
(try
(com/mk-session [[rule-using-unsupported-accessor]])
(is false "An exception should be thrown")
(catch Exception e
(loop [exc e]
(cond
(re-find #"Failed compiling alpha node" (.getMessage exc))
:success

(.getCause exc)
(recur (.getCause exc))

:else
(is false "Exception didn't contain a message containing alpha node failure")))))))
39 changes: 39 additions & 0 deletions src/test/java/clara/test/facts/BeanTestFact.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
package clara.test.facts;

/**
* A Java Pojo for the express purpose of testing the behavior of compilation and execution of rules.
*
* This class should not be included in the released artifact, if it did make it into a released artifact it should be
* ignored and not consumed as it may be moved/removed without warning to consumers.
*/
public class BeanTestFact {
private String[] locations;
private String[] roadConditions;

public BeanTestFact(String[] locations) {
this.locations = locations;
}

// Standard and Indexed property accessors
public void setLocations(String[] locations) {
this.locations = locations;
}
public String[] getLocations() {
return locations;
}
public void setLocations(int pos, String location) {
locations[pos] = location;
}
public String getLocations(int pos){
return locations[pos];
}

// Partial Indexed property accessor, ie. no standard accessor
// See https://github.com/cerner/clara-rules/issues/446 for more details
public void setRoadConditions(int pos, String condition) {
roadConditions[pos] = condition;
}
public String getRoadConditions(int pos){
return roadConditions[pos];
}
}

0 comments on commit bda21fe

Please sign in to comment.