destroyById crashes the app when model with the id does not exist

[loopbackio]

destroyById crashes the app when model with the id does not exist

[superkhau]

@sviox Can you provide a sample repo for us to clone and reproduce the issue on our end?

[superkhau]

@sviox Ping

[superkhau]

@Setogit Might be an SG issue too, so you may want to take a look.

[b-admike]

@sviox I got an error as well using the following steps:

1. create 2.x loopback application with notes template
2. create a loopback-connector-redis data-source and attach it to the Note model
3. populate the Note model
4. delete by Id (using explorer) for an id that didn't exist - I had 500 return code on explorer
5. delete by id which existed - 200 return code
6. delete by id which didn't exist and get:

| => node .
Web server listening at: http://0.0.0.0:3000
Browse your REST API at http://0.0.0.0:3000/explorer
/Users/badmike/redisapp/node_modules/redis/index.js:602
                throw err;
                ^

TypeError: Cannot read property 'id' of null
    at /Users/badmike/redisapp/node_modules/loopback-connector-redis/lib/redis.js:635:61
    at /Users/badmike/redisapp/node_modules/loopback-connector-redis/node_modules/async/lib/async.js:567:21
    at /Users/badmike/redisapp/node_modules/loopback-connector-redis/node_modules/async/lib/async.js:246:17
    at /Users/badmike/redisapp/node_modules/loopback-connector-redis/node_modules/async/lib/async.js:122:13
    at _each (/Users/badmike/redisapp/node_modules/loopback-connector-redis/node_modules/async/lib/async.js:46:13)
    at async.each (/Users/badmike/redisapp/node_modules/loopback-connector-redis/node_modules/async/lib/async.js:121:9)
    at _asyncMap (/Users/badmike/redisapp/node_modules/loopback-connector-redis/node_modules/async/lib/async.js:245:13)
    at Object.map (/Users/badmike/redisapp/node_modules/loopback-connector-redis/node_modules/async/lib/async.js:216:23)
    at _parallel (/Users/badmike/redisapp/node_modules/loopback-connector-redis/node_modules/async/lib/async.js:565:20)
    at Object.async.parallel (/Users/badmike/redisapp/node_modules/loopback-connector-redis/node_modules/async/lib/async.js:595:9)
    at /Users/badmike/redisapp/node_modules/loopback-connector-redis/lib/redis.js:637:13
    at /Users/badmike/redisapp/node_modules/loopback-connector-redis/lib/redis.js:613:9
    at /Users/badmike/redisapp/node_modules/loopback-connector-redis/lib/redis.js:111:38
    at /Users/badmike/redisapp/node_modules/loopback-connector-redis/lib/redis.js:97:9
    at try_callback (/Users/badmike/redisapp/node_modules/redis/index.js:592:9)
    at RedisClient.return_reply (/Users/badmike/redisapp/node_modules/redis/index.js:685:13)

I tried the same steps using a spanish locale to get strong-globalize involved but I get the same error.

version info:

___________________    | ~/espanolapp @ Biniams-MacBook-Pro (badmike)
| => npm -v
3.10.10
___________________    | ~/espanolapp @ Biniams-MacBook-Pro (badmike)
| => node -v
v4.6.0
___________________    | ~/espanolapp @ Biniams-MacBook-Pro (badmike)
| => slc -v
strongloop v6.0.3 (node v4.6.0)
├── [email protected] (3dd25ac)
├── [email protected] (a98f310)
├── [email protected] (9b29df0)
├── [email protected] (62e539b)
├── [email protected]
├── [email protected] (f8c2339)
├── [email protected] (28c261d)
├─┬ [email protected]
│ └── [email protected] (b51fc45)
├── [email protected] (6a0e2c0)
├── [email protected] (16841d6)
└── [email protected]

[sviox]

Thank you b-admike for providing an example. My app was a bit complicated to strip down so I would have needed to create basically an empty application to demonstrate it just like you did.

[b-admike]

You're welcome @sviox.

[gclyne]

@b-admike I have a fix, if you can give it a test please. I tested it myself, and it works.

[b-admike]

@gclyne I have tested your fix and while my app doesn't crash, it still returns 200 status code and an empty array for the ids which don't exist. If that is the expected behaviour, then we are golden.

EDIT: I have also tested the same steps with a mongoDB datasource and besides returning the count of rows deleted, the status code is still 200 for both existing and non-existing ids. Thus, IMO, the behaviour is correct.

[gclyne]

@b-admike Cool, so is it ok to go ahead and merge this fix or is there anyone else who should review this? @superkhau?

[b-admike]

@gclyne I think it is safe to merge. @superkhau can pitch in. @sviox can you also try the fix?

[superkhau]

@gclyne I responded in the PR, can you add some tests? Should be able to land/release right after that is done.

@b-admike Can you do the initial review in the PR and ping me to do a final review? PTAL at https://github.com/strongloop-internal/loopback-knowledge-base/blob/master/triaging-pull-requests.md

[superkhau]

cc @strongloop-community/loopback-managers We need a sprint 27 milestone for strongloop-community also.

[superkhau]

@b-admike Please fill in the assignees, estimates, etc yourself next time, same for the corresponding fix PR.

[b-admike]

Will do @superkhau and since the change is reviewed and merged, this issue can be closed as soon as @sviox also verifies it. We might also be able to close #44 since it seems like a related issue.

[superkhau]

@b-admike Can you own the above? Feel free to close this issue after verifying with @sviox and close #44 after referencing the PR I landed or this issue for posterity.

[b-admike]

@sviox I am closing this issue because I have verified the fix. However, if you see it again after using the master version of this repo (until next release), then we can investigate further. I have created a repro app here that you can use.