Skip to content
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

Feat/soft delete #5034

Merged
merged 31 commits into from Feb 25, 2020
Merged

Feat/soft delete #5034

merged 31 commits into from Feb 25, 2020

Conversation

iWinston
Copy link
Contributor

@iWinston iWinston commented Nov 5, 2019

  1. added @DeleteDateColumn
  2. added the softDelete and restore methods to query builder
  3. added the softDelete and restore methods to repository
  4. added the softRemove and recover methods to repository
  5. added the support of the cascades soft-remove and recover

@iWinston iWinston mentioned this pull request Nov 5, 2019
@iWinston
Copy link
Contributor Author

iWinston commented Nov 5, 2019

Hi, I have forked TypeORM and added some features here: TypeORM+.

The README.md may be useful to show you more details about This PR.

@codeandcats
Copy link

@iWinston thank you for this legendary effort ❤️

I hope this can be pulled in soon.

@shayded-exe
Copy link
Contributor

@iWinston if you set up a BAT wallet so I can tip you, I'll buy you a coffee. This is really great.

@ksharonov
Copy link

Waiting for release)

@pleerock
Copy link
Member

haha great 😆

@vinimdocarmo
Copy link

This will be really helpful! Thanks

@aaronhawkey
Copy link

Excited for this! Much needed. 👏

@mcblum
Copy link

mcblum commented Dec 14, 2019

I realize this is annoying and not helpful to the discussion, but is there any estimated timeline on merging this? I think there are a lot of us who are trying to figure out whether to write our own implementation / use this fork until it's merged / just wait...

Good things take time, but if there's any possibility to merge this it would be greatly appreciated!

@artoodeeto
Copy link
Contributor

@pleerock let the community review this and give you a information if its ok to merged or not, so itll be less burden to you sir. just a suggestion because I think community are waiting for this. please 🙏

@pleerock
Copy link
Member

Okay since so many people want this feature, let's try to revisit the concept and merge this PR.

@pleerock
Copy link
Member

@iWinston good job on implementation, looks really good. Few questions:

  • there is a confusion between softRemove and softDelete. Do we really need both? Also, is it the best name for this method?
  • looks like you didn't implement exclude found results from find method calls. I think this is what people expect the most from this feature. We need to exclude soft deleted rows from find queries, add option to find options to enable it when needed and think if we want to exclude rows from query builder queries

@Destreyf
Copy link

@pleerock It would be wonderful if we could get this feature merged, it would allow me to remove quite a bit of duplicated code from my builds.

I do agree that it would be beneficial (and probably could be required) to support the filtering of soft-deletes in a find query.

@iWinston
Copy link
Contributor Author

iWinston commented Dec 21, 2019

Thank you for your attention. @pleerock

  1. The difference between softRemove and softDelete is the same as remove and delete. One for entities, and one for given conditions.

  2. I have done some work on the Query Scopes which can exclude found results from find method calls easily. And I would like to make a PR if It is considered to be a good idea.

Let me show you how the Query Scopes work:

Query Scopes

Query scopes allow you to add constraints to all queries for a given entity. You can register scopes in your entity:

1. Registering Scopes

import { DeleteDateColumn } from 'typeorm-plus'

export class Entity {

    static scope = {
      'default': {
        deletedAt: IsNull()
      },
      'myScope': {
        deletedAt: Not(IsNull())
      }
    }

}

2. Applying Scopes To QueryBuilder

When you are calling queryBuilder, you can also apply the scope.

import {createConnection} from "typeorm-plus";
import {Entity} from "./entity";

createConnection(/*...*/).then(async connection => {

    const repository = connection.getRepository(Entity);
    await repository.createQueryBuilder().setScope("myScope").getMany();

}).catch(error => console.log(error));

The param scope of the setScope function selects scope to apply to the repository. If it is false, none of the scopes will be applied. If it is undefined, the value will be "default".

3. Applying Scopes To Repository

