-
Notifications
You must be signed in to change notification settings - Fork 107
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
add associateJoin function #157
Conversation
this helps avoid N+1 queries
It looks like the lower bound on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Please include a Changelog
entry with a minor version bump (i think it'd be 3.1.2
) and modify the Cabal file accordingly. I'll try to get this released soon.
-- 'where_' (foo '^.' FooParentId '==.' 'val' parentId) | ||
-- 'pure' (foo, bar) | ||
-- @ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a @since
annotation with the relevant version.
-- getFoosAndNestedBarsFromParent :: ParentId -> (Map (Key Foo) (Foo, [Entity Bar])) | ||
-- getFoosAndNestedBarsFromParent parentId = 'fmap' associateJoin $ 'select' $ | ||
-- 'from' $ \\(foo `'LeftOuterJoin`` bar) -> do | ||
-- 'on' (bar '^.' BarFooId '==.' foo '^.' FooId) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bar
access here should use ?.
because a LeftOuterJoin
implies nullability on the right hand side. That this isn't enforced is a bug (see #150). The type of this would then be Map (Key Foo) (Foo, [Maybe (Entity Bar)])
.
Map.insertWith | ||
(\(oneOld, manyOld) (_, manyNew) -> (oneOld, manyNew ++ manyOld )) | ||
(entityKey one) | ||
(entityVal one, [many]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd define this directly in Database.Esqueleto
and not one of the internal modules. It's easy to miss the reexports necessary.
- moves associateJoin to Database.Esqueleto - relaxes bounds on containers dep -
======== | ||
|
||
- @tippenein | ||
- [#149](https://github.com/bitemyapp/esqueleto/pull/157): Added `associateJoin` query helpers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙌
This function might want to live in esqueleto for avoiding N+1 queries? Just a rough example here along with (probably insufficient) code comment. It also adds a
containers
dep which might be undesirable.