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. |
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.
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.
Will do the second portion of disallowing prototype names as field names in a follow-up PR.
Why
Noticed this while reading through mutator code. The issue is that previously we would not re-load the entity if
skipDatabaseUpdatewas 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.