quoll / asami

A flexible graph store, written in Clojure
Eclipse Public License 1.0
329 stars 10 forks source link

:id lookup includes new entities in current tx #2

Closed holyjak closed 2 years ago

holyjak commented 2 years ago

Allow ID lookup (when creating or referring to new entities with :id) to include entities being created in the same transaction. It solves the following problem:

I struggle to understand what is the correct way to 1. create a new entity and 2. link an existing entity to it. For the link I cannot use the new entity's :id as a lookup ref because it is not created yet and I do not have any other ID (since I cannot use :db/id -1 together with specifying a custom :id). It is demonstrated by the following failing test:

(deftest existing-add-ref-to-new
  @(d/transact *conn* {:tx-data [{:id "existing"}]})
  @(d/transact *conn* {:tx-data [{:id "new"}
                                 [:db/add [:id "existing"] :ref [:id "new"]]]})
  ; BROKEN: Resolving the lookup ref does not work, it is stored as-is - likely b/c the entity cannot be looked up
  ;         since we are only just creating it
  (is (= {:id "existing",
          :ref {:id "new"}}
         (d/entity (d/db *conn*) "existing"))))
; => Expected {... :ref {:id "new"}}, Actual {... :ref [:id "new"]}

NOTE

The 2nd commit 42aa2e01865094d590707583bef3e960b0cb6a51 refactors tests to make it possible to re-run them in the REPL without them failing due to left-over state from a previous run. It can/should be reviewed separately from the rest.

holyjak commented 2 years ago

Hi! I would very much welcome your critique of the patch, especially w.r.t.:

  1. What other test cases could I write?
  2. How can I simplify / improve / make more readable the code?
  3. Is there perhaps a different, better way to do this?

Also:

  1. Notice I have some question in the code where I was unsure so please react and I will delete the comments
  2. I guess the Changelog update needs to be improved when we agree on the patch

PS: I would be happy to clean up and merge the commits and improve commit messages however you want. Or just squash them together.

holyjak commented 2 years ago

~I see 2 tests in api-tests are failing (I had to restart my repl to get the error). I will look into that. It seems I have somehow managed to break the resolution of [:db/id -1] references.~

holyjak commented 2 years ago

Hi Paula! I have fixed everything you mentioned as best I could. Let me know if there is anything I should change. Thank you!!!