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

Support for has many through / jointable model #877

Merged
merged 12 commits into from Sep 27, 2013

Conversation

3 participants
@janmeier
Member

janmeier commented Sep 3, 2013

This pull request adds support for specifying a model to use as the join table when doing double linked has-many associations, as described in #254

The join table is specified by adding a joinTableModel, which should be an instance of DAOFactory to the options when calling hasMany. The model only comes into effect after the second call (before that we don't know whether this is actually a double linked has many. Writing this code has led me to think that associations should be refactored for 2.0.0, so we can implicitly call hasManyDouble or something like that, instead of calling hasMany twice)

Some questions that I am not quite sure about regarding the code:

  • Right now, when adding an association, there are two posibilites for adding data to the join table model - one is to add a property to the object that is being associated, and the other is to pass an extra argument to the addAssociation call. Should we really support both ways, or only one?
  • When adding multiple associations with setAssociations you can again both set the join table attributes on the actual model that is being associated, or you can specify an extra argument with the values. Since we are associating multiple values, we need to be able to specify individual join table attributes for each, so here we need to support a property on each object. I chose to add an extra argument with "default values", so the user doesn't have to set the same attributes on each object - do you like this?
// FIXME - Should we do this? Normally join tables does not have an id, and it is not needed,
// but users might wonder what hapened if they defined the ID themselves
delete this.connectorDAO.rawAttributes.id

This comment has been minimized.

@janmeier

janmeier Sep 3, 2013

Member

I still haven't decided on this line. If the join table is created by sequelize it will not have an id, but as said we might not want to confuse the user by removing it. Or should we?

This comment has been minimized.

@durango

durango Sep 3, 2013

Member

It should be declarable in someway, or at the very least... sort through some sort of association primary key

This comment has been minimized.

@janmeier

janmeier Sep 3, 2013

Member

My though was that if id is defined, then it was probably put there by sequelize, so we can remove it. But if custom primary keys were set on the table the user deliberately wanted them, so we should keep them

This comment has been minimized.

@janmeier

janmeier Sep 3, 2013

Member

Agree, this should probably be configurable. But the again, that is a very small config, so maybe we should just keep it. Not really a problem in having an extra integer field, even though its redundant

@durango

This comment has been minimized.

Member

durango commented Sep 6, 2013

@janmeier whenever you get the chance hope on IRC and we'll discuss this PR :)

@janmeier janmeier referenced this pull request Sep 10, 2013

Merged

docs for join table model #72

@janmeier

This comment has been minimized.

Member

janmeier commented Sep 20, 2013

Rebased @sdepold

@sdepold sdepold merged commit 7b07cd0 into sequelize:master Sep 27, 2013

1 check passed

default The Travis CI build passed
Details

@janmeier janmeier deleted the janmeier:jointablemodel branch Sep 27, 2013

@sdepold

This comment has been minimized.

Member

sdepold commented Sep 27, 2013

great work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment