Issue446: Bean accessors don't account for IndexedPropertyDescriptors #449

EthanEChristian · 2020-02-01T03:23:49Z

A PR for #446

mrrodriguez · 2020-02-04T21:26:13Z

project.clj

@@ -78,6 +78,9 @@
                                            (some->> x :ns ns-name str (re-matches (re-pattern (apply str patterns)))))))
                   :generative (fn [x] (some->> x :ns ns-name str (re-matches #"^clara\.generative.*")))
                   :performance (fn [x] (some->> x :ns ns-name str (re-matches #"^clara\.performance.*")))}
+
+  ;; Remove any "Test Facts" from being included in the jar.
+  :jar-exclusions [#".*TestFact\.(java|class)"]


They really shouldn't even be in the "main" src path, but that's a topic for another build related issue I think.

Any reason this was added in this particular PR though?

removed with:
b41ffee

mrrodriguez · 2020-02-04T21:27:28Z

src/main/java/clara/test/facts/BeanTestFact.java

+/**
+ * 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


Why can't it be under src/test/java instead? ie, a dev-time-only :source-paths?

Agreed, I don't understand why this needs to be in the main source path and then removed from the jar afterward rather than just put in a test source path from the start.

My lein is not that strong, but yeah that works:
b41ffee

mrrodriguez · 2020-02-04T21:28:43Z

src/test/clojure/clara/test_java_facts.clj

+
+   :sessions [empty-session [kansas-rule string-query] {}]}
+
+  (let [locs (make-array String 2)]


You could use doto around make-array to do the aset's all in one go and make it seem somewhat more "functional/immutable" in style

done:
1a12065

mrrodriguez · 2020-02-04T21:29:09Z

src/test/clojure/clara/test_java_facts.clj

+;; see https://github.com/cerner/clara-rules/issues/446
+(tu/def-rules-test test-basic-rule
+  {:rules [kansas-rule [[[BeanTestFact
+                          (= ?locs locations)


So would direct field access on roadConditions not work since it has no standard accessor?

That's what I'd think yes. A test that an actual invocation of the indexed accessor in a rule for the property without a standard accessor still works might be nice to have and wouldn't be hard to create but isn't crucial. I realize it should still pass with this implementation, I just tend to prefer having tests be implementation agnostic where possible.

added a test demonstrating that a compilation error is thrown when trying to use the standard accessor:
1a12065

mrrodriguez · 2020-02-04T21:31:18Z

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

@@ -31,7 +31,8 @@
            Accumulator
            ISystemFact]
           [java.beans
-            PropertyDescriptor]
+            PropertyDescriptor
+            IndexedPropertyDescriptor]


This new import does not seem like it is needed.

cleaned up:
27e120f

WilliamParker

A few minor suggestions and something to check re reflection but this looks basically sound to me.

WilliamParker · 2020-02-12T09:11:06Z

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

+              ;; 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)))])))


You'll want to be careful that nothing here adds new reflection. Perhaps compile a session (thus doing evals) with reflection warnings turned on. This would be a good test to have actually though perhaps a bit out of scope here; as far as I know we don't have a test for this.

Adding

:global-vars {*warn-on-reflection* true}

to the project and building doesn't show this as a reflection.
To test that this is correct, i removed the type hint for PropertyDescriptor and i get the following:

Reflection warning, clara/rules/compiler.clj:179:33 - reference to field getReadMethod on java.lang.Object can't be resolved.
Reflection warning, clara/rules/compiler.clj:183:36 - reference to field getName on java.lang.Object can't be resolved.
Reflection warning, clara/rules/compiler.clj:184:29 - reference to field getName can't be resolved.
Reflection warning, clara/rules/compiler.clj:179:33 - reference to field getReadMethod can't be resolved.
Reflection warning, clara/rules/compiler.clj:183:36 - reference to field getName can't be resolved.
Reflection warning, clara/rules/compiler.clj:184:29 - reference to field getName can't be resolved.

So, i think we are good for this example.

Yep, doesn't look like reflection.

I think we should just have

:global-vars {*warn-on-reflection* true}

at the top-level of our lein project.clj though in general

why not:
dc754f9

To be clear, my concern wasn't necessarily with reflection warnings in Clara itself but rather with reflection when invoking accessors on Java objects. For example:

(defrule testrule [JavaBeanWithFieldA (= a "some value")] => ;; do something )

reflecting when invoking the .getA method behind the scenes. However in any case if that were happening I'd expect to see more warnings in the Travis build with global reflection warnings enabled - in particular, warnings interspersed through test execution when eval calls build rule networks. So +1.

Ah, yeah that makes sense. I believe the reason that the rules network fns don't experience reflection is because we add the metadata a bit later:
https://github.com/cerner/clara-rules/blob/1bafa05c2502eb452bdd765c1b45c158725f575e/src/main/clojure/clara/rules/compiler.clj#L390

WilliamParker · 2020-02-12T09:11:51Z

src/main/java/clara/test/facts/BeanTestFact.java

+/**
+ * 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


Agreed, I don't understand why this needs to be in the main source path and then removed from the jar afterward rather than just put in a test source path from the start.

WilliamParker · 2020-02-12T09:14:17Z

src/test/clojure/clara/test_java_facts.clj

+;; see https://github.com/cerner/clara-rules/issues/446
+(tu/def-rules-test test-basic-rule
+  {:rules [kansas-rule [[[BeanTestFact
+                          (= ?locs locations)


That's what I'd think yes. A test that an actual invocation of the indexed accessor in a rule for the property without a standard accessor still works might be nice to have and wouldn't be hard to create but isn't crucial. I realize it should still pass with this implementation, I just tend to prefer having tests be implementation agnostic where possible.

WilliamParker · 2020-02-12T09:15:12Z

src/test/clojure/clara/test_java_facts.clj

+            [clojure.test :refer [is deftest run-tests testing use-fixtures]])
+  (:import [clara.test.facts
+            BeanTestFact]))
+


This test seems like it could go in the test_java namespace as well, but I don't have strong feelings about it.

I was on the fence about that, I kind of felt like the test_java namespace was testing the Java apis rather than the usage of java roots in the rulebase and thats why i created a new namespace.

WilliamParker · 2020-02-12T09:16:36Z

src/test/clojure/clara/test_java_facts.clj

+
+;; 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


Note that using def-rules-test isn't really needed here since this presumably isn't going to be tested in ClojureScript, but it doesn't do any harm.

I did this for consistency

Might as well test all that we can test on all platforms.

WilliamParker · 2020-02-12T09:19:54Z

src/test/clojure/clara/test_java_facts.clj

+                                    (rules/fire-rules)
+                                    (rules/query string-query)))]
+      (is (= 1 (count session-strings)))
+      (is (contains? session-strings "Kansas Exists")))))


A slightly more concise option:

(let [session-strings (map :?s (-> empty-session (rules/insert (BeanTestFact. locs)) (rules/fire-rules) (rules/query string-query)))] (is (= ["Kansas"] session-strings)))

updated:
1a12065

WilliamParker · 2020-02-12T09:33:26Z

src/main/java/clara/test/facts/BeanTestFact.java

+        return locations[pos];
+    }
+
+    // Partial Indexed property accessor, ie. no standard accessor


Perhaps add the issue link to this comment as well.

added:
2d81c3e

mrrodriguez · 2020-02-13T19:42:18Z

project.clj

@@ -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/main/java"


You don't have to repeat "src/main/java". It's already at the root under :java-source-paths. :dev profile will merge on top of that.
lein merges collections together if you don't have metadata on them like :replace or :displace.

updated:
dc754f9

mrrodriguez · 2020-02-13T19:43:13Z

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

@@ -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]


Were these just unused ns's to clean up?

yes, might as well clean up while im here

mrrodriguez

It's a yes from me dawg

WilliamParker

+1 to merge

WilliamParker · 2020-02-16T20:44:21Z

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

+              ;; 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)))])))


To be clear, my concern wasn't necessarily with reflection warnings in Clara itself but rather with reflection when invoking accessors on Java objects. For example:

(defrule testrule [JavaBeanWithFieldA (= a "some value")] => ;; do something )

reflecting when invoking the .getA method behind the scenes. However in any case if that were happening I'd expect to see more warnings in the Travis build with global reflection warnings enabled - in particular, warnings interspersed through test execution when eval calls build rule networks. So +1.

WilliamParker · 2020-02-16T20:46:39Z

src/main/java/clara/test/facts/BeanTestFact.java

+        return locations[pos];
+    }
+
+    // Partial Indexed property accessor, ie. no standard accessor


WilliamParker · 2020-02-16T20:49:16Z

src/test/clojure/clara/test_java_facts.clj

+;; see https://github.com/cerner/clara-rules/issues/446
+(tu/def-rules-test test-basic-rule
+  {:rules [kansas-rule [[[BeanTestFact
+                          (= ?locs locations)


WilliamParker · 2020-02-16T20:49:33Z

src/test/clojure/clara/test_java_facts.clj

+
+   :sessions [empty-session [kansas-rule string-query] {}]}
+
+  (let [locs (make-array String 2)]


WilliamParker · 2020-02-16T20:49:51Z

src/test/clojure/clara/test_java_facts.clj

+                                    (rules/fire-rules)
+                                    (rules/query string-query)))]
+      (is (= 1 (count session-strings)))
+      (is (contains? session-strings "Kansas Exists")))))


Issue446: Bean accessors don't account for IndexedPropertyDescriptors

4bd9490

EthanEChristian mentioned this pull request Feb 1, 2020

Bean accessors don't account for IndexedPropertyDescriptors #446

Closed

EthanEChristian requested review from WilliamParker and mrrodriguez February 1, 2020 03:25

mrrodriguez requested changes Feb 4, 2020

View reviewed changes

WilliamParker reviewed Feb 12, 2020

View reviewed changes

EthanEChristian self-assigned this Feb 13, 2020

ec027900 added 4 commits February 12, 2020 19:19

Move test java class to test root

b41ffee

Clean up requires and imports

27e120f

Add comment on java class

2d81c3e

Add/Modify tests

1a12065

mrrodriguez reviewed Feb 13, 2020

View reviewed changes

mrrodriguez approved these changes Feb 13, 2020

View reviewed changes

Update project.clj

dc754f9

WilliamParker approved these changes Feb 16, 2020

View reviewed changes

EthanEChristian merged commit bda21fe into oracle-samples:master Feb 18, 2020

EthanEChristian deleted the Issue_446 branch February 18, 2020 00:18


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

		(let [locs (make-array String 2)]

Issue446: Bean accessors don't account for IndexedPropertyDescriptors #449

Issue446: Bean accessors don't account for IndexedPropertyDescriptors #449

Conversation

EthanEChristian commented Feb 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WilliamParker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrrodriguez Feb 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrrodriguez left a comment

Choose a reason for hiding this comment

WilliamParker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EthanEChristian commented Feb 1, 2020 •

edited

Loading

mrrodriguez Feb 13, 2020 •

edited

Loading