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

[EthanEChristian]

…rializing rulebase

#264

[EthanEChristian]

could be named resolve-or-fail

[mrrodriguez]

That’s used elsewhere I think?

[EthanEChristian]

not in this repo, but it is around ;)

[mrrodriguez]

I have minor suggestions.

[mrrodriguez]

That’s used elsewhere I think?

[EthanEChristian]

I plan on merging this this weekend to allow for a little additional time for @WilliamParker if he is able to.

[WilliamParker]

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.

[mrrodriguez]

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.

[EthanEChristian]

corrected:
5252ca5

[EthanEChristian]

todo:
add a test

[WilliamParker]

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

[mrrodriguez]

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

[EthanEChristian]

corrected:
5252ca5

[EthanEChristian]

correct terminology

[WilliamParker]

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".