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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for issue #629 #1716

merged 1 commit into from Dec 29, 2017


Copy link

@syangdev syangdev commented Dec 28, 2017

Can't attach belongsToMany relation to models fetched with fetchAll

PR name

  • Related Issues: 629
  • Previous PRs: N/A


Can't attach "belongsToMany" relation to models fetched with fetchAll. Will run to errors when the parent model access the collections of the child models and tries to detach.


A many to many with a pivot table is pretty common. This bug makes bookshelf useless in this scenario.

Proposed solution

The current workaround documented in #629 is to load the child models again which is not very database friendly. The simple solution is to have the parent model recreate the the relation related data when pairing the child models with the parent models (note that before the child model fetch, a interim relation object was created and so the parent model couldn't be associated downstream of this process).

Fixes #629.

Current PR Issues

Not sure.

Alternatives considered

This seems like it would also affect many to many relationships.

Can't attach belongsToMany relation to models fetched with fetchAll
Copy link

This just needs tests. I can add tests myself if you don't have the time for that.

Copy link

@ricardograca Yea, I don't have time. This is also my first time collaborating on open source project too, so forgive me. I just wanted to show a simple solution. Please note that this bug probably affects a many to many relationship too.

Copy link

@ricardograca ricardograca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll be writing some tests for this after it's merged.

@ricardograca ricardograca changed the title Fix for issue: Fix for issue #629 Dec 29, 2017
@ricardograca ricardograca merged commit c37a28b into bookshelf:master Dec 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet

Successfully merging this pull request may close these issues.

None yet

2 participants