typestack / typeorm-typedi-extensions

Dependency injection and service container integration with TypeORM using TypeDI library.
MIT License
262 stars 37 forks source link

Constructor DI in entities #2

Closed MichalLytek closed 8 years ago

MichalLytek commented 8 years ago

I wonder if it's possible to have a constructor depenency injection using TypeDI in entities. To clarify situation this is sample code, not real case πŸ˜‰ But I just need some logic in one of my entity which depend on 3rd party module:

@Service()
@Table()
export class DependentEntity {
    @Column()
    public name: string;

    private dependency: TheLib;
    public constructor(@Require("node-lib") dependency: TheLib) {
        this.dependency = dependency;
    }

    public doSomething() {
        return this.dependency.doSomething();
    }
}

It's so much easier to mock that the normal, require one:

import * as nodeLib from "node-lib";

@Table()
export class DependentEntity {
    @Column()
    public name: string;

    public doSomething() {
        return nodeLib.doSomething();
    }
}

When I can't use constructor DI, I can do something like this:

import * as nodeLib from "node-lib";

@Table()
export class DependentEntity {
    @Column()
    public name: string;

    private static dependency: TheLib = nodeLib;
    public static setDependency(dependency: TheLib) {
        this.dependency = dependency;
    }

    public doSomething() {
        return DependentEntity.dependency.doSomething();
    }
}

But then I had to remember to mock the dependency everytime in every test

beforeEach(() => {
    User.setDependency(new MockDependency())
});

because I don't have compilator error about missing constructor parameter. What will happen when there will be more classes and more dependencies?

I see in code that you just call constructor with no arguments:

if (this.target instanceof Function)
    return new (<any> this.target)();

But maybe you could first call Container.get(this.target)? Could it works? πŸ˜‹

pleerock commented 8 years ago

bad idea. your entities should be dummy, they aren't services and dont even think about it. Also I don't recommend to use @Require("node-lib") since its experimental feature of typedi. Instead better if you import dependencies using strandard es6 import syntax. What about mocking, I think because of dynamic nature of javascript there are other ways too to mock your imported objects t

MichalLytek commented 8 years ago

The whole TypeORM is in alpha and have strange bugs but it's so great that I can't just wait and use Mongoose or Sequelize 😜 For me and for now @Require works ok.

The real life example is User and password hashing. I use argon2 module which is async. My user entitity has hashedPassword field which is private (encapsulation) because I don't want to let placing there any string by accident.

So it has two methods - setPasswordAsync, which perform async hashing and set the property, and verifyPasswordAsync, which hashes the password from argument and compares it with this.hashedPassword.

I can make the entities dummy and move this methods to a new class and inject it into controller but to do that I would have to make hashedPassword public, so by mistake I could assign to this property the plain password and store it in database because I don't have any guards.

I know that JS is dynamic & easy to mock but we want use DI to have clean & testable code and don't have to play with mock-require and other stuffs πŸ˜‰

pleerock commented 8 years ago

just keep your encryption logic in separate services, not in the models. and call these methods and save encrypted result into the user before you save your users. I do it this way

MichalLytek commented 8 years ago

I've already have written the service but there was a problem in dependency injection:

import { Require, Service } from "typedi";

import { HashingLibrary } from "./hashing-library";

@Service()
export class PasswordHashingHelper {

    private readonly hasher: HashingLibrary;

    public constructor(@Require("argon2") hasher: HashingLibrary) {
        this.hasher = hasher;
    }

    public async hashPasswordAsync(plainPassword: string): Promise<string> {
        const salt = await this.hasher.generateSalt();
        return this.hasher.hash(plainPassword, salt);
    }

    public verifyPasswordsAsync(passwordHash: string, plainPassword: string): Promise<boolean> {
        return this.hasher.verify(passwordHash, plainPassword);
    }
}

But then if in User I can have this:

public async setPasswordAsync(plainPassword: string): Promise<void> {
    this.hashedPassword = await this.passwordHashingHelper.hashPasswordAsync(plainPassword);
}

I have to do this in controller (user registration):

user.hashedPassword = this.passwordHashingHelper.hashPasswordAsync(bodyUser.password);

instead of

user.setPasswordAsync(bodyUser.password);

Which demands me to set hashedPassword as public and I can do

user.hashedPassword = bodyUser.password

by accident, which I would like to avoid.

BTW, service and helper is the same? Or there's a difference and I should change this name to HashingService? :wink:

pleerock commented 8 years ago

probably its the same if you treat your helpers as services. I don't think anything bad in your code. If you don't want to expose hashedPassword then simply create a setter for it, without getter, thats all

MichalLytek commented 8 years ago

I thought about this but currently TS support only readonly so if you create only a setter, you don't have compile-time error about let pass = user.password; - it will return undefined in runtime. I will have to make a "Java-like" set function.

But anyway, thanks for explanation, I will figure sth out πŸ˜‰

MichalLytek commented 8 years ago

With the private property with setter and no getter, I'm not able to verify password from body with password hash of user from database.

So it have to be public or exposed by getter and setter or I have to import it and use in entities method like in the earliest version before I wanted testability and mock hashing in unit test.

Or I will fork TypeORM to add factories for creating entities with dependency, hahah πŸ˜†

github-actions[bot] commented 3 years ago

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.