Add basic json query support #304
Conversation
zbarbuto
requested review from
b-admike,
dhmlau,
jannyHou,
kjdelisle and
virkt25
as code owners
|
@slnode test please |
| @@ -321,6 +321,30 @@ function escapeLiteral(str) { | |||
| return escaped; | |||
| } | |||
|
|
|||
| /* | |||
There was a problem hiding this comment.
Can we add JSDocs here for the property param?
| return property.split('.').length > 1; | ||
| } | ||
|
|
||
| /* |
There was a problem hiding this comment.
JSDocs would be great here as well.
lib/postgresql.js
Outdated
| // Unknown property, ignore it | ||
| debug('Unknown property %s is skipped for model %s', key, model); | ||
| continue; | ||
| if (isNested(key)) { |
There was a problem hiding this comment.
would p be null if the property has nested json keys? In other words, could we move this condition outside of the p == null check? I'm worried that this is unreachable code, unless I'm missing something.
There was a problem hiding this comment.
p can be null if the property is not defined on the model. Model might have foo property but foo.bar is being queried so props[key] is null but isNested(key) is true.
On the flip side if we try to query baz (which doesn't exist on the model and isn't a nested query) it should fall through so nothing unreachable.
However you do make a point that p could be null if it is nested and the property doesn't exist on the model. Will fix that.
| db.automigrate(function(err) { | ||
| if (err) return done(err); | ||
| Customer.create([{ | ||
| address: { |
There was a problem hiding this comment.
Can we add test/s for 1 more level of nested keys (for checking the recursive calls in https://github.com/strongloop/loopback-connector-postgresql/pull/304/files#diff-ac54005f9bbf753aa45ceebc4a032628R341, or would that be redundant? I.e.
Customer.create([{
name: {
fullName: {
firstName: 'foo'
lastName: 'bar'
}
}
}], function (err, customers) {
...
There was a problem hiding this comment.
Definitely not redundant since it uncovered a bug 😅
Syntax for deeply nested needed to be foo->bar->>baz since ->> returns the value at the path as text but -> returns the actual object at the path.
https://www.postgresql.org/docs/9.5/static/functions-json.html
There was a problem hiding this comment.
Ah great! thank you for the link :-).
|
@slnode test please |
|
Unable to see failures due to IBM firewall - but tests were passing locally and seem to be failing in other pull requests so possibly unrelated. |
|
@slnode test please |
|
@slnode test please |
|
@zbarbuto It was failing for the following reason: although I don't see that module used here. I've kicked off another build. |
|
Correct, that wasn't added as part of this PR. Apparently there were some issues with the internal registry recently - probably related |
|
@zbarbuto all the tests are passing now 🎉. Please squash your commits into one since they're all for one logical change, and we should be able to land. |
zbarbuto
force-pushed
the
feature/query-json
branch
from
283de77 to
d912164
Compare
|
Squished |
|
Thanks @zbarbuto! |
Description
Add basic support for json querying (where and order-by only - no field limiting to keep it simple).
Related issues
Checklist
guide
Incidentally, I notice this repo doesn't support ES6 syntax online some of the other loopback repos now do 😞