feat: controller inheritance

[ivanpadavan]

This PR aims to fix #147.

First of all, thank you for amazing typestack! I've just started my exploration and got a feeling that I found a robust solution for full-stack typescript programming. This solves may be more concerns that I have at the moment, so huge thank you again. Making backend was never so nice and natural.

I found a problem that bothers me - inability to use inheritance without limitations & concerns.I've found it's unnatural and counterintuitive.

Solving this provides an opportunity to create complicated controllers (that simulates OData endpoint, for example) which could be reused easily. Or the definition of simple CRUD controller becomes a one-liner. Which is great, as for me. So solving this also opens doors for contribution on top of the stack.

Best Regards, sorry for my english.

[MichalLytek]

There is no tests cases for this new feature so we don't know if it really works. Secondly, you need to write some words about it in readme, so other users may use it too. And I don't know if we really need this feature as we discussed in #147.

[NoNameProvided]

What's up with this PR @19majkel94 and @ivanpadavan? Do we need this, is it well covered with tests, etc?

[enko]

Anything I can do, to move this pull request forward?

[NoNameProvided]

First, some word in the README and tests. When it has it we can see if it's works as intended or not.

But first it would be really nice if you could reason why you need this change. As in the linked issue is mentioned, neither of us (maintainers) convienced it's a thing we need. As @19majkel94 wrote: composition over inheritance is the way.

I just cannot see a real usecase for this when controller inheritance can save you a lot of time. Except if you write a very basic CRUD app without authorization logic or validation.

And the big deal here is the validation. You cannot have validation with inheritance, just only for your base entity. This reduce the valid use-case to deleting entities, when no payload or special query parameter is expected.

[enko]

@NoNameProvided I have thought about the issue and come to the conclusion that I do not want to use this feature, as I would have no way of disabling inherited routes. In my opinion it is better to explicitly state your desired routes in an explicit controller. I think I will create a base class as a construction kit, that can be used by the controller if needed, something like this:

https://gist.github.com/enko/26eff8ca7e7528a708d384a6eb39f597

So from my standpoint this would not be needed.

[github-actions]

Stale pull request message

[dmackenz]

I just wanted to comment saying that I have created a function similar to what this PR aims to achieve and I have found it extremely useful for reducing code duplication in my routing-controllers/typeorm servers.

With this function, you only need to define the entity that you want to operate CRUD operations on, and the rest is generically handled by routing-controllers.

Entity:

import { BaseEntity, Entity, PrimaryGeneratedColumn, Column } from "typeorm";

@Entity()
export class Payload extends BaseEntity {
  @PrimaryGeneratedColumn()
  id: string;

  @Column()
  num: number;
}

Create CRUD Controller function:

import { BaseEntity } from "typeorm";
import {
  JsonController,
  Get,
  Param,
  Post,
  Body,
  Put,
  Delete
} from "routing-controllers";

export interface IEntityClassBuilder<T extends BaseEntity> {
  new (): T;
  find(): Promise<T[]>;
  findOne(id: number): Promise<T>;
  create(entity: T): T;
  save(entity: T): Promise<T>;
  update(id: number, entity: T): Promise<any>;
  remove(entity: T): Promise<T>;
}

export function createCRUDController<T extends BaseEntity>(
  prefix: string,
  entityClass: IEntityClassBuilder<T>
): any {
  @JsonController(prefix)
  class Controller {
    @Get("/")
    getAll() {
      return entityClass.find();
    }

    @Get("/:id")
    get(@Param("id") id: number) {
      return entityClass.findOne(id);
    }

    @Post("/")
    post(@Body() body: T) {
      return entityClass.create(body).save();
    }

    @Put("/:id")
    async put(@Param("id") id: number, @Body() body: T): Promise<T> {
      await entityClass.update(id, body);
      return entityClass.findOne(id);
    }

    @Delete("/:id")
    async delete(@Param("id") id: number): Promise<T> {
      return entityClass.remove(await entityClass.findOne(id));
    }
  }

  return Controller;
}

Usage:

const app = createExpressServer({
  controllers: [createCRUDController("/payloads", Payload)]
});

This is just my own way of reducing the code duplication of things that I want to perform simple CRUD operations on. @ivanpadavan Is this what you were looking for?

[jotamorais]

@ivanpadavan @dmackenz @enko @alexey-pelykh @closertb @lixaotec @95gabor @pleerock @MichalLytek @jselesan @ssljivic
I'm reviewing some old PRs and trying to understand if it still makes sense before closing it.
In this specific one, I looked up the arguments in favor and against and I as @pleerock and @MichalLytek, I'm also not entirely convinced of the benefits versus the problems or overhead it could bring (hard to debug, hard/impossible to add validation, etc).
So, with that being said, I will wait a few more days to hear some final/eventual consideration, and if none, I will close this PR.

