mpdairy / posh

A luxuriously simple and powerful way to make front-ends with DataScript and Reagent in Clojure.
Eclipse Public License 1.0
460 stars 45 forks source link

Posh with Datomic #17

Open alexandergunnarson opened 7 years ago

alexandergunnarson commented 7 years ago

UPDATE: Posh transactions and reactive queries work with both DataScript and Datomic! This can be merged if you'd like.

mpdairy commented 7 years ago

Ok I'll take a look at it on Monday, though I'm not very experienced with Datomic.

Also, I was wondering, why did you have to redo the Reagent ratom and reaction stuff for the backend? Posh core already functionally calculates changed q's and pull's, and it looks like you're using that: (https://github.com/mpdairy/posh/blob/master/src/posh/plugin_base.cljc#L41)

alexandergunnarson commented 7 years ago

So plugin_base.cljc is simply a platform-agnostic version of what was formerly reagent.cljs — I didn't write any of it (the diff is misleading and should really just say I renamed the file and added a few things here and there). I just added dcfg as the first param in the appropriate locations/functions. reagent.cljs originally used reagent reactive atoms in order to make the reactivity work (reagent.ratom/make-reaction is an indispensable dependency for make-query-reaction, and thus for q and pull), so I ported reagent.ratom/make-reaction to .cljc (now in lib/ratom.cljc) so it could be used in backend/Clojure contexts. Do you feel like there might have been a different way to make Posh work in the backend?

mpdairy commented 7 years ago

So how are you planning to use the reactions on the server side? Is this for front-end stuff in clj or is it for communicating changes to the front-end? if it's for the latter, wouldn't you want to send the datoms that caused the reaction rather than the result of the pull or q (which is returned by the reaction).

alexandergunnarson commented 7 years ago

The latter, and yes, returning the datoms (i.e. only the diff) would be nice (really, only the datoms affected by the registered query/pull expressions — not all the datoms from the transaction report). However, what I'd optimally like to do is to have the various clients register (and deregister) their query/pull expressions, have posh figure out who needs to get notified with what datoms, and send those to the appropriate clients. I suppose I haven't fully thought through yet how to do that. I haven't dug through posh's internals (planning to soon), but I assume there's quite a bit of logic in there to figure out, given some new datoms from a transaction report, which reactive expressions need to be notified. Maybe we can leverage that? Again, I haven't fully thought this through yet. Thoughts from your side?

Oh, and also, as for uses of reactive expressions — frontend stuff in clj would potentially be useful, yes, but another thing I'm envisioning with reactive queries is in situations in which the frontend isn't even involved. This might be something like incremental query recalculation for an expensive query (say, in an analytics context where no frontend is involved). In a simpler situation, it could also allow non-incremental query recalculation with efficient change notifications (i.e. no 'query polling'). There are likely other situations in which reactive queries would be useful.

mpdairy commented 7 years ago

Well you'll be happy to that I had a similar vision for the posh backend regarding sending only the necessary datoms and I added some core functionality to make that possible. It's a complicated problem because often times you have to send more than just the tx'd datom; for example, a q might be getting all the cars in a lot, and you add a new car to the lot by just changing the :car/lot ref, although only:car/lot was tx'd you really have to send that and all the new info q will find about that car. So I figured out a way to get all the datoms that affect the result of a q and a pull, and it's just some option that goes into the query graph when you create it initially in posh core. Probably usually on the backend you'll just want those datoms and won't even have to get the result, since that gets calc'ed on the front-end.

So, although I don't remember all the details, what I imagined you'd do on the backend is set up a filtered db for each user (for security restrictions) and then start a query graph from posh core for each user, The query graph can be synced between client and server because you can get it as plain data (through core), but you'd have to do something to make sure the entity ids are in sync.

I think @metasoarous said he was getting something like this working with Datsync ("optimized updates", I think he said), but I'm not sure.

seantempesta commented 7 years ago

Any updates on this? I know you guys are going for the more ambitious "send all relevant datoms to the client", but I'd just be happy with the forwarding the answer to the query. Is that already working?

metasoarous commented 7 years ago

Howdy; @alexandergunnarson Awesome work!

So far, keeping entity ids in sync is one of the primary concerns of Datsync. And I've recently taken a new approach on this for true sync (local first transactions, synced based on the tx-log), with lookup refs instead of manual tracking of entity ids. I made this switch so that P2P sync wouldn't be forcluded in the design.

