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

Support embedded query in memory connector. Fix memory connector bug #697

Merged
merged 1 commit into from
Aug 18, 2015
Merged

Support embedded query in memory connector. Fix memory connector bug #697

merged 1 commit into from
Aug 18, 2015

Conversation

mazesoul
Copy link
Contributor

Adds support for the ability to query embedsMany models from the parent.

Fix a memory connector bug that could occur when having an "or" or "and"
clause combined with another property. In that case, the and would revert
true for 'Paul McCartney'

{name:'John Lennon',and: [{role:'lead'}, {vip:true}]}}

Adds support for the ability to query embedsMany models from the parent.

Fix a memory connector bug that could occur when having an "or" or "and"
clause combined with another property. In that case, the and would revert
true for 'Paul McCartney'

```
{name:'John Lennon',and: [{role:'lead'}, {vip:true}]}}
```
@mazesoul
Copy link
Contributor Author

@altsang Are you in a position to give me an ETA for the review/merge. Also, is this something that you can accept?

@raymondfeng
Copy link
Contributor

@mazesoul Thank you for the patch! Contributions are always welcome!

  1. Before your PR, the and and or operator cannot be combined with regular clauses. Your case needs to be written as:
{and: [{name:'John Lennon'}, {role:'lead'}, {vip:true}]}

If we allow and/or to be used with regular clauses, we need to make sure other connectors support it too.

  1. We already support nested properties for the memory connector even before this PR, see https://github.com/strongloop/loopback-datasource-juggler/blob/master/test/memory.test.js#L397-L428. What else does the PR add?

@mazesoul
Copy link
Contributor Author

@raymondfeng The memory connector didn't support nested properties over arrays. Like this https://github.com/WyzeLink/loopback-datasource-juggler/blob/feature/support-embedded-query-in-memory/test/memory.test.js#L431-L439. Where friends is an array of object. This is important to be able to test embedsMany relations

Concerning the support for the and/or clauses, from what I can gather the SQL connectors all default to loopback-connector's _buildWhere that does concatenate all the keys together https://github.com/strongloop/loopback-connector/blob/master/lib/sql.js#L827.

Moreover, in the memory connector's implementation, there was a bug even if it doesn't support both. If the and/or clause appeared before another key, it could be reverted because of the forEach iteration. An and that was true, could have been made false if the key was declared after. The pass in the closure, although only setting to false, could in certain circumstances cause confusion by failing the filtering on the secondary case.

As for mongo, its where build is additive: https://github.com/strongloop/loopback-connector-mongodb/blob/master/lib/mongodb.js#L553

@mazesoul
Copy link
Contributor Author

And finally, a bug which I forgot to mention in the PR's comment: supposing there are two objects, only one of which having a value for a string key, the regex match would throw because there was no verification: https://github.com/strongloop/loopback-datasource-juggler/pull/697/files#diff-98aa884e786e8c02d53648b6f0afaa82R481

It would be trying undefined.match

raymondfeng added a commit that referenced this pull request Aug 18, 2015
…in-memory

Support embedded query in memory connector. Fix memory connector bug
@raymondfeng raymondfeng merged commit 75751eb into loopbackio:master Aug 18, 2015
@mazesoul
Copy link
Contributor Author

Thanks :)

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

Successfully merging this pull request may close these issues.

3 participants