-
Notifications
You must be signed in to change notification settings - Fork 4
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: always reload entity after update since cascading changes may have changed it since commit #233
Conversation
…ve changed it since commit
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #233 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 69 69
Lines 1892 1895 +3
Branches 265 261 -4
=========================================
+ Hits 1892 1895 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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
packages/entity/src/EntityMutator.ts
Outdated
private ensureStableIDField(updatedFields: Partial<TFields>): void { | ||
const originalId = this.originalEntity.getID(); | ||
const idField = this.entityConfiguration.idField; | ||
if (idField in updatedFields && originalId !== updatedFields[idField]) { |
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.
In practice this won't matter, but if idField were "constructor" or "toString", we don't want to confuse those with Object.prototype.constructor, etc.
if (idField in updatedFields && originalId !== updatedFields[idField]) { | |
if (updatedFields.hasOwnProperty(idField) && originalId !== updatedFields[idField]) { |
It probably wouldn't be a bad idea to ban fields named any of the following:
Object.prototype.__proto__
Object.prototype.constructor
Object.prototype.hasOwnProperty
Object.prototype.isPrototypeOf
Object.prototype.propertyIsEnumerable
Object.prototype.toLocaleString
Object.prototype.toString
Object.prototype.valueOf
Technically it is possible to write code in a way that allows for these fields (use hasOwnProperty()
instead of in
, including for ... in
) but "constructor" is the only field I could see someone wanting to use and they should just choose another name and move on.
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.
Will do the second portion of disallowing prototype names as field names in a follow-up PR.
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.
Why
Noticed this while reading through mutator code. The issue is that previously we would not re-load the entity if
skipDatabaseUpdate
was true since we assumed the entity was unchanged. But there are some scenarios where this isn't true, namelySET_NULL_INVALIDATE_CACHE_ONLY
.For example, if a field in this entity is set null when cascade deleting, we want to reload it before calling the after-update triggers (especially the post-commit ones).
Closes ENG-12407.
How
Always re-load the entity. Note that this requires an explicit check to ensure a stable ID. Previously this check was implicitly enforced by the database adapter.
Test Plan
Run new tests.