When you are calling repository, you can apply the scope. The scope mode supports these methods of repository: find, findOne, findOneOrFail, countfindByIds And findAndCount.

import {createConnection} from "typeorm-plus";
import {Entity} from "./entity";

createConnection(/*...*/).then(async connection => {

    const repository = connection.getRepository(Entity);
    // Delete a entity
    await repository.find({
        scope: 'myScope'
    });

}).catch(error => console.log(error));

The property scope of the find options selects scope to apply to the repository. If it is false, none of the scopes will be applied. If it is undefined, the value will be "default".

4. Working with Soft Delete

TypeORM's own soft delete functionality utilizes global scopes to only pull "non-deleted" entities from the database.

If the @DeleteDateColumn is set, the default scope will be "non-deleted".

@pleerock
Copy link
Member

"scopes" you are talking about is a different functionality and should come from a separate discussion and PR if we decide to go with it. Can we implement soft deletion without them? I'm sure most people just want either to load records with soft deleted rows included or records without them, most probably by default. Thanks

@iWinston
Copy link
Contributor Author

@pleerock
Yes, I agree with you about loading records with soft deleted rows or records without them is enough in the most conditions.

We can just add a default "non-deleted" condition in this functionality. And maybe we also need to add the withDeleted: true option to Repository and the withDeleted method to QueryBuilder.

But if we adds "scopes" functionality in the future, should we change the withDeleted: true option and the withDeleted method to another way?

@pleerock
Copy link
Member

But if we adds "scopes" functionality in the future, should we change the withDeleted: true option and the withDeleted method to another way?

I don't think so. soft deletion is kinda standalone feature

We can just add a default "non-deleted" condition in this functionality. And maybe we also need to add the withDeleted: true option to Repository and the withDeleted method to QueryBuilder.

can you implement that?

@feather-jmalone
Copy link
Contributor

@avin-kavish i think the compelling reason -- and I truly mean no disrespect or ingratitude towards @pleerock, @Kononnable, or any other contributor -- is that typeorm's roadmap / release cycle has had its ups and downs since the beginning; 'soft delete' in particular was first proposed in june 2017, with significant interest from the community (#534), but it never took off.

coupled with the fact that this forum is no stranger to cynical discussions about the state of the project overall, i wouldn't be surprised if @iWinston simply wanted to build some features on his own timeline.

but as @johannesschobel said, the work is now merged into the main project, and to me, the end result of this thread has been a very positive signal that typeorm is alive and well (and listening to its community)

@avin-kavish
Copy link

avin-kavish commented Mar 15, 2020

@johannesschobel I know this one is merged, but there's more plus features in the other one.

Suggestion: Instead of TypeORM+, being a fork, can't we have it so that it will augment this library when imported, or maybe it can have this as a dependency? (I don't know if it's possible though) @iWinston

Its a common pattern in C#/ASP.NET to have a library and then another library that extends it's functionality. Since C# supports "extension methods" and that can be attached to classes from other libraries and also class inheritance, it's possible to do this without requiring to fork a project.

I imagine the same can be done with Object.prototype and ES6 classes.

@iWinston
Copy link
Contributor Author

@avin-kavish Hi, I have tried to patch TypeORM to add extra functionalities : TypeORM-patch, but it's too complex to finish it.

Sorry, maybe I shouldn't name it as TypeORM+, I just attend to add more functionalities for our own team at the beginning.

As more and more developers noticed the repo, I kept the repo just intended to get advice from them easily. And when the functionalities became mature, I made some PRs for TypeORM. I have mentioned this on 1 Oct 2019.

Thanks for @pleerock and @Kononnable, TypeORM is a wonderful repo and very helpful for me.

@iWinston
Copy link
Contributor Author

@iWinston hello again sir. sorry to bother you but may I asked why is it softRemove method not implemented in BaseEntity? Thank you and keep rockin.

BaseEntity

Thank you to point it out! I missed this part, maybe you can make a PR for it.

