Allow eager loading associations w/ a renamed fk#1
Conversation
Because Korma's :transforms/:prepares features allow users to rename fields, we can't necessarily assume that the foreign key passed to the database query is the same as the one in the result set. This removes that assumption from the eager-loading stitcher by transforming the foreign key in the same way as records are transformed.
src/cumin/core.clj
Outdated
There was a problem hiding this comment.
Nice catch! I'm worried a transformation may expect some data to be present and throw an exception when mimicking a record with {fk nil}.
How do you feel about removing the transforms, stitching the records together, and then applying the transformation to the stitched result set?
(defn- ^:no-doc apply-transforms
[results f k]
(mapv (fn [row]
(if (contains? row k)
(update-in row [k] #(mapv f %))
row))
results))
(defn ^:no-doc include* [ent mapping body-fn rows]
(let [[pk fk] (first (dissoc mapping :as))
as-k (get mapping :as (keyword (:table ent)))
pks (distinct (remove nil? (map pk rows)))
sub-q (-> ent (dissoc :transforms) (select*) (body-fn))
results (when (seq pks)
(-> sub-q
(cond-> (not-any? #{:* :korma.core/* fk} (:fields sub-q))
(update-in [:fields] conj fk))
(where {fk [in pks]})
(select)))]
(if (seq results)
(-> rows
(stitch results as-k [pk fk])
(apply-transforms (apply comp (:transforms ent)) as-k))
rows)))There was a problem hiding this comment.
Go for it - this works totally great for me. The {fk nil} was totally a hack, and I like this better.
There was a problem hiding this comment.
If you make the changes, I'll happily merge this PR - the tests are great. Thanks for spotting this!
There was a problem hiding this comment.
Cool deal, will do, thanks!
This way we don't assume all transforms are able to handle a nil value,
missing keys, etc. the way that transforming `{fk nil}` would require.
h/t @kgann for the PR review & modified code.
|
Updated. By the way, thanks for this library - I'm super-impressed at how concisely you've added these features, and I'm looking forward to rolling it out for some huge performance wins. |
|
Awesome! I'm glad you're getting some use out of it. Keep the improvements comin'! |
Allow eager loading associations w/ a renamed fk
|
I released |
2 participants
Because Korma's :transforms/:prepares features allow users to rename fields, we can't necessarily assume that the foreign key passed to the database query is the same as the one in the result set. This removes that assumption from the eager-loading (
include) stitching, by transforming the foreign key in the same way as records are transformed.I do realize that this test case's particular kebab/snake-case dance introduces a lot of complexity for what ends up being a pretty low benefit, but it happens, and takes some work to rip out. So as a general tool on top of Korma, which allows this kind of thing, I think it's great if Cumin can handle this use case.