fix: resolve issue delete column null on after update event subscriber

[spotykatch]

Fixes: #6327
Fixes: #6349

Description of change

Subscriber afterUpdate now gets entity with changed delete and update date column.

Adds softDelete, getSoftDeletionReturningColumns methods to ReturningResultsEntityUpdator class to get right returning columns and search for entity withDeleted(). Resolves bug in SubjectExecutor class. Adds tests for the issue.

Pull-Request Checklist

• Code is up-to-date with the master branch
• npm run lint passes with this change
• npm run test passes with this change
• This pull request links relevant issues as Fixes #0000
• There are new or updated unit tests validating the change
• Documentation has been updated to reflect this change
• The new commits follow conventions explained in CONTRIBUTING.md

[spotykatch]

All local tests for cockroach DB passed. Circleci failed due to timeout. Should I rebase without changes?

[spotykatch]

So, I don't really know what to do when I have to merge. What should I do to make this pr to be included in the next version?

[pleerock]

Is this line of code the only difference with update method? If so, I would like to re-use update method to prevent another peace of complex code to be added into the repo... We can make update method to accept returning columns if needed.

[pleerock]

oh another diff I see is withDeleted. Maybe we can add a third parameter to update, like type: "default" | "soft-delete". Dunno, better to came up with better names, but having single update method is better for sure.

[spotykatch]

Ok. Got it.
I think withDeleted should be used with updates too. So if the entity is changed when soft deleted, it will fire afterUpdate event. It will not if there is no withDeleted.

[spotykatch]

Ok. Let me think in writing ))) Just helps me to think))) Those are only my thoughts. You can skip it.
To get rid of softDelete method we do this:
As this.expressionMap passed by reference in SoftDeleteQueryBulder.execute() and UpdateQueryBuilder.execute() methods and this.expressionMap.extraReturningColumns are the columns we need to update

// if update entity mode is enabled we may need extra columns for the returning statement
const returningResultsEntityUpdator = new ReturningResultsEntityUpdator(queryRunner, this.expressionMap);
if (this.expressionMap.updateEntity === true &&
    this.expressionMap.mainAlias!.hasMetadata &&
    this.expressionMap.whereEntities.length > 0) {
    this.expressionMap.extraReturningColumns = returningResultsEntityUpdator.getUpdationReturningColumns();
}

and expressionMap is required in the ReturningResultsEntityUpdator constructor

constructor(protected queryRunner: QueryRunner,
            protected expressionMap: QueryExpressionMap) {
}

and extraReturningColumns are never null or undefined

/**
 * Extra returning columns to be added to the returning statement if driver supports it.
 */
extraReturningColumns: ColumnMetadata[] = [];

we don't need to get updationColumns like so

const updationColumns = this.getSoftDeletionReturningColumns();

insead we can use

const updationColumns = this.expressionMap.extraReturningColumns;

So that updationColumns are unique for update and soft-delete and we don't need to call the same method multiple times.

Also, we can use withDeleted() for both update and soft delete as select is done by id. Guess it won't slow down a lot even with lots of soft-deleted rows. I already mentioned, that if the entity is changed while soft-deleted it won't make changes to the entity fired to afterUpdate event. And we can lose the blame in history if the event is used for that case.

[pleerock]

what is the motivation behind this sleep for a second?

[spotykatch]

SQLite doesn't see changes in the date if it's less than a second. Should be a check for a driver?

[pleerock]

yes you can do it, also please add a detailed description above it.

[spotykatch]

Ok. Let me think in writing ))) Just helps me to think))) Those are only my thoughts. You can skip it.
Now, when I thought for some time and made some tests, I do have some doubts about making a check for a driver. I found out that Mysql and MariaDB randomly fail tests. Course, I don't know, they sometimes do not care about milliseconds? So if a new driver will be added like "DatabeseThatDoesntCareAboutMillisecondsToo", it can course some damage in the test cases.
So we've got two options.

• Leave await sleep(1000); without checks (only comments). As all the checks are done asynchronously it will not take really long time.
• Check only for updated values in the event passed to afterUpdate. If there are some values then it's ok. We 99% know why it doesn't work and we know what to check and we don't want to test a single commit for 3 hours using a ton of memory )))

[pleerock]

TBH I still didn't get why timeout is needed. Commit to the database happens after line 25, I don't understand why and why don't see changes

[spotykatch]

Was working in saturday. Sorry. Running tests now.
This line fails for sqlite, better-sqlite, mysql and mariadb if there's no timeout, as timestamp doesn't care about millisecons.
expect(entity!.updatedAt.getTime()).not.to.be.eq(data!.softDeleteDate.getTime());
I'll delete it.

[spotykatch]

Made a real mess with the commits. Should I create new pr and close this one?

[pleerock]

Made a real mess with the commits. Should I create new pr and close this one?

Not necessary, but go ahead if you want.