Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 6 commits into from
Feb 18, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions project.clj
Original file line number Diff line number Diff line change
Expand Up @@ -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)"]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed with:
b41ffee


:scm {:name "git"
:url "https://github.com/cerner/clara-rules"}
Expand Down
14 changes: 9 additions & 5 deletions src/main/clojure/clara/rules/compiler.clj
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,8 @@
Accumulator
ISystemFact]
[java.beans
PropertyDescriptor]
PropertyDescriptor
IndexedPropertyDescriptor]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new import does not seem like it is needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cleaned up:
27e120f

[clojure.lang
IFn]))

Expand Down Expand Up @@ -179,10 +180,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)))])))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not:
dc754f9

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


(defn effective-type [type]
(if (compiling-cljs?)
Expand Down
38 changes: 38 additions & 0 deletions src/main/java/clara/test/facts/BeanTestFact.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

* 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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps add the issue link to this comment as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added:
2d81c3e

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

public void setRoadConditions(int pos, String condition) {
roadConditions[pos] = condition;
}
public String getRoadConditions(int pos){
return roadConditions[pos];
}
}
29 changes: 29 additions & 0 deletions src/test/clojure/clara/test_java_facts.clj
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
(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]])
(:import [clara.test.facts
BeanTestFact]))

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

;; 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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did this for consistency

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{:rules [kansas-rule [[[BeanTestFact
(= ?locs locations)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

(some #(= "Kansas" %) ?locs)]]
(rules/insert! "Kansas Exists")]]
:queries [string-query [[] [[?s <- String]]]]

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

(let [locs (make-array String 2)]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done:
1a12065

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

(aset locs 0 "Florida")
(aset locs 1 "Kansas")
(let [session-strings (into #{}
(map :?s)
(-> empty-session
(rules/insert (BeanTestFact. locs))
(rules/fire-rules)
(rules/query string-query)))]
(is (= 1 (count session-strings)))
(is (contains? session-strings "Kansas Exists")))))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated:
1a12065

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1