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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix upserts containing tempids in tuples #379

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ash14
Copy link
Contributor

@ash14 ash14 commented Dec 18, 2020

PR to address #378. Not sure how dirty the implementation is for this ... but tests pass so 馃し

Cheers.

@tonsky
Copy link
Owner

tonsky commented Dec 18, 2020

Thank you for the bug report and the PR. This is an interesting problem :)

I feel that tempid resolution should not happen in resolve-upserts, but somewhere earlier, around

(and (ref? db a) (tempid? v))
(if-some [resolved (get tempids v)]
(let [report' (update report ::value-tempids assoc resolved v)]
(recur report' (cons [op e a resolved] entities)))
(let [resolved (next-eid db)
report' (-> report
(allocate-eid v resolved)
(update ::value-tempids assoc resolved v))]
(recur report' es)))

Basically, same code should run, but for all tuple values, not just for a single v.

Also, checking for being a tuple is better not by coll?, but by going to schema

(defn #?@(:clj [^Boolean tuple?]
:cljs [^boolean tuple?]) [db attr]
(is-attr? db attr :db.type/tuple))

@ash14 ash14 marked this pull request as draft December 21, 2020 09:33
@ash14
Copy link
Contributor Author

ash14 commented Dec 21, 2020

I've updated the test case - the entity to upsert now comes first which breaks the approach I was taking with this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants