Skip to content
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.

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

Conflicting upsert when ids are added out of order #285

Closed
Cortys opened this issue Jan 7, 2019 · 3 comments
Closed

Conflicting upsert when ids are added out of order #285

Cortys opened this issue Jan 7, 2019 · 3 comments

Comments

@Cortys
Copy link

@Cortys Cortys commented Jan 7, 2019

Upserts fail if multiple tempids are first allocated and then unified in a different order:

(d/db-with (d/empty-db {:name {:db/unique :db.unique/identity}})
           [[:db/add -1 :age 42]
            [:db/add -2 :likes "Pizza"]
            [:db/add -2 :name "Bob"]
            [:db/add -1 :name "Bob"]])
;; => Fails: Conflicting upsert: -1 resolves both to 3 and 2

If however unification happens in the same order as the initial allocation, the issue does not occur:

(d/db-with (d/empty-db {:name {:db/unique :db.unique/identity}})
           [[:db/add -1 :age 42]
            [:db/add -2 :likes "Pizza"]
            [:db/add -1 :name "Bob"]
            [:db/add -2 :name "Bob"]])
;; => Works.

Is this a bug or intentional (i. e. similar to #172)?

@tonsky

This comment has been minimized.

Copy link
Owner

@tonsky tonsky commented Jan 8, 2019

It’s strange that second way works. Are you sure it does what you want? Tempid resolution in DS is top-to-bottom, with no backtracing, so changing this behaviour (adding unification) would need a huge refactoring.

@Cortys

This comment has been minimized.

Copy link
Author

@Cortys Cortys commented Jan 8, 2019

Yes, it seems to work as expected irrespective of the number of unified tempids:

(deftest unification
  (let [n 100
        db (d/db-with (d/empty-db {:x {:db/cardinality :db.cardinality/many}
                                   :key {:db/unique :db.unique/identity}})
                      (concat ; Allocate n tempids:
                              (map #(vector :db/add (- %) :x %) (range n))
                              ; Unify them:
                              (map #(vector :db/add (- %) :key "foo") (range n))))]
    ; All n tempids are resolved to the same entity:
    (is (= (d/pull db '[*] [:key "foo"])
           {:db/id 0 
            :key "foo"
            :x (vec (range n))}))))

As far as I understand this works because retry-with-tempid is called after each addition of :id.
This effectively unifies one tempid -t with [:id "foo"] at a time. The old entity id allocated-id that -t was assigned via [:db/add -t :x t] is replaced with upserted-id, i.e. the id of [:id "foo"].

In total this performs O(n^2) operations but at least it works reliably. I'm however not quite sure why the unification order matters.

@tonsky

This comment has been minimized.

Copy link
Owner

@tonsky tonsky commented Jan 9, 2019

Right! I forgot about this. I’ll take a closer look into the issue soon

@tonsky tonsky closed this in f0490b0 Jan 26, 2019
tonsky added a commit that referenced this issue Jan 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.