Thank you!

[ivanproskuryakov]

@jotamorais I would love to finalize this PR, add missing tests.

I also find controller inheritance very strong and important feature, it helps reduce duplicative(typical) CRUD operations (as described above) and makes the overall design more flexible.
Including supportive tests(unit/integrational).

My example:

import "reflect-metadata";
import {
  JsonController,
  UseBefore,
} from "routing-controllers";
import {ValidateJWT} from "../middleware/ValidateJWT";
import {ApiControllerTemplate} from "./ApiContollerTemplate";
import {ProductManager} from "../service/commerce/ProductManager";
import {ProductRepository} from "../repository/ProductRepository";

@UseBefore(ValidateJWT)
@JsonController("/product")
export class ProductController extends ApiControllerTemplate<Product> {
  constructor() {
    super();

    this.domain = "product";
    this.cloudManager = new ProductManager();
    this.localManager = new ProductRepository();
  }
}

import {getRepository} from "typeorm";
import {Body, Delete, Get, HttpCode, Param, Post, Put, Res} from "routing-controllers";

import {Authenticator} from "../service/Authenticator";
import {ProductRepository} from "../repository/ProductRepository";
import {CategoryRepository} from "../repository/CategoryRepository";
import {UserRepository} from "../repository/UserRepository";
import {CurrentUser} from "../decorator/CurrentUser";
import {User} from "../entity/User";
import {ProductManager} from "../service/commerce/ProductManager";
import {Product} from "../entity/Product";
import {CategoryManager} from "../service/commerce/CategoryManager";

export class ApiControllerTemplate<T> {
  public domain: string;
  public cloudManager: CategoryManager | ProductManager | any;
  public localManager: CategoryRepository | ProductRepository | any;

  protected authenticator: Authenticator;
  protected userRepository: UserRepository;

  constructor() {
    this.authenticator = new Authenticator();
    this.userRepository = new UserRepository();
  }

  @Post()
  @HttpCode(201)
  public async create(
    @CurrentUser() currentUser: User,
    @Body() payload: any,
    @Res() res: any
  ) {
    try {
      const cloud = await this.cloudManager.create(payload);
      const local = await this.localManager.create(currentUser, cloud.data.data);

      res.status(201);
      res.location(this.buildLocationUrl(local.id));

      return {};
    } catch (e) {
      res.status(e.response.status);

      return this.buildErrorView(e);
    }
  }

  @Get("/:id")
  public async get(
    @Param("id") id: number,
    @Res() res: any
  ) {
    try {
      const local = await getRepository(Product).findOneOrFail(id);
      const cloud = await this.cloudManager.get(local.id);

      return cloud.data.data;
    } catch (e) {
      res.status(e.response.status);

      return this.buildErrorView(e);
    }
  }

  @Delete("/:id")
  public async delete(
    @Param("id") id: number,
    @Res() res: any
  ) {
    try {
      await getRepository(Product).findOneOrFail(id);
      await this.cloudManager.delete(id);

      return {};
    } catch (e) {
      res.status(e.response.status);

      return this.buildErrorView(e);
    }
  }

  @Put("/:id")
  public async edit(
    @Param("id") id: number,
    @Body() payload: any,
    @Res() res: any
  ) {
    try {
      await getRepository(Product).findOneOrFail(id);
      await this.cloudManager.update(id, payload);
      res.status(204);

      return {};
    } catch (e) {
      res.status(e.status);

      return this.buildErrorView(e);
    }
  }

  protected buildLocationUrl(id: number): string {
    return `/api/${this.domain}/${id}`;
  }

  protected buildErrorView(e: any) {
    if (e.response.data) {
      return {
        title: e.response.data.title,
        errors: e.response.data.errors
      };
    }

    return {};
  }
}

https://en.wikipedia.org/wiki/Template_method_pattern

[jotamorais]

Since the original source repository of this repository no longer exists, I've merged this PR via a patch and it can be tested in the release candidate 0.9.0-alpha.1.
Please, leave feedback or open an issue if you experience unexcepted behavior.

[ivanproskuryakov]

@jotamorais Hi, thanks for merging the PR, templates, and inheritance will help much in my work.
Shall I cover this feature with unit-tests for better sustainability?

[jotamorais]

Yes, absolutely. I posted a comment in the the issue mentioning that this feature is still lacking test coverage, documentation and examples.
Appreciate if you can do that.

[ivanproskuryakov]

@jotamorais cool, btw thanks for your quick replies

[github-actions]

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