Upsert with tx-data in list format

[dwwoelfel]

It looks like upserts are only resolved for tx-data in the map format. Here's an example that demonstrates the problem:

(let [db (d/db-with (d/empty-db {:name  { :db/unique :db.unique/identity }})
                    [{:db/id 1 :name "Ivan"}])]
  (d/with db [[:db/add -1 :age 35]
              [:db/add -1 :name "Ivan"]]))

=> throws {:error :transact/unique, :attribute :name, :datom #datascript/Datom [2 :name Ivan 536870914 true]}

Added a test on my fork, which also fails in CI.

[tonsky]

Wow that is tricky. So -1 gets resolved to 2 first, and then it conflicts with upsert in a second clause.

This won’t work in current design, because in DS every transaction clause is executed one after another. Each intermediate DB state is materialized in between. So it’s not possible to “undo” first clause or change tempid binding. It allows us to have intermediate DBs passed into db.fn/call. If you add some data and then has db.fn/call in the same transaction, fn’ll see that data. Also, if you insert new record with new unique attribute, you can use lookup refs to it in the same transaction.

This is different from Datomic. Datomic first resolves all tempids and calls all transaction functions, passing the same db-before value to all of them. So later transaction clauses cannot see results of earlier transaction clauses, and sometimes you have to split your transaction to several clauses to get around that. But as a benefit you can re-decide tempid bindings.

This is more like a design issue here. I’m currently happy with DS design, as it has its benefits and more intuitive in my opinion. If this is a serious issue for you, please elaborate more. Right now it looks like it can be easily avoided.

[dwwoelfel]

I can't say for sure yet whether it's a serious issue, still working through some edge cases. Making the outcome order-dependent is going to cause other problems, though. Here's an example resolving tempids with refs where changing the order will result in an exception:

(deftest test-upsert
  (let [db (d/db-with (d/empty-db {:name  { :db/unique :db.unique/identity }
                                   :friend { :db/type :db.type/ref }})
                      [{:db/id 1 :name "Ivan"}
                       {:db/id 2 :name "Petr"}])
        touched (fn [tx e] (into {} (d/touch (d/entity (:db-after tx) e))))]

    ;; This test passes
    (testing "upsert with references"
      (let [tx (d/with db [{:db/id -2 :name "Petr"}
                           {:db/id -1 :name "Ivan" :friend -2}])]
        (is (= {:name "Ivan" :friend {:db/id 2}} (touched tx 1)))
        (is (= {:name "Petr"} (touched tx 2)))))

    ;; Changing the order causes it to fail with
    ;;  {:error :transact/upsert, :attribute :name, :entity {:db/id 3, :name Petr}, :datom #datascript/Datom [2 :name Petr 536870913 true]}
    (testing "upsert with references, reversed order of txes, fails"
      (let [tx (d/with db [{:db/id -1 :name "Ivan" :friend -2}
                           {:db/id -2 :name "Petr"}])]
        (is (= {:name "Ivan" :friend {:db/id 2}} (touched tx 1)))
        (is (= {:name "Petr"} (touched tx 2)))))))

[tonsky]

Well, output is order-dependent anyway (you can’t swap additions and retractions for example). In case of tempids, we can sort it out even in the current model, actually. Just need to add some backtracking. Don’t expect it soon though :)

[levand]

@tonsky Your explanation of clause ordering and tempid resolution makes sense. But do you plan to fix the issue that upserts always fail when in vector form, regardless of clause ordering?

Currently:

(d/with db [{:db/id -1, :id "foo"}]) ;; succeeds
(d/with db [[:db/add -1 :id "foo"]] ;; fails

Is that the intended behavior?

[tonsky]

@levand it does not fails, there’s plenty of :db/add -1 in the test base: https://github.com/tonsky/datascript/search?utf8=✓&q=%3Adb%2Fadd+-1

It only causes problems when -1 is used several times during same tx, and it is forced to be resolved to different values (e.g. first it allocates new id, then it is resolved via upsert attribute, etc).

Yes, I’m going to fix this, though cannot promise you exact timeline

[levand]

It does fail, if :id is :db.unique/identity and there already exists a datom with the same attribute (in other words, the 1-clause transaction is just re-asserting something already in the DB).

The map form does an upsert, the vector form throws a uniquness exception.

I'll see if I can get you a PR with a test case later today.

[tonsky]

Oh. Can you check with Datomic? It might be intentional.

On Thu, Aug 27, 2015 at 6:51 PM Luke VanderHart notifications@github.com
wrote:

It does fail, if :id is :db.unique/identity and there already exists a
datom with that ID.

The map form does an upsert, the vector form throws a uniquness exception.

I'll see if I can get you a PR with a test case later today.

—
Reply to this email directly or view it on GitHub
#76 (comment).

[levand]

Test case: #109

This works fine in Datomic.

[tonsky]

Thanks! You’re right, upserts are not resolved in vector form at all :( Will fix

[brandonbloom]

I know this issue has been closed, but I wanted to throw in my two cents regarding the design question here: Datomic's behavior makes a lot more sense to me: 1) order-independent id unification and 2) transaction before/after dbs rather than database function before/after dbs. As it is now, hashing order can cause random uniqueness failures if you try to batch add a few maps, where datomic will happily accept the transaction with last-written wins. If your data is internally consistent, such as the same entity twice in the result of a d/pull, then all the writes are the same.

[brandonbloom]

This test fails, but passes if you swap the order of keys in the map. I'll try to see if I can fix it.

(deftest test-ref-upsert
  (let [db (d/empty-db {:name {:db/unique :db.unique/identity}
                        :ref {:db/valueType :db.type/ref}})]
    (are [tx res] (= res (tdc/all-datoms (d/db-with db tx)))
      [(array-map :ref {:name "Ivan"} :name "Ivan")]
      #{[1 :name "Ivan"]
        [1 :ref 1]})))