Skip to content

#264:Provide better exception when unable to resolve class while dese… #476

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

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

#264:Provide better exception when unable to resolve class while dese… #476

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6ab490b
#264:Provide better exception when unable to resolve class while dese…
Jul 19, 2022
afb0604
record -> fact type
EthanEChristian Jul 21, 2022
cff17c5
record -> fact type
EthanEChristian Jul 21, 2022
5252ca5
Correct terminology around class
EthanEChristian Nov 11, 2022
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
6 changes: 5 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 @@ -132,7 +132,11 @@
([^Reader rdr]
(read-record rdr nil))
([^Reader rdr add-fn]
(let [builder (-> (.readObject rdr) resolve deref)
(let [try-resolve #(if-let [clazz (resolve %)]
Copy link
Member Author

Choose a reason for hiding this comment

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

could be named resolve-or-fail

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 used elsewhere I think?

Copy link
Member Author

Choose a reason for hiding this comment

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

not in this repo, but it is around ;)

clazz
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this is a class, but rather a Clojure Var for the record constructor function. From the implementation before this PR:

(defn write-record
  "Same as write-with-meta, but with Clojure record support.  The type of the record will
   be preserved."
  [^Writer w tag rec]
  (let [m (meta rec)]
    (.writeTag w tag 3)
    (.writeObject w (record-map-constructor-name rec) true)
    (write-map w rec)
    (if m
      (.writeObject w m)
      (.writeNull w))))

Then looking at the current read implementation:

(defn read-record
  "Same as read-with-meta, but with Clojure record support.  The type of the record will
   be preserved."
  ([^Reader rdr]
   (read-record rdr nil))
  ([^Reader rdr add-fn]
   (let [builder (-> (.readObject rdr) resolve deref)
         build-map (.readObject rdr)
         m (read-meta rdr)]
     (cond-> (builder build-map)
       m (with-meta m)
       add-fn add-fn))))

If the resolved value was a class it wouldn't be possible to invoke it as a Clojure function - note the (builder build-map) call.

Assuming I'm not missing something, this makes testing easier if you're so inclined, though for this sort of error handling I don't think it is critical. You could just use ns-unmap before deserializing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a good point. So the suggestion (besides testing) is that the resolved symbol isn't a class at all and the message should avoid being worded suggesting that it is. Also, we shouldn't call it clazz, but perhaps just resolved or resolved-var or something like that? It is notable that deref is called on it a step later, so it definition can't be a class.

Copy link
Member Author

Choose a reason for hiding this comment

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

corrected:
5252ca5

Copy link
Member Author

Choose a reason for hiding this comment

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

todo:
add a test

(throw (ex-info (str "Unable to resolve fact type symbol: '" % "'")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this is necessarily a fact, although if the Fressian handlers are being brought in by an IWorkingMemorySerializer implementation they usually will be - the Clara engine nodes have their own handlers anyway: https://github.com/cerner/clara-rules/blob/0.21.2/src/main/clojure/clara/rules/durability/fressian.clj#L393 So this seems OK either way as "fact type symbol" or "record constructor symbol".

{:fact-type-class %})))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this is specific to facts? It is being used for all Clojure records from what I can see here (admittedly it has been a while since I read through this): https://github.com/cerner/clara-rules/blob/0.21.2/src/main/clojure/clara/rules/durability/fressian.clj#L386

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you are right. It really is a failure to resolve a record builder fn.

Copy link
Member Author

Choose a reason for hiding this comment

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

corrected:
5252ca5

builder (-> (.readObject rdr) try-resolve deref)
build-map (.readObject rdr)
m (read-meta rdr)]
(cond-> (builder build-map)
Expand Down