As for the filters and such (optimized updates; whatevs); Yes, I've been in hammock mode trying to think things through carefully this go around, and think I'm on the verge of settling some of these issues. One of my big realizations of late is that I've decided to leverage Onyx for describing the flow of data on clients and server, now that we have the local runtime. This has some nice consequences:

I wish I was further along on this than I am, but times have been busy. I've been working on a blog post to try and get the hammock portion of this work all sketched out for feedback. After that I hope to start building out some of the basic building blocks. @alexandergunnarson Your work here will be a major help!

Feel free to chat me up more about the bits I've been working on over Datsync Gitter chat or GH issues.

@seantempesta I'd like to have direct syncing of query results (without transacting into a db) as an option for Datsync.

seantempesta commented 7 years ago

Okay, so I don't really understand the Posh codebase, but when I run the test this is the tx-report I'm seeing:

(d/transact conn* [{:db/id     (tempid)
                     :test/attr "Abcde"}])
=>
#object[datomic.promise$settable_future$reify__6470
        0x77a3d068
        {:status :ready,
         :val {:db-before datomic.db.Db,
               @177783d2 :db-after,
               datomic.db.Db @3b5378a6,
               :tx-data [#datom[13194139534314 50 #inst"2017-01-13T22:27:47.953-00:00" 13194139534314 true]
                         #datom[277076930200555 64 "Abcde" 13194139534314 true]],
               :tempids {-9223090561879065154 277076930200555}}}]

It looks like the attribute :test/attr is being reported as it's underlying entity id (64 in this case). Maybe that's why it isn't matching?

seantempesta commented 7 years ago

Looking up the attribute via d/ident fixed it. Although there's still a race condition between the thread picking up the change and updating the ratom, so the test fails unless you execute the statements by hand. I'm a git noob, so I have no idea how to update a pull request, but here are the relevant changes:

/posh/clj/datomic.clj

-(defn datom->seq [^datomic.Datom d]
-  [(.e d) (.a d) (.v d) (.tx d) (.added d)])

