Skip to content

Support filtering across relations (apply joins / fix column name). Fixes #18 #21

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

Merged
merged 1 commit into from
Oct 8, 2015

Conversation

techniq
Copy link
Member

@techniq techniq commented Oct 8, 2015

No description provided.

@jmdobry
Copy link
Member

jmdobry commented Oct 8, 2015

I've got hand it to you, I'm impressed you waded through that gnarly relations code and refactored it. 👏

One thing, can you squash your commits into a single commit?

@techniq techniq force-pushed the filter-joins branch 3 times, most recently from 67b148a to 3f74c25 Compare October 8, 2015 11:42
@techniq
Copy link
Member Author

techniq commented Oct 8, 2015

Squashed. I wasn't sure how verbose you wanted the commit message (I left the individual commit messages in the squashed commit).

No worries on the code, I used it as a learning exercise 😀

This PR only supports simple filtering (basically a single "foreign property" can be specified). If we need to support more complex filtering, we probably need to look at creating aliases on the joined tables and keeping track of what we've joined against already.

I'm might try to knock out filtering within the same chain of relations. For example:

var comments = yield adapter.findAll(Comment, { 'user.name': 'John' , 'user.profile.email': '[email protected]'})

Right now js-data-sql creates a join from comment > user for the first filter, then does the same for comment > user > profile which errors with ER_NONUNIQ_TABLE: Not unique table/alias: 'user'.

I'll send this improvement as a separate PR when I get time 😀

…ixes js-data#18

Consolidate and simplify loading of "with" relations for both find and findall

Yield promises with generator instead of chaining promises when cleaning up tests

Simplify looking up table name from resource config

Remove unnecessary logging
@techniq
Copy link
Member Author

techniq commented Oct 8, 2015

Disregard the comment about filtering within the same chain of relations, I implemented that and amended this PR 😀

jmdobry added a commit that referenced this pull request Oct 8, 2015
Support filtering across relations (apply joins / fix column name). Fixes #18
@jmdobry jmdobry merged commit 7f15722 into js-data:master Oct 8, 2015
@jmdobry
Copy link
Member

jmdobry commented Oct 8, 2015

I'll cut a release tonight.

@techniq
Copy link
Member Author

techniq commented Oct 8, 2015

👍

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

Successfully merging this pull request may close these issues.

None yet

2 participants