fix: DateTime equality in $dirty

[mdsadiqueinam]

Proposed changes

Describe the big picture of your changes here to communicate to the maintainers why we should accept this pull request. If it fixes a bug or resolves a feature request, be sure to link to that issue.

Types of changes

What types of changes does your code introduce?

Put an x in the boxes that apply

• Bugfix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

• I have read the CONTRIBUTING doc
• Lint and unit tests pass locally with my changes
• I have added tests that prove my fix is effective or that my feature works.
• I have added necessary documentation (if appropriate)

Further comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...

[RomainLanz]

Hey @sadiqueWiseboxs! 👋🏻

Could you please update your message with some description?

We would like to know why you did this change with a proper example.
If you are fixing a bug, we need a test to ensure regression.

[mdsadiqueinam]

Hello @RomainLanz

I am a software engineer and using adonisjs5 for building a backend, while I was doing a manual test on the backend I noticed that the model is updating data in the database even though all the columns are the same, and on debugging the issue I found that if there is any DateTime column in the model excluding createdAt and updatedAt

example Model

import { DateTime } from 'luxon'
import { BaseModel, column } from '@ioc:Adonis/Lucid/Orm'

export default class Product extends BaseModel {
  @column()
  public name: string

 @column.dateTime()
 public expiresAt: DateTime

 @column.dateTime({
      autoCreate: true,
    })
 public createdAt: DateTime
   
 @column.dateTime({
      autoCreate: true,
      autoUpdate: true,
    })
 public updatedAt: DateTime
}

when I will update the data by

product.merge(newData).save()

it will save the data in the database even though the newData is as same as precious data/original data

The reason behind this is $dirt is giving expiresAt
because in $dirt getter originalValue === value will give false even though both DateTime value is the same but both are different instance of 'DateTime'

so there I replace the equals operator with DateTime.equals which will check date and time is same or not, not the instance of DateTime class

[RomainLanz]

Nice catch!

Could you please add a test to ensure we have no regression on this one?

[ghost]

implemented test for check DateTime field is in $dirty

[mdsadiqueinam]

@RomainLanz why there is so many failed test but not in local

[ghost]

@RomainLanz There is several migration test error

[Julien-R44]

Can you please format and lint your code ?
Also, looks like you broke something when implementing your fix. Can you please take a look?

[ghost]

@Julien-R44 I fixed the lint but error is still there

yarn run v1.22.19
warning ..\..\package.json: No license field
$ sh ./scripts/run-tests.sh pg
[+] Running 2/2
 ✔ Container scripts-mysql-1  Removed                                                                                                           1.7s 
 ✔ Network scripts_default    Removed                                                                                                           0.4s 
[+] Running 2/2
 ✔ Network scripts_default  Created                                                                                                             0.1s 
 ✔ Container scripts-pg-1   Started                                                                                                             0.8s 
╭────────────────────────────────────────────────────╮
│    "@japa/run-failed-tests"                        │
│────────────────────────────────────────────────────│
│                                                    │
│                                                    │
│    3 failed test(s) found                          │
│    Applying filter to run only failed tests        │
│                                                    │
╰────────────────────────────────────────────────────╯

MakeMigration (test\commands\make-migration.spec.ts)
  ✖ use custom directory when defined (1s)

Migrator (test\migrations\migrator.spec.ts)
  ✖ use a natural sort to order files when configured (560ms)
  ✖ use a natural sort to order nested files when configured (694ms)

 FAILED 

Tests   : 3 failed (3)
Time    : 2s


✖ use custom directory when defined

  Assertion Error: expected false to be true

- Expected  - 1
+ Received  + 1

- true
+ false

   at Object.executor C:/Users/mdsad/Projects/lucid/test/commands/make-migration.spec.ts:187
   182|
   183|        const successLog = makeMigration.ui.testingRenderer.logs[0].message
   184|        const userSchema = await fs.get(successLog.replace('green(CREATE:)', '').trim())
   185|        const schemaTemplate = await templatesFs.get('migration-make.txt')
   186|
 ❯ 187|        assert.isTrue(successLog.startsWith('green(CREATE:) database/c'))
   188|
   189|        assert.deepEqual(
   190|          toNewlineArray(userSchema),
   191|          toNewlineArray(
   192|            schemaTemplate


✖ use a natural sort to order files when configured

  Assertion Error: expected 'database/migrations/1_accounts' to equal 'database\migrations\1_accounts'

- Expected  - 1
+ Received  + 1

- database\migrations\1_accounts
+ database/migrations/1_accounts

   at Object.executor C:/Users/mdsad/Projects/lucid/test/migrations/migrator.spec.ts:1017
   1012|      const files = await migrator.getList()
   1013|
   1014|      db.getRawConnection('primary')!.config = originalConfig
   1071|    })
   1072|
   1073|    test('raise exception when rollbacks in production are disabled', async ({ assert }) => {
   1074|      app.nodeEnvironment = 'production'


error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

mdsad@Sadique MINGW64 ~/Projects/lucid (basemodel)
$ yarn lint
yarn run v1.22.19
warning ..\..\package.json: No license field
$ eslint . --ext=.ts
Done in 20.54s.

it seems there is some failed test in migration

[Julien-R44]

Yes ignore those, they fail because I guess you run the tests on Windows. They should be adapted but it's not important for the moment. If you look at the pipeline logs then you will see that they pass.

However, there is one that fails and that seems to be linked to the changes you made, look : https://github.com/adonisjs/lucid/actions/runs/4880607319/jobs/8708555577?pr=929#step:6:1968

[ghost]

@Julien-R44 I added a delay of 2ms between user creation and update in user-model test because in test case always set datetime value when autoUpdate is true there is no time gap between user creation and user update which does not make any difference in created and updated time there the $dirty getter is not detecting any difference in joined_at previous and current value, and previously this test was passing because $dirty was always giving DateTime type date as not equal with previous/original value.

[ghost]

@Julien-R44 please review my pr

[mdsadiqueinam]

I have tested this and also required a minor change in test case please review my pr

[stale]

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.

[RomainLanz]

LGTM