+(defn datom->seq [db-after ^datomic.Datom d]
+  [(.e d) (d/ident db-after (.a d)) (.v d) (.tx d) (.added d)])
-            (try (let [txn-report' (update txn-report :tx-data
-                                     #(map datom->seq %))]

+            (try (let [db-after (:db-after txn-report)
+                       txn-report' (update txn-report :tx-data 
+                                     (fn [datoms] 
+                                         (map #(datom->seq db-after %) datoms)))]

;; Execute each of these statements one at a time and the test works.
;; Looks like there's just a race condition between when the thread
;; see the change and updates the ratom

(d/create-database "datomic:mem://test")
(def conn* (d/connect "datomic:mem://test"))
(def poshed (db/posh! conn*))
(def conn (-> poshed :conns :conn0))
(db/start conn)
(instance? PoshableConnection conn)
(db/transact! conn (install-partition default-partition))
(transact-schemas! conn
                   [{:db/ident       :test/attr
                     :db/valueType   :db.type/string
                     :db/cardinality :db.cardinality/one}])
(def sub (db/q '[:find ?e
                 :where [?e :test/attr]]
               conn))
(= @sub #{})
(db/transact! conn
              [{:db/id     (tempid)
                :test/attr "Abcde"}])
(= @sub
   @(db/q [:find '?e
           :where ['?e :test/attr]]
          conn)
   (d/q [:find '?e
         :where ['?e :test/attr]]
        (db/db* conn)))
(db/stop conn)
alexandergunnarson commented 7 years ago

Super helpful @seantempesta ! Great job finding the issue. I'll add that commit in today.

seantempesta commented 7 years ago

Thanks! I'm planning on using this feature so let me know if there's other stuff you need help on.

seantempesta commented 7 years ago

Um, so how do we detect changes to the ratom subscription? I'm used to having reagent do this automagically as long as I deref'ed it in the render function.

alexandergunnarson commented 7 years ago

@seantempesta It's possible to detect changes if you deref within a reaction, or (anywhere, including outside of a reaction) if you use add-watch (verified by looking at the code). You can call add-watch on a ratom as well as a reaction.

alexandergunnarson commented 7 years ago

@seantempesta Very astute point about the race condition. I was wondering whether the single-threadedness of the ratoms might pose an issue and it looks like indeed it does. I'll make a thread-safe version later today (no ^:unsynchronized-mutable hints, which are safe in CLJS but not in CLJ).

I guess the ratoms are being modified by the PoshableConnection thread (via the Posh callbacks registered as listeners) as well as the original (run-tests) thread (via posh/transact!)?

seantempesta commented 7 years ago

So I've tried both add-watch and run! and neither is re-run after a change happens. They only run after you deref the ratom.

(def sub (db/q '[:find ?e ?pfn ?pf ?pda
                   :where
                   [?e :participant/firstName ?pfn]
                   [?e :participant/lastName "Tempesta"]
                   [?e :participant/photo ?pf]
                   [?e :participant/dateAdded ?pda]]
                 conn))
=> #'kidlink-server.db.posh/sub
(add-watch sub :watcher
             (fn [key atom old-state new-state]
               (log/debug "-- Atom Changed --")
               (log/debug "key" key)
               (log/debug "atom" atom)
               (log/debug "old-state" old-state)
               (log/debug "new-state" new-state)))
=> nil
(ratom/run! (log/debug "running reaction!" @sub))
2017-01-16 12:42:56,886 [nREPL-worker-0] DEBUG kidlink-server.db.posh - -- Atom Changed -- 
2017-01-16 12:42:56,888 [nREPL-worker-0] DEBUG kidlink-server.db.posh - key :watcher 
=> #object[posh.lib.ratom.Reaction 0x2b91d3d1 {:status :ready, :val nil}]
2017-01-16 12:42:56,891 [nREPL-worker-0] DEBUG kidlink-server.db.posh - atom #object[posh.lib.ratom.Reaction 0x73cb639e {:status :ready, :val #object[java.util.HashSet 0x1b8ed792 [[17592186047976 Sean nophoto #inst "2016-09-29T08:38:59.296-00:00"]]]}] 
2017-01-16 12:42:56,891 [nREPL-worker-0] DEBUG kidlink-server.db.posh - old-state nil 
2017-01-16 12:42:56,892 [nREPL-worker-0] DEBUG kidlink-server.db.posh - new-state #object[java.util.HashSet 0x1b8ed792 [[17592186047976 Sean nophoto #inst "2016-09-29T08:38:59.296-00:00"]]] 
2017-01-16 12:42:56,892 [nREPL-worker-0] DEBUG kidlink-server.db.posh - running reaction! #object[java.util.HashSet 0x1b8ed792 [[17592186047976 Sean nophoto #inst "2016-09-29T08:38:59.296-00:00"]]] 

Now I'm changing the underlying data

(db/transact! conn  [{:db/id 17592186047976
                      :participant/firstName "Sean-changed"}])
=>
{:db-before datomic.db.Db,
 @60037661 :db-after,
 datomic.db.Db @e5b8c13d,
 :tx-data [#datom[13194139536881 50 #inst"2017-01-16T17:43:25.177-00:00" 13194139536881 true]
           #datom[17592186047976 94 "Sean-changed" 13194139536881 true]
           #datom[17592186047976 94 "Sean" 13194139536881 false]],
 :tempids {}}

At this point, neither the add-watch or the run! function have executed, and they won't unless I manually deref the atom.

@sub
2017-01-16 12:43:50,628 [nREPL-worker-1] DEBUG kidlink-server.db.posh - -- Atom Changed -- 
2017-01-16 12:43:50,632 [nREPL-worker-1] DEBUG kidlink-server.db.posh - key :watcher 
2017-01-16 12:43:50,632 [nREPL-worker-1] DEBUG kidlink-server.db.posh - atom #object[posh.lib.ratom.Reaction 0x73cb639e {:status :ready, :val #object[java.util.HashSet 0x4cff9be1 [[17592186047976 Sean-changed nophoto #inst "2016-09-29T08:38:59.296-00:00"]]]}] 
2017-01-16 12:43:50,633 [nREPL-worker-1] DEBUG kidlink-server.db.posh - old-state #object[java.util.HashSet 0x1b8ed792 [[17592186047976 Sean nophoto #inst "2016-09-29T08:38:59.296-00:00"]]] 
2017-01-16 12:43:50,633 [nREPL-worker-1] DEBUG kidlink-server.db.posh - new-state #object[java.util.HashSet 0x4cff9be1 [[17592186047976 Sean-changed nophoto #inst "2016-09-29T08:38:59.296-00:00"]]] 
2017-01-16 12:43:50,634 [nREPL-worker-1] DEBUG kidlink-server.db.posh - running reaction! #object[java.util.HashSet 0x4cff9be1 [[17592186047976 Sean-changed nophoto #inst "2016-09-29T08:38:59.296-00:00"]]] 
=> #{[17592186047976 "Sean-changed" "nophoto" #inst"2016-09-29T08:38:59.296-00:00"]}

I think somewhere in the datomic Posh code we need to deref the underlying ratom/reaction to kick off any registered listeners. No idea where to look though.

alexandergunnarson commented 7 years ago

That makes sense that the watches wouldn't be called until the reactions are derefed, at least in the case of a reaction. For a reaction, the cases where the watches are called are on reset!, swap!, deref, and the reaction's internal run function/method (and of course, anything that calls any of those functions). Even then, watches are only notified on certain conditions. I'll have to play around with it in a bit.

we need to deref the underlying ratom/reaction to kick off any registered listeners

This could be true, but if it were true for CLJ Datomic, it should be true for CLJ DataScript, right (which it's not)? In the dcfg, both use the :ratom and :make-reaction implementations provided by posh.lib.ratom. Maybe DataScript does something special? I'd have to look at it in a few hours.

alexandergunnarson commented 7 years ago

Wait, so for your use case, you want to get notified every time something changes in the reactive query, right? Why don't you do something like this? :

(def notified-times (atom 0))
(def query (db/q ...))
(run! @query
      (swap! notified-times inc))
(transact! ...)
(is (= @notified-times 2))

The reaction will re-run every time the query data changes.

Alternatively, if you want to have the notification run only on change (no initial run), use the following:

(make-reaction
   (fn [] @query
          (swap! notified-times inc)) 
   :auto-run true)
(transact! ...)
(is (= @notified-times 1))
(transact! ...)
(is (= @notified-times 2))

UPDATE : it works in DataScript but not Datomic, probably due to that race condition you were talking about.

seantempesta commented 7 years ago

Yeah. I want to have the client send the queries they want to execute the server, execute them on the server and send the changes down to the client whenever the results change. I just wrote this test for ratoms:

(deftest ratom-watch-test
  (try (let [ratom (r/atom nil)
             _ (is (= @ratom nil))
             add-watch (add-watch ratom :watcher (fn [key atom old-state new-state]
                                                   (throw (Exception. "change-detected"))))
             testing-watch (reset! ratom "test")
             _ (is false)]) ;; should not get to this point as the exception should be thrown
       (catch Exception e
         (println "Exception caught.  Test successful"))))

And it works fine. Isn't Posh using an r/atom to store the results of the query? Does a deref of a Posh query cause the query to be run again or does it just report the results stored inside the ratom?

alexandergunnarson commented 7 years ago

@seantempesta :

Isn't Posh using an r/atom to store the results of the query?

I haven't looked extensively at all the internals of posh. But it seems that yes.

Does a deref of a Posh query cause the query to be run again or does it just report the results stored inside the ratom?

This ties into the first question. If Posh doesn't do too much additional magic and acts just like a normal reaction, then the query will not be run on every deref. It will only recalculate when needed.

alexandergunnarson commented 7 years ago

@mpdairy @seantempesta @metasoarous :

The Datomic tests (minimal as of yet, but even so) officially work! Using what @seantempesta said as a very helpful starting place, I was able to figure out that the Posh listeners needed to be run/notified before transact! returned. Once they did, I got consistent results.

Another thing that you might notice is that the Clojure implementation of reactions/ratoms is now thread-safe (using clojure.core/atoms). It's theoretically possible that unsynchronized-mutable (a holdover from single-threaded CLJS) would have been fine, but I wanted to be safer rather than sorrier. All in all the thread-safe version is 2.5x slower than the single-threaded based on the perf test contained in posh.lib.ratom-test/ratom-perf, but that's to be expected. Unsynchronized mutability / atomicity can be toggled in the code very easily via the macros in ratom.cljc so if we need that then it's available.

I'll look over all your comments (shout out to @metasoarous , who I haven't yet responded to!) and such to get a feel for next steps and I'll tackle those this week.

seantempesta commented 7 years ago

Very cool! Great work @alexandergunnarson. I'm still not able to get my query subscriptions to trigger on transact! (still trying via add-watch and run!) until they are deref'ed. Is it working for you? Can you take a look at this test and tell me what I'm doing wrong?

(deftest query-watch-test
  (with-conn conn*
             (let [poshed (db/posh! conn*)                  ; This performs a `with-meta` so the result is needed
                   conn (-> poshed :conns :conn0)           ; Has the necessary meta ; TODO simplify this
                   _ (is (instance? PoshableConnection conn))]
               (try (let [txn-report (db/transact! conn (install-partition default-partition))
                          txn-report (transact-schemas! conn
                                                        [{:db/ident       :test/attr
                                                          :db/valueType   :db.type/string
                                                          :db/cardinality :db.cardinality/one}])
                          sub (db/q [:find '?e
                                     :where ['?e :test/attr]]
                                    conn)
                          _ (is (= @sub #{}))
                          add-watch (add-watch sub :watcher (fn [key atom old-state new-state]
                                                              (throw (Exception. "change-detected"))))
                          txn-report (db/transact! conn
                                                   [{:db/id     (tempid)
                                                     :test/attr "Abcde"}])
                          _ (is (= "db/transact! should trigger the watch which should throw an exception" false))])
                    (catch Exception e
                      (println "Exception caught.  Test successful"))
                    (finally (db/stop conn))))))            ; TODO `unposh!`
alexandergunnarson commented 7 years ago

Thanks @seantempesta !

So I just created a function called add-eager-watch which lets you do what you'd like. If you do (posh.lib.ratom/add-eager-watch sub :watcher ...) instead of (add-watch sub :watcher ...), your test will work. The normal add-watch will only run lazily on deref or reset.

seantempesta commented 7 years ago

Hey @alexandergunnarson. The eager stuff isn't working for me. In fact, if you split out just that part of the test it fails.

(defn just-eager-test [conn dcfg]
  (let [sub-no-deref ((:q dcfg) [:find '?e :where ['?e :test/attr]] conn)
        notified-no-deref (atom 0)
        _ (r/add-eager-watch sub-no-deref :k-no-deref (fn [_ _ _ _] (swap! notified-no-deref inc)))]
    (testing "Eager watch test"
      ((:transact! dcfg) conn
        [{:db/id     ((:tempid dcfg))
          :test/attr "Abcde"}])
      (is (= @notified-no-deref 1)))))
lein test :only posh.clj.datomic-test/just-eager-test

FAIL in (just-eager-test) (common_tests.clj:55)
Eager watch test
expected: (= (clojure.core/deref notified-no-deref) 1)
  actual: (not (= 0 1))

lein test posh.lib.ratom-test

Ran 18 tests containing 606 assertions.
1 failures, 0 errors.
Tests failed.

I'm not sure why it isn't working, but it's also matching my real world use. Until I deref the reaction I'm not getting any watches to run.

alexandergunnarson commented 7 years ago

Well that's upsetting... I suppose the sub and sub-no-deref have weird interactions which were apparently masked when I wrote the test.

This:

(with-setup
  [{:db/ident       :test/attr
    :db/valueType   :db.type/string
    :db/cardinality :db.cardinality/one}]
  (fn [conn]
    (let [sub-no-deref      (db/q [:find '?e :where ['?e :test/attr]] conn)
          notified-no-deref (atom 0)
          _ (r/add-eager-watch sub-no-deref :k-no-deref (fn [_ _ _ _] (swap! notified-no-deref inc)))]
      (println @notified-no-deref)
      @sub-no-deref 
      (println @notified-no-deref)
      (db/transact! conn
        [{:db/id     (tempid)
          :test/attr "Abcde"}])
      (println @notified-no-deref)
      @sub-no-deref
      (println @notified-no-deref)
      (db/transact! conn
        [{:db/id     (tempid)
          :test/attr "Abcde"}])
      (println @notified-no-deref)
      @sub-no-deref
      (println @notified-no-deref))))

prints 0 0 0 1 1 2 when it should be 0 0 1 1 2 2. Sorry about that! — strangely, in the tests, I'm derefing only the other sub, but since they perform the same query, they interact, which was unexpected. That's extremely strange... I'll have to fix that behavior soon.

seantempesta commented 7 years ago

Yeah, I figured there was some interplay between the code. Are we missing something obvious here? When I used reagent ratoms in frontend code I always had to deref them at least once (I assumed that somewhere this registered a watch). Is the run! code different from the approach you took with the add-eager-watch?

seantempesta commented 7 years ago

Hey Alex. Any updates on this? I've hacked together a manual pattern matching solution and while it works it's pretty ugly.

alexandergunnarson commented 7 years ago

Hey @seantempesta ! Sorry, just got through a long semester and I'm just getting back to working on this. I should push some updates over the next few weeks.

seantempesta commented 7 years ago

Sweet. No worries. I appreciate everything you've done. The manual pattern matching solution I've got is actually working pretty well, so there's no rush.