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

targetKey for hasMany and hasOne #4258

Closed
rm-richard opened this Issue Aug 4, 2015 · 35 comments

Comments

@rm-richard

rm-richard commented Aug 4, 2015

I have two legacy tables in a 1:m association:
Property(id, statistical_id, name, ...), VisitorStats(id, statistical_id, total, ...)

The following works:

VisitorStats.belongsTo(Property, { foreignKey: 'statistical_id', targetKey: 'statistical_id'} );

Then, in the controller, I can query for VisitorStats and eager-load the Property for it.

However, I want to query the Property table and include the VisitorStats model. For this, I would have to do a hasMany association, like this:

Property.hasMany(VisitorStats, { foreignKey: 'statistical_id' });

hasMany's doesn't support the targetKey parameter option, and automatically joins VisitorStats.statistical_id on Property.id when I query. I expected the targetKey property to work like at a belongsTo association.

Is it possible to change the target key using hasMany? If not, how should I represent this 1:m association?

@janmeier janmeier added the feature label Aug 5, 2015

@janmeier

This comment has been minimized.

Member

janmeier commented Aug 5, 2015

targetKey is not yet supported for hasMany

@janmeier janmeier changed the title from hasMany association always joins on primary key to targetKey for hasMany and hasOne Aug 6, 2015

@treejanitor

This comment has been minimized.

treejanitor commented Aug 29, 2015

This seems like a very useful modeling feature, especially for legacy databases.

@sijmenvos

This comment has been minimized.

sijmenvos commented Aug 31, 2015

I agree +1

@treejanitor

This comment has been minimized.

treejanitor commented Aug 31, 2015

I ended up having to use a raw SQL query, otherwise I would have had to invert the true object model to make the query work. So, it's possible to get the job done, but my premise is that an ORM ideally models logically the object tree as it should exist.

@PyroSA

This comment has been minimized.

PyroSA commented Sep 7, 2015

As per #4269, it is not working for HasOne/BelongsTo relationships either.
The ForeignKey works, but it still uses .id for the include statement.

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Sep 7, 2015

@PyroSA BelongsTo is the only one it works for, are you saying it no longer works, or are you saying it doesn't work from the HasOne side (which it never has)?

@PyroSA

This comment has been minimized.

PyroSA commented Sep 7, 2015

It does not work for the BelongsTo in our case either.

Not my project, but off the top of my head it would look like:

Model: Ta:
columns[id, guid, someStuff]
associate: Ta.hasOne Tb, {foreignKey: 'guid'}
Model: Tb:
columns[id, guid, otherStuff]
associate: Tb.belongsTo Ta, {foreignKey: 'guid', targetKey: 'guid'}

Not sure of the order (Ta/Tb) but:
Tb.findAll include: [Ta]
then tries to join on Ta.id == Tb.guid instead of Ta.guid == Tb.guid

I can confirm the exact sequence if it's unclear.
guids are stored as strings in this case.

@PyroSA

This comment has been minimized.

PyroSA commented Sep 7, 2015

@mickhansen - Tried to clarify above

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Sep 7, 2015

@PyroSA We have a test for that feature, so it should work - Please open a seperate issue with a reproduceable case since this issue deals with expanding the feature.

@PyroSA

This comment has been minimized.

PyroSA commented Sep 7, 2015

@mickhansen - I brought it up because #4269 was closed and referred to this feature. If it's considered a separate issue, can't that issue be re-opened?

If not I can create a new issue.

@guigrpa

This comment has been minimized.

guigrpa commented Sep 8, 2015

I have opened a new issue, as @mickhansen suggested: #4455

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Sep 8, 2015

@PyroSA that issue is about HasOne, not Belongsto :)

@colegleason

This comment has been minimized.

colegleason commented Sep 16, 2015

Hey, I also have legacy tables and would love this feature. Is there any good workaround in the meantime?

@janmeier

This comment has been minimized.

Member

janmeier commented Sep 16, 2015

@colegleason No workaround unfortunately

@rm-richard

This comment has been minimized.

rm-richard commented Sep 16, 2015

The cleanest solution I found was to create a dummy table, holding the commonly referenced IDs. Following up on the original example:

Property(id, statistical_id, name, ...), VisitorStats(id, statistical_id, total, ...)
Add the following table: Stats(statistical_id)

Set the associations:

Property.belongsTo(Stats, { foreignKey: 'statistical_id', targetKey: 'statistical_id'} );
Stats.hasMany(Property, { foreignKey: 'statistical_id'} );
Stats.hasMany(VisitorStats, { foreignKey: 'statistical_id'} );

After this, you can simply include either model in a query.
If you think about it, the legacy database model is flawed, since the 'statistical_id' points to a common, missing entity from both tables, they were never meant to reference each other. What I ended up doing was creating the table as I described, inserting all the 'statistical_id'-s into the new table and writing triggers to keep it updated with the new values from the legacy app.

@renewooller

This comment has been minimized.

renewooller commented Feb 15, 2016

Is this issue on the roadmap, or is it 'does not work as intended' ?

I think it would be useful, and would also make the package more consistent.

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Feb 16, 2016

@renewooller It's an open feature request, however not specifically on any roadmap.

@moxious

This comment has been minimized.

moxious commented Mar 2, 2016

