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

[Feature Request] Simple way to extend/override TypeOrmCrudService #226

Closed
alexmantaut opened this issue Aug 21, 2019 · 12 comments · Fixed by #318
Closed

[Feature Request] Simple way to extend/override TypeOrmCrudService #226

alexmantaut opened this issue Aug 21, 2019 · 12 comments · Fixed by #318

Comments

@alexmantaut
Copy link

alexmantaut commented Aug 21, 2019

Hi,

I am trying to create a custom TypeOrmCrudService for my application, overriding the behaviour of the different methods...

To do this, I am basing my override methods on the current implementations on TypeOrmCrudService. Thing is, there is a lot of really useful methods on the base class (ex getOneOrFail(), prepareEntityBeforeSave(), etc) , but I cannot use them on my override method, because they have private visibility...

Would it be possible to make those methods protected instead, to be able to call them from derived classes? Or do you feel this would make the interface harder to maintain? If not, what would be a good way to be able to use those methods?

Thanks,

@alexmantaut alexmantaut changed the title [Feature] Simple way to extend/override TypeOrmCrudService [Feature Request] Simple way to extend/override TypeOrmCrudService Aug 21, 2019
@RDeluxe
Copy link

RDeluxe commented Sep 10, 2019

Agreed, that's not a big change but that would make things easier.

@jbjhjm
Copy link

jbjhjm commented Oct 7, 2019

Yes please. Private methods are so annoying for those who needs to customize functionality. +1 for protected methods!

@mbaroukh
Copy link

mbaroukh commented Oct 26, 2019

Hi.
I'm new to nest. I use this module because it allow to quickly bootstrap a project (coupled with react-admin). Thanks for it :). But I also had this problem. For example, all my entities have a modificationDate field and I want to update them before saving.
Do I miss a method to actually do it easily ?

For now, I made de TypeORM Repository wrapper which use a method beforeSave called to wrap entity before each save :
https://gist.github.com/mbaroukh/841618b97fce7aa29c08a0bd2daff35b

It seems to work for my need.
I use it this way :

@Injectable()
export class ClientsService extends TypeOrmCrudService<Client> {
 constructor(
   @InjectRepository(Client) private readonly clientRepository: Repository<Client>,
 ) { 
   super(new RepositoryWrapper(clientRepository, e => {
     return {
       ...e,
       modificationDate: new Date()
     }
   }))
 }
}

But I can't believe there is no other way to do this.

@jbjhjm
Copy link

jbjhjm commented Oct 28, 2019

@mbaroukh
No need to set this up via a service, typeORM offers this out of the box:

export class MyEntity {
  // ... 
  // date column which will be static after INSERT
  @CreateDateColumn({ name: 'created_at', nullable: false })
  createdAt: Date;

  // date column which will be updated on every UPDATE
  @UpdateDateColumn({ name: 'updated_at', nullable: true })
  updatedAt: Date;

  // if you need versioning, this will be incremented on every UPDATE
  @VersionColumn() version: number;

}

@mbaroukh
Copy link

very good point @jbjhjm , thank you !
But I will continue my way until prepareEntityBeforeSave() is protected for now because modification date is not the only column.
I have other fields to consolidate on save.

Thanks !

@jbjhjm
Copy link

jbjhjm commented Oct 28, 2019

@mbaroukh there's a solution for this too:

export class MyEntity {

  @BeforeUpdate()
  prepareTheEntity() {
    // this code will be executed before running queries to update the db table.
    // There are others like @BeforeInsert(), @AfterUpdate() etcpp.
    // in your case just do the following here to achieve the same:
    this.modificationDate = new Date()
  }

}

@mbaroukh
Copy link

Hi @jbjhjm. I already tried this. But the event is not fired.
I suppose I reach this issue and maybe it has not been published yet in stable version :
#51

I tried to upgrade to 4.3.0-alpha.0 (upgrading crud, crud-typeorm and crud-request but I have this typescript error :

node_modules/@nestjsx/crud-request/lib/types/request-query.types.d.ts:62:5 - error TS2411: Property '$and' of type 'undefined' is not assignable to string index type 'string | number | boolean | SFieldOperator | (SFields | SConditionAND)[]'.

62     $and?: never;

But I must says that I'm not fan of this method anyway. I think that every interaction with entity must be centralized in service, dao or repository. This is why being able to override prepareEntityBeforeSave would be a greater option imho.

@jbjhjm
Copy link

jbjhjm commented Oct 30, 2019

@mbaroukh in 4.3.0-alpha0 this should work fine, I'm using it in my repo and had no problems at all. What does the code of your entity looks like? I guess you made sure the error's gone when you don't use one of the event decorators.

Best practice depends on what you need to do I think.
If it's transforming data etc my opinion is it's good to do this inside the entity as it allows you to use it easily and not need to care about internals just the final entity with transformations etc applied.

If you want to decouple this typeORM offers the same events to be placed in a special class named Subscriber which is essentially like a service allowing the same event listeners to be specified as methods. Be aware though that it's default behavior is to subscribe to all events of a CONNECTION, not an entity.

Currently I'm using such subscribers like a "plug-in-solution" for typeORM to provide custom db functionality based on attached Decorator metadata. I think it's the best / most reusable method that's available right now.


Nevertheless, I agree 100% it would be beneficial to use protected methods to allow for customization without headaches :)

@mbaroukh
Copy link

@jbjhjm in 4.3.0-alpha.0, I only updated crud-xxx packages and I have this error whenever I use or not the event decorators. I saw this this morning and could not investigate more. I'm surprised it works for you :(. I tried with TS 3.6.3, 3.6.4 and 3.7.1.

You are right that there are multiple solutions and that's what is nice :).
My bias is that I'm coming from Java world where I uses to have simple objects for entities (POJO).
I'll look at those subscribers, thanks.

@jbjhjm
Copy link

jbjhjm commented Oct 30, 2019

@mbaroukh
Hmm I'm using an older typescript / nestjs version. Possibly it's broken with most recent versions.
Should plan in some time to do updates soon. My setup is:

{
    "@nestjs/common": "^6.5.3",
    "@nestjs/core": "^6.5.3",
    "@nestjs/platform-express": "^6.5.3",
    "@nestjs/swagger": "^3.1.0",
    "@nestjs/typeorm": "^6.1.3",
    "@nestjsx/crud": "^4.3.0-alpha.0",
    "@nestjsx/crud-typeorm": "^4.2.0",
    "typescript": "^3.5.3",
}

@mbaroukh
Copy link

mbaroukh commented Nov 3, 2019

well, this error disappear if I use "strict: false" in tsconfig.json.
But I don't wan't to do that. Nobody should disable strict mode :(.

@alexmantaut
Copy link
Author

Just created a PR for the issue with the visibility. I was conservative and only changed the visibility of 3 methods, as I was unsure whether it was a good idea to change visibility for other methods...

Please let me know if you want me to change the visibility of other methods in the class as well.

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 a pull request may close this issue.

4 participants