lilactown/pyramid

Replacing an entity using `delete` and `add` throws exception if entity does not exist

wardle opened this issue · 2 comments

When there is already data in the database, this works:

(-> {}
    (pyr/add {:person/id 2 :person/name "Mark"})
    (pyr/delete [:person/id 2])
    (pyr/add {:person/id 2 :person/name "Will"}))

=> #:person{:id {2 #:person{:id 2, :name "Will"}}}

If one avoids the delete, there is also no issue:

(-> {}
    (pyr/add {:person/id 2 :person/name "Mark"})
    #_(pyr/delete [:person/id 2])
    (pyr/add {:person/id 2 :person/name "Will"}))

=> #:person{:id {2 #:person{:id 2, :name "Will"}}}

But if there is no initial data in the database at all for that entity:

(-> {}
    #_(pyr/add {:person/id 2 :person/name "Mark"})
    (pyr/delete [:person/id 2])
    (pyr/add {:person/id 2 :person/name "Will"}))

Execution error (NullPointerException) at pyramid.core/update-ref (core.cljc:85).
Cannot invoke "clojure.lang.Associative.assoc(Object, Object)" because "em" is null

Of course, it is fine if there is at least one other entity in the database:

(-> {}
    (pyr/add {:person/id 1 :person/name "Mark"})
    (pyr/delete [:person/id 2])
    (pyr/add {:person/id 2 :person/name "Will"}))

=> #:person{:id {1 #:person{:id 1, :name "Mark"}, 2 #:person{:id 2, :name "Will"}}}

Looking at the code, it looks as if this exception could never be thrown, as em in update-ref should never be nil, as the get falls back to a {}. The issue I think is that delete clears the value to nil, but the key exists, so the get returns nil and not an empty map.

One fix is I think to replace (get m k {}) with (or (get m k) {}).

I will create a fork and submit a pull request if you are think that would be acceptable?

The fundamental issue is that delete results in a nil value:

(-> {} 
    (p/delete [:person/id 1]))

=> #:person{:id nil}

But my inclination is that it is better to not touch the delete code so it handles this edge case, and instead fix update-ref to operate with a nil value. I'll submit the PR on this basis but you might prefer a different solution of course!