Loopback: Token middleware user literal replacement fails when user ID is 0

[shaharz]

Happened to run into this edge case. The culprit is this if statement, obvious in hindsight: https://github.com/strongloop/loopback/blob/6964914bab64496c789c434fec39d2914231ee23/server/middleware/token.js#L24

Happy to send a PR if you need a hand.

P.S. I noticed another issue with ID == 0 where an include filter fails (doesn't show relations) but haven't had time to look into it yet.

[davidcheung]

@shaharz I believe this is not a one-off scenario but a general loopback behavior. In order to keep behaviors consistent, I feel the support for id==0 might be quite a large scope, if the decision is to support Boolean(id) == false I think it will need to be done across the board for all ID instances

/cc @strongloop/loopback-dev any thoughts?

For example: model, access-token

[jannyHou]

I agree with

support for id==0 might be quite a large scope

And btw, now when we open Explorer and try POST/ MyModels, the default id value in model instance is id = 0, this kinds of hints users that we support id = 0, which is not expected.

[davidcheung]

Upon discussion with @raymondfeng, he agrees that we should fix it, would you mind submitting a patch? that would be great!
I will create a separate issue attempting to track all the other places in LoopBack that has that behavior.

[rmg]

Heads up in case it hasn't occurred to anyone yet...

Relational databases generally avoid using 0 as a primary key. MySQL requires changing the server mode, sqlite3 just doesn't allow it, in Postgres and SQL Server you have to override the default when you define the table/column. I'd be surprised if fixing this in loopback is the end of someone's problems if they are using multiple data sources _and_ have IDs that start at 0. Might be worth a mention in some docs somewhere in case people are used to working with NoSQL and don't realize this might bite them later.

[shaharz]

PostgreSQL allows id 0 - granted, I happened to encounter the issue due to automated testing (which generated & inserted that id) so it's not something that would normally happen.

Deciding to not support id 0 sounds completely reasonable to me, however in that case I'd expect Loopback to assert() on that, and at the very least it should be mentioned in the docs.

[richardpringle]

@davidcheung, what was the result of the discussion? @rmg makes some good points and I tend to agree with the following:

I'd be surprised if fixing this in loopback is the end of someone's problems if they are using multiple data sources and have IDs that start at 0.

Regardless of what the decision is, I think a warning should be issued in the docs somewhere.
^ @crandmck

[davidcheung]

I believe we should still fix it, the only disadvantage is people relying on if(id) { in their code, this is breaking change for sure tho, if people previously had their code doing

if(id) { 
  //operation
}

I have created an issue for this (#2374), probably best to merge it at once so we don't have inconsistent behavior throughout the framework. and when we are close to done we could probably document this behavior at once, I guess it could be documented before this is done too as we have many things on the line for 3.0 release and incase this gets slipped through.

[richardpringle]

@davidcheung, can you follow up with @crandmck for documentation please?

[crandmck]

@davidcheung Could you please clarify what you think needs to be added or changed in docs?

[davidcheung]

@crandmck Not sure where we would document this, not sure if this is only a problem with loopback built-in models (this specific case is about built-in models), but main goal would be to advice Users to not save model with id=0 because it casts to boolean as false, and in some places in loopback we check if(id) which if user's DB auto-increment for some reason starts at 0 then they would experience wrong behavior.

[rmg]

@davidcheung would you be able to write a blog post about the general problem? Then we could drop in links to it in a few places in the docs where people read about custom IDs or whatnot.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale[bot]]

This issue has been closed due to continued inactivity. Thank you for your understanding. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository.