oakes / odoyle-rules

A rules engine for Clojure(Script)
The Unlicense
530 stars 20 forks source link

Rules fire despite :then not= option #16

Closed thomascothran closed 1 year ago

thomascothran commented 2 years ago

I have a ruleset that looks like this:

(declare place-id')
(declare places')

(o/ruleset
    #...
    ::associate-place-to-persons
    [:what
     [place-id    :domain.place/id      place-id {:then false}]
     [person-id   :domain.person/place  place-id {:then not=}]
     [:global     :domain/places        places  {:then false}]
     [:global     :domain/persons       persons {:then false}]
     :then
     (do (clojure.pprint/pprint {:place-id place-id
                                 :same-thing? (= place-id' place-id)})
         (def place-id' place-id)
         (->> (assoc-in places [place-id :domain.place/persons person-id]
                        (get persons person-id))
              (o/insert! :global :domain/places)))]

    ::unpack-places
    [:what [:global :domain/places places {:then not=}]
     :then
     (do (clojure.pprint/pprint {:places-same? (= places places')})
         (def places' places)
         (->> places
              (mapv (fn [[place-id place]]
                      (o/insert! place-id place))))))
    #_...  

As I understand it, the :same-thing? and places-same? should always be false. The inline defs for place-id' and places' allow us to compare the previous matches.

However, :same-thing? and places-same? are false the first time, and then these rules re-trigger each other until the recursion limit hits.

oakes commented 2 years ago

Yeah you can see this issue explained in this comment: https://github.com/oakes/odoyle-rules/blob/1009ef700a54bd4c8484a069cb6e652d1ed93d51/test/odoyle/rules_test.cljc#L729

The workaround is to not do a join between the id and value place-id, but instead enforce that via the :when block. I'm investigating a way to show a warning about this; there should be a way for me to detect it and show a message.

thomascothran commented 2 years ago

Thanks, @oakes. However, I think there's another issue at play as well.

I removed the join at (1):

(o/ruleset
  #...
  ::associate-place-to-persons
  [:what
   #_[place-id    :domain.place/id      place-id {:then false}]     ;; (1) - removed join
   [person-id   :domain.person/place  place-id {:then not=}]
   [:global     :domain/places        places  {:then false}]
   [:global     :domain/persons       persons {:then false}]
   :then
   (do (clojure.pprint/pprint {:place-id    place-id
                               :same-thing? (= place-id' place-id)})
       (def place-id' place-id)
       (->> (assoc-in places [place-id :domain.place/persons person-id]
                      (get persons person-id))
            (o/insert! :global :domain/places)))]

  ::unpack-places
  [:what [:global :domain/places places {:then not=}]
   :then
   (do (clojure.pprint/pprint {:places-same? (= places places')})
       (def places' places)
       (->> places
            (mapv (fn [[place-id place]]
                    (o/insert! place-id place)))))]
  ::fetcher-rule                                            ;; (2) ;; - rule I previously omitted from the example
  [:what [id :domain.place/id id {:then not=}] ;; (3)
   :then-finally
   (do (println "Fetching places")
       (->> (o/query-all o/*session* ::fetch-places)
            fetch-places-from-db
            (reduce (fn [acc {id :domain.place/id :as p}]
                      (assoc acc id p))
                    {})
            (o/insert o/*session* :global :domain/places)
            o/reset!))]
  )

However, the ::associate-place-to-persons rule still fires until the recursion limit is hit.

The error message says that ::associate-place-to-persons is invoking itself. However, after some debugging, there's another rule (at (2)) with a :then-finally block which is firing (but not indicated in the error message).

There are a few things I did wrong here, I think. One is that I used the :then not= option (at (3)), thinking it would prevent the then-finally rule from firing.

But I suspect there's still an underlying problem here, where the update in a :then-finally block of a rule can cause another rule to not obey the {:then not=} option.

thomascothran commented 2 years ago

Narrowing in on the problem a bit. The fetch-places rule was refiring itself in a loop. Here's what I found:


::fetch-places
[:what [id :domain.place/id id {:then not=}] ;; (1) not= doesn't work
 :then-finally
 (let [places
       (->> (o/query-all o/*session* ::fetch-places)
            fetch-places-from-db
            (reduce (fn [acc {id :domain.place/id :as p}]
                      (assoc acc id p))
                    {}))
       insert-ids
       (fn [session]
         (->> (keys places)
              (reduce (fn [session' id]
                        (o/insert session' id :domain.place/id id))
                      session)))]
   (->> (o/insert o/*session* :global :domain/places places)
        insert-ids
        o/reset!))]

However, if the :what block is changed to this:

 [_ :domain.place/id id {:then not=}] 

Now I see the join you were talking about was within a single :what vector, not across vectors like I had originally (incorrectly) thought.

I can put together a PR not note this problem in the docs. Is the idea that the not= condition should applied to the value and not the id, or that in the future it may be modified and this is a workaround?

oakes commented 2 years ago

The technical reason for this is that if a value is part of a join, and that value is updated, i cannot safely update it in place because its new value could change the validity of the join. So instead i have to fully retract and recreate the match (internally this is called disable-fast-updates). Since it is momentarily retracted, the not= condition can't be run; it's as if the match is completely new. By removing the join and enforcing that equality via the :when block, this no longer happens.

I'll add this to the readme. Maybe i can also try to detect it and log a warning. It should be easy enough to detect -- i could just look for :what tuples that contain a join and are using a :then option. I'm wary about logging though, since it could be an unwelcome side effect if these rules are being added in a hot loop somewhere. But maybe i could log it from within the ruleset macro, so it'll only happen at compile time. Gotta think about it more...

thomascothran commented 2 years ago

@oakes

I do think logging would be useful; I was running into this and working around it by trial and error in some other places, without realizing the root cause. I'm happy to take a look as well, since I'm starting to use O'Doyle heavily (and loving it). I don't have much of an understanding of the internals yet though.

oakes commented 1 year ago

I decided to just make it throw an exception in this case starting in the next version, so this will no longer be a surprising edge case. I'll close this since it is now fixed in https://github.com/oakes/odoyle-rules/commit/43b0e831c2b489db2597786c93d90f185dc4c792