@iWinston
Copy link
Contributor Author

iWinston commented Mar 15, 2020

@iWinston thank you. Can I make a PR for this to put in main typeorm documentation or are you doing it?

Hi, I can make a PR for the doc if it's necessary.

@shayded-exe
Copy link
Contributor

Adding an extensibility framework to TypeORM would be really great. That way we can start bringing new features at our own pace without resorting to forks.

Thanks again for all your work @iWinston

@artoodeeto
Copy link
Contributor

@iWinston Hello sir. for sure I can create a PR for this. Can I use your repo though so you can guide me? this is embarrassing but I'm not really sure about what's happening in the code. :sad: I can also make a pr for the docs this weekend if I'm not busy.

@TheDuckman
Copy link

So, I have a question. Are the soft-deleted entities supposed to show up in the related entities when I execute a findOne()? Is there a specific way to suppress the soft-deleted related entities? I'm looking for a way of doing this using the repository, not the queryBuilder, if possible.

The BloodPressure entities have the "deleted" field correctly set by TypeORM when I call softDelete() but they still show up in the array of related entities.

Can someone help me out with this? Thanks in advance! (sample code below)

@Entity({name: "appointment"})
export class Appointment {
	(...)
	
	@OneToMany(type => BloodPressure, blood_pressure_measurements => blood_pressure_measurements.appointment, {
		eager: true,
		cascade: ['insert', 'update'],
	})
	blood_pressure_measurements?: BloodPressure[];
}

@Entity({name: "blood_pressure"})
export class BloodPressure {
	(...)
	
	@DeleteDateColumn({ name: 'deleted' })
	deleted?: Date
	
	@ManyToOne(type => Appointment)
	@JoinColumn({ name: 'appointment_id' })
	appointment?: Appointment;
}

const repo = getManager().getRepository(Appointment);
const data = await repo.findOne({ id: appointmentId });

@iWinston
Copy link
Contributor Author

@TheDuckman
The default condition of "non-deleted" is only applied to the main alisa.

When you don't want to suppress the soft-deleted related entities in your left join, maybe you should add the condition to the JOIN's ON clause.
Soft delete field causes left join not to return my “left” side data?

@TheDuckman
Copy link

But I don't see a way of setting the "ON" clause explicitly on the JoinOptions of FindOneOptions (repository findOne() options types). From what I've seen, that is only possible when using the queryBuilder. Am I missing something?

If that's the only way, I'll have to use the queryBuilder but I'd really rather use the repository.

@ruslanguns
Copy link

ruslanguns commented Apr 29, 2020

EDITED

I am not sure if opening an issue that is why I am writting here...

I just tried to make cascade working with soft-delete but is not working with child related tables. Only works with first entity...

This is the parent entity()

@Entity('user')
export class User {
  @PrimaryGeneratedColumn({ type: 'bigint' })
  id: number;

  @Index({ unique: true })
  @Column({ type: 'varchar', length: 20, nullable: false })
  username: string;

  @Column({ type: 'varchar', length: 500, nullable: true, select: false })
  password: string;

  @OneToMany(type => UserMetadata, meta => meta.user, { eager: true, cascade: true })
  @JoinTable()
  metadata: UserMetadata[];

  @DeleteDateColumn({ name: 'deleted_at' })
  public deletedAt: Date;
}

The user metadata entity looks like that:

@Entity('user_metadata')
export class UserMetadata {
  @PrimaryGeneratedColumn({ type: 'bigint' })
  id: number;

  @Column({ type: 'varchar', length: 255, nullable: true })
  key: string; 

  @Column({ type: 'varchar', length: 255, nullable: true })
  value: string;  

  @Index()
  @ManyToOne(type => User, (user) => user.metadata)
  @JoinColumn({ name: 'user_id' })
  user!: User;

  @DeleteDateColumn({ name: 'deleted_at' })
  public deletedAt: Date;
}

This is after soft-removing user entity

image

And this is the user_metadata table

image