I hate to "me too" on this issue/feature, but uh....me too? Doing some research and looking back through this thread, it appears a similar issue was opened and fixed on BelongsTo (#4455) - the actual commit looks dead simple. Is this a much more complex situation for hasMany or could it be done similarly and then a pull request created?

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Mar 2, 2016

@moxious It's not overly complicated, just takes some work making sure most cases are covered.

@arenddeboer

This comment has been minimized.

arenddeboer commented Mar 8, 2016

I would vote this up if there was a button, for now +1.
Currently stuck looking for a solution to map some legacy federated mysql tables into the sequelize mix.

matthewhardern pushed a commit to matthewhardern/sequelize that referenced this issue Apr 29, 2016

stellasoft-will stellasoft-will
Fixes hasMany issue in sequelize#4258
added a source key field to allow the source table to have a custom
foreign key that is not primary, defaults to the primary if not given
@matthewhardern

This comment has been minimized.

matthewhardern commented Apr 29, 2016

I have a fix for this, but not sure whether i have covered all bases. I could submit a pull request, but probs best to check out fix first

@janmeier

This comment has been minimized.

Member

janmeier commented Apr 29, 2016

@matthewhardern Go ahead, its easiest for us to review in PR form ;)

@matthewhardern

This comment has been minimized.

matthewhardern commented Apr 29, 2016

have submitted, but its got issues that will need solving by the looks of it

@truginis

This comment has been minimized.

truginis commented Jun 9, 2016

is this part of sequelize now?

@Paxa

This comment has been minimized.

Paxa commented Sep 30, 2016

@truginis Still pending #5823

@andrewholsted

This comment has been minimized.

andrewholsted commented Oct 28, 2016

I came across this today trying to refactor our codebase to exclusively use sequalize. It's a deal breaker for us that I can't declare a targetKey for a hasOne relation. As @treejanitor mentioned, I would have to invert our relations to make it work as stands. I took a look at #5823 and it doesn't appear to include hasOne but I couldn't follow it very well.

@felixfbecker

This comment has been minimized.

Contributor

felixfbecker commented Oct 28, 2016

@andrewholsted That PR was approved but it doesn't have any tests and has been stale for a while. All you need to do is pick up his work, rebase and add some tests :)

@justinpincar

This comment has been minimized.

justinpincar commented Nov 28, 2016

Over a year later and still no fix? 😞

I worked around this by making a second query and manually joining json representations.

twolfson added a commit to twolfson/sequelize that referenced this issue Jan 16, 2017

twolfson added a commit to twolfson/sequelize that referenced this issue Jan 16, 2017

twolfson added a commit to twolfson/sequelize that referenced this issue Jan 16, 2017

@twolfson twolfson referenced this issue Jan 17, 2017

Closed

V3 Added sourceKey support for hasOne #7115

0 of 5 tasks complete

sushantdhiman added a commit that referenced this issue Jan 20, 2017

@LoneWolfPR

This comment has been minimized.

Contributor

LoneWolfPR commented Jan 23, 2017

Is there any chance of this getting addressed anytime soon. We have a project using Heroku Connect with Salesforce. Salesforce tables join using the 'sfid' field which is not an integer primary key. I really need to be able specify a 1 to many relationship with hasMany setting the targetKey to this field instead of the primary key. Just seems strange to me that this has been sitting out so long.

@janmeier

This comment has been minimized.

Member

janmeier commented Jan 23, 2017

@LoneWolfPR it's been sitting so long because no one has taken the time to finish the code and tests.

As Felix mentions, the code is actually there, some one just needs to pick it up and add some tests

@LoneWolfPR

This comment has been minimized.

Contributor

LoneWolfPR commented Jan 23, 2017

@janmeier Thanks, I guess I missed that. That said this PR seems to address all the issues. Is it possible to just get the conflict in the changelog fixed so this can get merged? #6652

janmeier added a commit that referenced this issue Jan 24, 2017

V3 Fix sourceKey for hasMany, issue: #4258 (#6652)
* Fixes hasMany issue in #4258

added a source key field to allow the source table to have a custom
foreign key that is not primary, defaults to the primary if not given

* fix mistake on belongs to

* More support for sourceKey in hasMnay

* Add docs for sourceKey in hasMnay

* Fix jslint error in has-many.test.js

* Fix failing tests

* Add unique index for sourceKey tests

@janmeier janmeier closed this Jan 24, 2017

mkaufmaner added a commit to mkaufmaner/sequelize that referenced this issue Apr 25, 2017

@joe-angell

This comment has been minimized.

joe-angell commented May 11, 2017

How about sourceKey for hasOne?

@mlyticsAdamGe

This comment has been minimized.

mlyticsAdamGe commented Feb 2, 2018

To describe the relationship
belongsTo --->use targetKey to set the foreigner key from
hasMany --->use sourceKey to set the foreigner key from

or it will bind to the primary key

@philipimperato

This comment has been minimized.

philipimperato commented Apr 3, 2018

Is Sequelize still being actively developed? There a bunch of issues that have are stale.

@eggerand13

This comment has been minimized.

eggerand13 commented Nov 28, 2018

Maybe this information should be added to the legacy database handling in the documentation (http://docs.sequelizejs.com/manual/advanced/legacy.html)

Do I understand correctly that a fix for v4 has been commited but not been merged? mkaufmaner@1879d39

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