adonisjs / lucid

AdonisJS SQL ORM. Supports PostgreSQL, MySQL, MSSQL, Redshift, SQLite and many more
https://lucid.adonisjs.com/
MIT License
1.03k stars 189 forks source link

$isDirty not using correct equality semantics on DateTime values #903

Closed juni0r closed 1 year ago

juni0r commented 1 year ago

Two DateTime values that represent the same time in different time zones aren't considered equal by BaseModel.$isDirty. It uses === to compare DateTime values which is not the proper semantics since both equals() as well as strict equality === for the respective valueOf() two DateTimes would be true:

const d1 = DateTime.fromISO("2000-01-01T12:00:00+02:00")
const d2 = DateTime.fromISO("2000-01-01T10:00:00Z")

d1 === d2  // => false :-o
d1.equals(d2) // => true
d1.valueOf() === d2.valueOf() // => true

https://github.com/adonisjs/lucid/blob/98644992cbd55cd1fdaa4b1fcf40d38540440083/src/Orm/BaseModel/index.ts#L1244-L1253

thetutlage commented 1 year ago

Nice catch. We should be using value.equals(originalValue) here. Can you please send a PR for the same?

juni0r commented 1 year ago

Upon closer inspection I realized that DateTime's concept of equality is … well … eccentric

Two DateTimes are equal iff they represent the same millisecond, have the same zone and location, and are both valid. To compare just the millisecond values, use +dt1 === +dt2.

I don't want to argue with this, but in my view a date represents a moment in time which is the same, no matter if you're in India, Germany or on the Moon. I just think you should be aware since there might be other parts of the codebase where this becomes an issue.

In terms of an ORM I believe it's the most sensible choice to consider two dates equal if their timestamp is equal, hence valueOf() which is precisely what fast-deep-equal uses. So that would boil down to this:

-      if (DateTime.isDateTime(value) || DateTime.isDateTime(originalValue)) {
-        isEqual = value === originalValue
-      } else if (isObject(value) && 'isDirty' in value) {
+      if (isObject(value) && 'isDirty' in value) {
         isEqual = !value.isDirty
       } else {
         isEqual = equal(originalValue, value)
stale[bot] commented 1 year ago

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.