As you can see soft-deleted did not applied in cascade... I think a method like onSoftDelete should be supported?

Edition

The reason why the cascade was not working was because softDelete unlike save method executes a primitive operation without cascades, relations and other operations included... for cascade should be used softRemove which receives the Entity as a parameter.

In the other hand for recovering also there's the same behavior with recover && restore, restore does the restore but without the cascade, etc. The recover receives an Entity as a parameter and does the cascade for you, however in fact How do you find a softdeleted Entity to provide this as a parameter for recovering?

@iulian03
Copy link

iulian03 commented May 6, 2020

@ruslanguns To recover a soft-deleted entity with all of it's relations, you should provide "withDeleted: true" as an Option when getting that specific entity:

let options: FindOneOptions<YourEntity> = {
            relations: [
                'rel1',
                'rel2'
            ],
            withDeleted: true
        };

        return await this.yourRepository.findOne(id, options);

PS: Also provide what relations you want to be fetched with your Entity.

@jaequery
Copy link

does this not work for ActiveRecord at the moment?

@jaequery
Copy link

@iWinston hello again sir. sorry to bother you but may I asked why is it softRemove method not implemented in BaseEntity? Thank you and keep rockin.

BaseEntity

Thank you to point it out! I missed this part, maybe you can make a PR for it.

is there anyone working on this?

@iWinston
Copy link
Contributor Author

@jaequery Hi, I can make a PR for it in the next few days.

@jaequery
Copy link

@iWinston that would be amazing Winston, thanks

@jaequery
Copy link

Hi @iWinston was this ever implemented?

@johannesschobel
Copy link
Contributor

See the "merged" flag on the PR..

@dan-j
Copy link

dan-j commented Jun 24, 2020

@iWinston is there any limitations on specific databases for this to work? More specifically, for my use case we need this functionality in MongoDB?

From my small understanding of how TypeORM works, I believe the implementation of the MongoDB driver is quite different under the hood compared to SQL databases, a cmd+f in this thread doesn't seem to mention it 🧐

@danielzev
Copy link

@iWinston what are your thoughts on respecting the @DeleteDateColumn in the eager loaded sub-entities, like in @TheDuckman's example with his BloodPressure entity in #5034 (comment)

In his example, the Appointment entity would contain an array of BloodPressure objects, though it would return even soft-deleted ones.

YES, we could use the query builder and build our own queries everywhere throughout the app. Though if that's the case, then we don't really need the @DeleteDateColumn, and probably not a good idea to use it if some spots use it and some spots don't.

I feel like everywhere throughout the ORM, and it's internal query builders, it should respect the field IS NULL where statement. Or maybe it's an option you can pass through in the repository functions. Something like cascadeDeleteCheck or something like that.

@Ihor-Lavrov
Copy link

@pleerock @iWinston hi guys seems like soft deletion and withDeleted flags work perfect, but I've wasted a ton of time to find a description of withDeleted flags in this thread. There are now additional info anywhere in docs or somewhere else. Could you please update the description of soft deletion and approaches to work within soft deletion in typeorm docs included with withDeleted flag. It would be very helpful! Thanks

@Ihor-Lavrov
Copy link

Ihor-Lavrov commented Sep 23, 2020

@pleerock @iWinston
And I've found a possible issue: if we have subqueris seems like we should add withDeleted methods both to main query and subquery. As for me it's little bit weired and may cause to overhead conditional comparing.

repository.createQueryBuilder('entity')
            .innerJoin(
                subQuery => {
                    subQuery
                        .select('search.pk', 'pk')
                        .withDeleted()
                        .from(entity, 'search');
                    return subQuery;
                },
                'sub',
                'sub.pk=entity.pk'
            )
            .withDeleted()
            .getMany();

(little bit cut code fragment)
I'd appreciate if you may comment this.

@nolazybits
Copy link

@danielzev have you found any solution. We hit the same problem where having relationship with eager loading will load them all even the soft deleted ones.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet