-
Notifications
You must be signed in to change notification settings - Fork 235
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
fix creation of LB4 models with auto-generated id #553
Conversation
- Add a try/catch block to prevent `coerceId` from crashing the process on uncaught exception (forward the coercion error via callback). - Fix `isObjectIDProperty` to use the correct ObjectID constructor in `instanceof` check. Signed-off-by: Miroslav Bajtoš <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 LGTM
I checked all the mongo id test cases, I try to summarize the CRUD behavior on
Please correct me if anything is incorrect. |
@agnes512 It's a great idea to document behavior for different combinations of To be honest, I find it difficult to understand the values in table cells. What does it mean when the value is I think that my pull request should not be changing the behavior, I am just fixing an incorrectly written |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Posting few comments to capture what I learned while making this change.
@@ -1963,7 +1969,7 @@ function isObjectIDProperty(modelCtor, propDef, value, options) { | |||
if (typeof value === 'string' && value.match(ObjectIdValueRegex)) { | |||
if (isStoredAsObjectID(propDef)) return true; | |||
else return !isStrictObjectIDCoercionEnabled(modelCtor, options); | |||
} else if (value instanceof ObjectID) { | |||
} else if (value instanceof mongodb.ObjectID) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ObjectID
is a factory function that creates new mongodb.ObjectID
, it's not a class. Therefore the expression value instanceof ObjectID
is never true.
type: 'string', | ||
id: true, | ||
generated: true, | ||
mongodb: {dataType: 'ObjectID'}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the property definition used in LB4.
When the primary keys is auto-generated by the database, juggler replaces the type provided by the user (string
) with the type defined by the connector (mongodb.ObjectID
).
As a result, we end up with a property defined as {type: mongodb.ObjectID, mongodb: {dataType: 'ObjectID'}}
, which is something we were not handling well before my pull request.
In order to keep it short I use short terms. T for
The top right case was supposed to be true, but we didn't have a test case, and it was a bug. I think your PR fix it 👍 |
Add a try/catch block to prevent
coerceId
from crashing the process on uncaught exception (forward the coercion error via callback).Fix
isObjectIDProperty
to use the correct ObjectID constructor ininstanceof
check.This fixes the problem described in loopbackio/loopback-next#4059 (comment).
Checklist
👉 Read and sign the CLA (Contributor License Agreement) 👈
npm test
passes on your machine