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

Many-To-Many Through patch #1227

Closed

Conversation

particlebanana
Copy link
Contributor

This should resolve issue #1133 discovered by @atiertant. It uses the patched version of Waterline-Schema from balderdashy/waterline-schema#32 to use the throughTable flag.

If everything looks good we will need to bump the minimum version of waterline-schema once this is merged.

This is an alternative for #1134 and includes the unit test from that PR.

EDIT: to be clear this isn't adding any new through support. Only fixing an issue with the support that already existed.

@atiertant
Copy link

@particlebanana flagging throughTable true or false is'nt suffisant to correct through associations.
the logic must be different than junctionTable because :
junctionTable is always a junctionTable but throughTable shoud act like it on some relation only.
junctionTable contain only two relations so populate skip parent relation and chose the other to find the child one but a throughTable contain more associations...so this can't be done like this.

i propose to add this in schema to correct the problem :

throughTable : [
   "driver.cars": "car",
   "car.drivers": "driver"
]

here we save the other part of throughTable in a relation dot notation key.
like this we can check if we must consider it as a throughTable or simply as a collection depending the context and know the correct association of the throughTable use in populate.

@particlebanana
Copy link
Contributor Author

@atiertant this fixes the issue shown in the test. It only loads the throughTable once with the original definition which allows you to use the Drive table as a normal collection or as a junctionTable.

I'm not sure I understand the issue. Using via should map up everything correctly for other associations on the Drive collection. Could you put together a gist showing something that doesn't currently work (after this patch and the schema patch)?

If you are saying you want three levels of results for many-to-many through that is a separate feature. I think most of that logic would live in waterline-cursor but would take some work. That is separate from this issue.

So to clear up this allows you to have a collection that can work as both a normal collection and a junction table.

To have a collection that returns the values on the throughTable when populating either side (User, Car) is a different issue and involves work with the cursor and elsewhere.

@atiertant
Copy link

@particlebanana i was not talking about a new feature but bug fix.(of course using dot notation open deep features but this is not the question here)
i will pr a new set of tests to show you what i was talking about next week.

@atiertant
Copy link

@particlebanana
Copy link
Contributor Author

@atiertant thanks, looking into it.

@particlebanana
Copy link
Contributor Author

@atiertant Ah I see now why this is needed. It was dumbly just looking for the other key in the junction table. Ok I will close mine and go with yours.

The .add() and .remove() test will fail in your test case but I have a fix for that I will PR after these are merged. Thanks for all your help and sorry it took so long for me to wrap my head around.

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

Successfully merging this pull request may close these issues.

2 participants