From 5b13c56df07251b97d544c9881d7796719d1a2c3 Mon Sep 17 00:00:00 2001 From: ec027900 Date: Sun, 10 May 2020 11:28:51 -0500 Subject: [PATCH] [Issue454]: Add validation that queries provide the correct parameters --- CHANGELOG.md | 1 + src/main/clojure/clara/rules/compiler.clj | 25 +++++++++++----- src/main/clojure/clara/rules/engine.cljc | 3 ++ src/test/clojure/clara/test_compiler.clj | 26 +++++++++++++++-- src/test/common/clara/test_simple_rules.cljc | 30 ++++++++++++++++++++ 5 files changed, 75 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fcfcaf28..05c0bdcf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ This is a history of changes to clara-rules. # 0.21.0 SNAPSHOT * Add names to anonymous functions generated by rule compilation, see [issue 261](https://github.com/cerner/clara-rules/issues/261) and [issue 291](https://github.com/cerner/clara-rules/issues/291) * Add alpha node types, see [issue 237](https://github.com/cerner/clara-rules/issues/237) +* BREAKING: Validate parameters provided to queries exist on query and compilation. see [issue 454](https://github.com/cerner/clara-rules/issues/454) # 0.20.0 * 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) diff --git a/src/main/clojure/clara/rules/compiler.clj b/src/main/clojure/clara/rules/compiler.clj index 322dc7c1..264cb885 100644 --- a/src/main/clojure/clara/rules/compiler.clj +++ b/src/main/clojure/clara/rules/compiler.clj @@ -1292,15 +1292,26 @@ beta-with-nodes)) ;; No more conditions to add, so connect the production. - (add-node beta-graph - parent-ids - (create-id-fn) - (if (:rhs production) + (if (:rhs production) + ;; if its a production node simply add it + (add-node beta-graph + parent-ids + (create-id-fn) {:node-type :production :production production - :bindings ancestor-bindings} - {:node-type :query - :query production})))))) + :bindings ancestor-bindings}) + ;; else its a query node and we need to validate that the query has at least the bindings + ;; specified in the parameters + (if (every? ancestor-bindings (:params production)) + (add-node beta-graph + parent-ids + (create-id-fn) + {:node-type :query + :query production}) + (throw (ex-info "QueryNode does not contain bindings specified in parameters." + {:expected-bindings (:params production) + :available-bindings ancestor-bindings + :query (:name production)})))))))) (sc/defn to-beta-graph :- schema/BetaGraph diff --git a/src/main/clojure/clara/rules/engine.cljc b/src/main/clojure/clara/rules/engine.cljc index 01dbe35e..24644fc7 100644 --- a/src/main/clojure/clara/rules/engine.cljc +++ b/src/main/clojure/clara/rules/engine.cljc @@ -1986,6 +1986,9 @@ (let [query-node (get-in rulebase [:query-nodes query])] (when (= nil query-node) (platform/throw-error (str "The query " query " is invalid or not included in the rule base."))) + (when-not (= (into #{} (keys params)) ;; nil params should be equivalent to #{} + (:param-keys query-node)) + (platform/throw-error (str "The query " query " was not provided with the correct parameters, expected: " (:param-keys query-node) ", provided: " (set (keys params))))) (->> (mem/get-tokens memory query-node params) diff --git a/src/test/clojure/clara/test_compiler.clj b/src/test/clojure/clara/test_compiler.clj index d593865c..1be64500 100644 --- a/src/test/clojure/clara/test_compiler.clj +++ b/src/test/clojure/clara/test_compiler.clj @@ -5,7 +5,8 @@ [clara.rules.engine :as eng] [clojure.string :as str] [clara.rules.accumulators :as acc] - [clojure.main :as m]) + [clojure.main :as m] + [clara.rules.compiler :as com]) (:import [clara.rules.engine AlphaNode TestNode @@ -14,7 +15,8 @@ ProductionNode NegationWithJoinFilterNode ExpressionJoinNode - RootJoinNode])) + RootJoinNode] + [clojure.lang ExceptionInfo])) ;; See https://github.com/cerner/clara-rules/pull/451 for more info (tu/def-rules-test test-nodes-have-named-fns @@ -55,4 +57,22 @@ "-" (:id node))) (-> node-fn str m/demunge (str/split #"/") last))) - (str "For node: " node " and node-fn: " node-fn))))) \ No newline at end of file + (str "For node: " node " and node-fn: " node-fn))))) + +;; See https://github.com/cerner/clara-rules/issues/454 for more info +(deftest test-query-node-requires-bindings-exist + (let [;; (defquery a-query + ;; [:?b] + ;; [?c <- ::a-fact-type]) + query {:lhs [{:type ::a-fact-type + :constraints [] + :args [] + :fact-binding :?c}] + :params #{:?b} + :name "a-query"}] + (try (com/mk-session [[query]]) + (catch ExceptionInfo exc + (is (= (ex-data exc) + {:expected-bindings #{:?b} + :available-bindings #{:?c} + :query "a-query"})))))) \ No newline at end of file diff --git a/src/test/common/clara/test_simple_rules.cljc b/src/test/common/clara/test_simple_rules.cljc index 8a18f2e5..f72d0d5e 100644 --- a/src/test/common/clara/test_simple_rules.cljc +++ b/src/test/common/clara/test_simple_rules.cljc @@ -194,3 +194,33 @@ (is (has-fact? (:cold @side-effect-holder) (->Temperature -10 "MCI") )) (is (has-fact? (:subzero @side-effect-holder) (->Temperature -10 "MCI"))))) + +(def-rules-test test-query-failure-when-provided-invalid-parameters + + {:queries [temp-query [[:?t] [[Temperature (= ?t temperature)]]]] + + :sessions [empty-session [temp-query] {}]} + + (let [session (-> empty-session + (insert (->Temperature 10 "MCI")) + ;; Ensure retracting a nonexistent item has no ill effects. + (retract (->Temperature 15 "MCI")) + fire-rules) + + expected-msg #"was not provided with the correct parameters"] + + ;; passivity test + (is (= #{{:?t 10}} + (set (query session temp-query :?t 10)))) + + (is (thrown-with-msg? #?(:clj IllegalArgumentException :cljs js/Error) + expected-msg + (query session temp-query :?another-param 42))) + + (is (thrown-with-msg? #?(:clj IllegalArgumentException :cljs js/Error) + expected-msg + (query session temp-query))) + + (is (thrown-with-msg? #?(:clj IllegalArgumentException :cljs js/Error) + expected-msg + (query session temp-query :?t 42 :?another-param 42)))))