feat/refactor to typescript

[brarsanmol]

trafficstars

List of Changes:

• Migrate from JavaScript to TypeScript for model and service layer.
• Migrate from MongoDB to Postgres.
  • Migrate from Mongoose to TypeORM.
    • TypeORM supports SQL and Mongo, so we can go back.
• Upgrade outdated packages
  • express-winston & winston.
• Create start script for building and testing
• Adding, removing, and updating members from teams.
• RBAC seems to be completely broken as the relevant data is not being loaded into the database.
• Migrate controllers and middleware to TypeScript.
• Migrate to a decorator based approach to controllers and middleware.
• Proper dependency injection (we now use TSyringe).

Incomplete Features:

• Setting's do not load into the database (this is broken by purpose as this should be a configuration file?).
  • The Settings Controller returns the current Date + Time Period for Expiry as of now, but the model and service still needs to be created.

To-Do:

• Flesh out a finalized database schema and verify that all relations between entities are valid.
• Change testing kit to support SQL instead of Mongo (simply modifying the identifier's on the dummy objects).
  • Maybe migrate to Jest?

Future Vision:

• Migrate to a decorator based approach to controllers and middleware.
  • A library already exists for the controller's portion.
    • https://www.npmjs.com/package/@decorators/express
    • It allows us to add middleware as a parameter onto the route but I would prefer if authentication and authorization were their own decorators.
      • Example: @Authorize("Admin,User,...")
      • Example: @Authenticated()

Type of Change:

• [x] New feature (non-breaking change which adds functionality)
• [X] Breaking change (fix or feature that would cause existing functionality to not work as expected)
• [X] New release
• [X] This change requires a documentation update

Checklist:

• [ ] My code follows the style guidelines of this project
• [ ] I have performed a self-review of my own code
• [ ] I have commented my code, particularly in hard-to-understand areas
• [ ] My changes generate no new warnings
• [ ] Listed change(s) in the Changelog
• [ ] I have added tests that prove my fix is effective or that my feature works
• [ ] New and existing unit tests pass locally with my changes
• [ ] I have made corresponding changes to the documentation
• [ ] Any dependent changes have been merged and published in downstream modules

[krubenok]

IMO - this should target a branch other than dev since there will probably need to be patches to dev and main before this is ready to be used in prime time. Maybe a refactor branch or something.

Exciting though! cc: @pierreTklein

[krubenok]

Migrate from MongoDB to Postgres. Migrate from Mongoose to TypeORM. TypeORM supports SQL and Mongo, so we can go back.

I explicitly disagree with migrating away from Mongo. I think Mongo is actually better for what the API is trying to do, but we're obviously not using it correctly. Schema-less gives us more flexibility to change things year to year and nest things tidily.

[krubenok]

Setting's do not load into the database (this is broken by purpose as this should be a configuration file?).

I forget where we left off on this, but we wanted more of what's currently in the config file to be in the database. I think moving back towards a config file is a mistake.

[krubenok]

Aside: I also think a major refactor like this is a good opportunity to investigate documentation and developer experience changes. We have API docs that are generated, but I think it's worth looking at alternatives to see if there's anything better out there.

Also repo onboarding easier and better documented would be hugely valuable.

[brarsanmol]

IMO - this should target a branch other than dev since there will probably need to be patches to dev and main before this is ready to be used in prime time. Maybe a refactor branch or something.

Exciting though! cc: @pierreTklein

Agreed, I will create a new dedicated branch for this merge to feed into.

[brarsanmol]

Migrate from MongoDB to Postgres. Migrate from Mongoose to TypeORM. TypeORM supports SQL and Mongo, so we can go back.

I explicitly disagree with migrating away from Mongo. I think Mongo is actually better for what the API is trying to do, but we're obviously not using it correctly. Schema-less gives us more flexibility to change things year to year and nest things tidily.

Hmm, I see. We would definitely need to come up with a better domain model, a suggestion might be embedding objects? My initial push towards Postgres was because of JSONB, as that would still allow us to store unstructured data, and SQL being a first class citizen with TypeORM.

[brarsanmol]

Setting's do not load into the database (this is broken by purpose as this should be a configuration file?).

I forget where we left off on this, but we wanted more of what's currently in the config file to be in the database. I think moving back towards a config file is a mistake.

Sounds good, I broke this functionality as the default values that were provided in the database were causing issues when launching the application, and I did not have the exact amount of time to figure out why, I will investigate further and reintegrate the functionality.

[brarsanmol]

Aside: I also think a major refactor like this is a good opportunity to investigate documentation and developer experience changes. We have API docs that are generated, but I think it's worth looking at alternatives to see if there's anything better out there.

Also repo onboarding easier and better documented would be hugely valuable.

This will be my next step. Two ideas that I have of the top of my head are:

• Integrating the stale PR from @meldunn for Postman Collections.
• Write a web-page or script for developer onboarding.
  • Webpage could be something similar to this from Gitea/Gogs:
  • 

Documentation is tricky, there is still a lot more code to rewrite, as the aim was simply to get the codebase functioning with this initial commit. I would suggest that a formal write-up be made for the future of the application architecture and then we slowly build up from there documenting middleware and controllers as necessary. The service layer require's minimum modification other than a few redundant functions that can be removed.

[krubenok]

Good on the other bits re:onboarding.

Documentation is tricky, there is still a lot more code to rewrite, as the aim was simply to get the codebase functioning with this initial commit. I would suggest that a formal write-up be made for the future of the application architecture and then we slowly build up from there documenting middleware and controllers as necessary. The service layer require's minimum modification other than a few redundant functions that can be removed.

Yeah - I meant more examining other doc generation packages and seeing if we still like the one we're using. For example, the one we're using makes it hard to do that postman generation that @meldunn and I did. Other docstrings and doc generation tools make that a lot easier.

[krubenok]

Migrate from MongoDB to Postgres. Migrate from Mongoose to TypeORM. TypeORM supports SQL and Mongo, so we can go back.

I explicitly disagree with migrating away from Mongo. I think Mongo is actually better for what the API is trying to do, but we're obviously not using it correctly. Schema-less gives us more flexibility to change things year to year and nest things tidily.

Hmm, I see. We would definitely need to come up with a better domain model, a suggestion might be embedding objects? My initial push towards Postgres was because of JSONB, as that would still allow us to store unstructured data, and SQL being a first class citizen with TypeORM.

Yes - We should be nesting objects in Mongo. We've discussed some ideas around this separately too. I'm curious what a now wiser @pierreTklein thinks about this now though...

[pierreTklein]

Thanks for this! Cool to see the backend converted to TS. I would personally push to separate out each of these bullet points into separate PRs, because they seem unrelated to the typescript migration.

That way, we can focus on each part more closely.

As for the database schema migration, I'm fine with nested schemas. Would be happy to review changes.

[brarsanmol]

Thanks for this! Cool to see the backend converted to TS. I would personally push to separate out each of these bullet points into separate PRs, because they seem unrelated to the typescript migration.

That way, we can focus on each part more closely.

I agree, I will set up the refactor branch sometime today and split up the changes recommended in this PR to separate requests.

As for the database schema migration, I'm fine with nested schemas. Would be happy to review changes.

I would love to set up some time to chat with you and @krubenok further on this. There are still some prickly models that might cause issues, for example one difficult implementation might be Teams, simply nesting the object into each user would be a terrible choice as it would create massive duplication, and using the relational id tag would lead us back down the same road that we are on.

Edit: I had a chat with @krubenok earlier, he suggested an async discussion format on a GitHub issue, I will create this issue later tonight and the relevant discussions can take place there with synchronous discussions being organized later if need be.

[brarsanmol]

I've gotten back on the saddle for a short period again. Here is some of the work that I've done with decorators and how the future service and controller layer may look.

I have already migrated the Database Service, Logger Service, and Environment Service to a similar format, I can commit said code if needed, but as of now I'm not deeming it essential.

Account Controller

@autoInjectable()
@controller("/account")
export class AccountController {
    constructor(private readonly accountService: AccountService) {}

    @action(ActionType.GET, "/")
    @middleware([ensureAuthenticated, ensureAuthorized])
    async get(request: Request, response: Response) {
        const account:
            | Account
            | undefined = await this.accountService.findByEmail(
            request.user?.email
        );

        if (!account) {
            return response.status(400).json({
                message: ErrorConstants.ACCOUNT_404_MESSAGE
            });
        }

        return response.status(200).json({
            message: SuccessConstants.ACCOUNT_READ,
            data: account.toStrippedJSON()
        });
    }
}

Account Service

@autoInjectable()
export class AccountService {
    constructor(
        private readonly userRepository: Repository<Account> = getRepository(
            Account
        ),
        private readonly loggerService: LoggerService
    ) {}

    public async findByIdentifier(
        identifier: number
    ): Promise<Account | undefined> {
        const TAG = `[Account Service # findByIdentifier]:`;
        return await this.userRepository
            .findOne(identifier)
            .then((account: Account) => {
                this.loggerService.queryCallbackFactory(
                    TAG,
                    "account",
                    identifier
                );
                return account;
            });
    }

    public async findByEmail(email: string): Promise<Account | undefined> {
        const TAG = `[Account Service # findByEmail]:`;
        return await this.userRepository
            .findOne({ where: { email: email } })
            .then((account: Account) => {
                this.loggerService.queryCallbackFactory(TAG, "account", email);
                return account;
            });
    }

    public async save(account: Partial<Account>): Promise<Account> {
        const TAG = `[Account Service # addAccount]:`;
        return await this.userRepository.save(account);
    }

    public async update(
        identifier: number,
        account: Object
    ): Promise<UpdateResult> {
        const TAG = `[Account Service # update]:`;
        return await this.userRepository.update(identifier, account);
    }

    public async updatePassword(identifier: number, password: string) {
        return await this.userRepository.update(identifier, {
            password: this.hashPassword(password)
        });
    }

    public async getAccountIfValid(
        email: string,
        password: string
    ): Promise<Account | undefined> {
        const account = await this.findByEmail(email);
        return account && account.comparePassword(password)
            ? account
            : undefined;
    }

    public hashPassword(password: string): string {
        return hashSync(password, 10);
    }
}

Edit: I will create attempt to organize this work soon into multiple PR's or places where we can facilitate discussion once I am free from exams, as of now I see this as fit because I don't see much more work being done over the next 2 weeks on this.

[brarsanmol]

Some further progress has been made. I decided integrate a library that I was using by Lucas Huggler into our codebase as a ways to create a more elegant solution. It is licensed through MIT so I believe it should be okay for us to use it as long as we provide original credit on the router.*.ts files.

This is some really important code, and I don't think I'll be able to get my project compiling for a little bit so any bug's that can be caught would much appreciated!

router.service.ts

type Method = (
    request: Request,
    response: Response,
    next: NextFunction
) => Promise<Response<any, Record<string, any>>>;

export enum ActionType {
    GET = "get",
    POST = "post",
    PUT = "put",
    DELETE = "delete",
    PATCH = "patch",
    ALL = "all"
}

interface ActionMetadata {
    type: ActionType;
    route: string;
}

@autoInjectable()
export class RouterService {
    constructor(
        private readonly router: Router = Router(),
        private readonly controllers: Array<Function>
    ) {
        this.controllers.forEach((controller) =>
            this.addController(controller)
        );
    }

    private addController(controller: Function): void {
        const actions = Object.getOwnPropertyNames(controller.prototype);
        const metadata = Reflect.getMetadata("controller", controller);
        if (!metadata) return;
        for (const action in actions) {
            this.addAction(
                metadata.route,
                action,
                controller.prototype[action]
            );
        }
    }

    private addAction(parent: string, action: string, method: Method) {
        const metadata: ActionMetadata = Reflect.getMetadata("action", action);
        if (!metadata) return;
        this.addMiddleware(parent + metadata.route, action);
        this.addActionUsingAppropriateMethod(parent, metadata, method);
    }

    private addMiddleware(route: string, action: string) {
        const metadata: Array<Method> = Reflect.getMetadata(
            "middleware",
            action
        );
        if (!metadata) return;
        metadata.forEach((middleware) => this.router.use(route, middleware));
    }

    private addActionUsingAppropriateMethod(
        parent: string,
        { type, route }: ActionMetadata,
        method: Method
    ) {
        route = parent + route;
        switch (type) {
            case ActionType.GET:
                this.router.get(route, method);
                break;
            case ActionType.POST:
                this.router.post(route, method);
                break;
            case ActionType.PUT:
                this.router.put(route, method);
                break;
            case ActionType.DELETE:
                this.router.delete(route, method);
                break;
            case ActionType.PATCH:
                this.router.patch(route, method);
                break;
            case ActionType.ALL:
                this.router.all(route, method);
                break;
        }
    }
}

router.decorators.ts

export const controller = (route: string): Function => {
    return (constructor: Function) =>
        Reflect.defineMetadata("controller", route, constructor);
};

export const action = (type: ActionType, route: string): Function => {
    return (target: any, key: string, descriptor: PropertyDescriptor) =>
        Reflect.defineMetadata(
            "action",
            { type, route },
            descriptor.value,
            key
        );
};

export const middleware = (functions: Array<Function>) => {
    return (target: any, key: string, descriptor: PropertyDescriptor) =>
        Reflect.defineMetadata("middleware", functions, descriptor.value, key);
};

A lot has changed since this comment, see issue #781, and the following commit.

[brarsanmol]

There are currently some security issues that exist with the account endpoints.

Here is a list:

• POST /account/

  • Able to update the account type.
    • We should be using the default hacker value unless it's an invite (we have not implemented this feature back yet).
  • Able to update the password field.

• PATCH /account/

  • Able to update the account type.
  • Able to update the password field.
  • Able to update the e-mail field.

My initial thoughts to fix this are...

• DTO Pattern (UserUpdateDto)
  • Essentially just replaces the validator pattern.
• Utility Types
  • Already implemented on some endpoints:
    • Example: Omit<Partial<User>, 'password', 'email'>