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.

Sign up for GitHub

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

Fix RuleOrderedActivation Fressian Handler #395

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix RuleOrderedActivation Fressian Handler #395

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5480524
Fix RuleOrderedActivation Fressian Handler
mmotter Jun 7, 2018
3b86cf9
- implement protocol function use-token-identity? for proctol Identit…
mmotter Jun 11, 2018
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ This is a history of changes to clara-rules.

### 0.19.0-SNAPSHOT
* Remove a warning about `qualified-keyword?` being replaced when using Clojure 1.9.
* Fix RuleOrderedActivation Fressian Handler. See [issue 394](https://github.com/cerner/clara-rules/issues/394)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we indicate directly in the changelog that the functional impact here is very limited as discussed at #394 (comment) and #395 (review) ? I'd like to avoid unnecessary user concern. Something like "The bug is only exposed if reusing the Clara Fressian handlers outside of Clara's direct APIs"


### 0.18.0
* Remove unnecessary memory operations from ProductionNode to optimize performance. Remove :rule-matches in session inspection that did not cause logical insertions and add a new optional feature to return all rule matches, regardless of what their RHS did or whether they were retracted. Add a new listener and tracing method fire-activation!. These changes are moderately non-passive with respect to listening, tracing, and session inspection but are otherwise passive. See [issue 386](https://github.com/cerner/clara-rules/issues/386) for details.
Expand Down
1 change: 1 addition & 0 deletions CONTRIBUTORS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ Cerner Corporation
- William Parker [@WilliamParker]
- Ethan Christian [@EthanEChristian]
- Pushkar Kulkarni [@kulkarnipushkar]
- Matt Motter [@mmotter]

Community

Expand Down
4 changes: 3 additions & 1 deletion src/main/clojure/clara/rules/durability/fressian.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -492,11 +492,13 @@
(.writeObject w (.-node-id ^RuleOrderedActivation c) true)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we recap the discussion about the non-usage of this handler in a sentence or two of comments inline here?

(.writeObject w (.-token ^RuleOrderedActivation c))
(.writeObject w (.-activation ^RuleOrderedActivation c))
(.writeInt w (.-rule-load-order ^RuleOrderedActivation c))))
(.writeInt w (.-rule-load-order ^RuleOrderedActivation c))
(.writeBoolean w (.-use-token-identity? ^RuleOrderedActivation c))))
Copy link
Collaborator

@mrrodriguez mrrodriguez Jun 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Clojure makes fields with metadata :unsynchronized-mutable private by default.
RuleOrderedActivation.use-token-identity? is marked with this metadata, so it is private.

I'd expect 2 things here:

  1. a reflection warning at compile time for the call (.-use-token-identity? ^RuleOrderedActivation c) since that private field isn't exposed so the clj compiler cannot emit an appropriate callsite for it.
  2. a runtime exception thrown when this is evaluated due to the field not being found (via reflection due to (1) already happening).

In clara.rules.memory you'll see a protocol IdentityComparable that is used in order to be able to implement at "setter" on RuleOrderedActivation for this private field.

There is no protocol function implemented that gives access to this field however. It may be reasonable to add another function to the IdentityComparable protocol, such as use-token-identity? that will return the value of this field for serialization. This protocol is really only an internal impl detail around this optimization, so that should be fine.

It wouldn't be too difficult to test this handler "round tripping" a RuleOrderedActivation to expose the problem.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Example of testing it:

(require 'clara.test-fressian)
(in-ns 'clara.test-fressian)
(require '[clara.rules.memory :as mem])

(let [act (mem/->RuleOrderedActivation 1 nil nil 1 false)]
  (test-serde act))

:readers {"clara/ruleorderactivation"
(reify ReadHandler
(read [_ rdr tag component-count]
(mem/->RuleOrderedActivation (.readObject rdr)
(.readObject rdr)
(.readObject rdr)
(.readObject rdr)
(.readObject rdr))))}}
Expand Down