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

Endpoint not found when extending base controller #1713

Closed
cuzox opened this issue Sep 15, 2018 · 7 comments
Closed

Endpoint not found when extending base controller #1713

cuzox opened this issue Sep 15, 2018 · 7 comments
Assignees

Comments

@cuzox
Copy link

cuzox commented Sep 15, 2018

Description / Steps to reproduce / Feature proposal

I want to have a base controller from which my controllers would inherit from

import { Filter, Where, DefaultCrudRepository, Entity, } from '@loopback/repository';
import {
  post,
  param,
  get,
  patch,
  del,
  requestBody
} from '@loopback/rest';

type ID = number | string;

export class BaseController<M extends Entity, R extends DefaultCrudRepository<M, ID>> {
  constructor(
    public repository: R, public endpoint: string
  ) { }

  @post(`/${this.endpoint}`)
  async create(@requestBody() entity: M)
    : Promise<M> {
    return await this.repository.create(entity);
  }

  @get(`/${this.endpoint}/count`)
  async count(@param.query.string('where') where?: Where): Promise<number> {
    return await this.repository.count(where);
  }

  @get(`/${this.endpoint}`)
  async find(@param.query.string('filter') filter?: Filter)
    : Promise<M[]> {
    return await this.repository.find(filter);
  }

  @patch(`/${this.endpoint}`)
  async updateAll(
    @requestBody() entity: M,
    @param.query.string('where') where?: Where
  ): Promise<number> {
    return await this.repository.updateAll(entity, where);
  }

  @get(`/${this.endpoint}/{id}`)
  async findById(@param.path.string('id') id: ID): Promise<M> {
    return await this.repository.findById(id);
  }

  @patch(`/${this.endpoint}/{id}`)
  async updateById(
    @param.path.string('id') id: ID,
    @requestBody() entity: M
  ): Promise<boolean> {
    return await this.repository.updateById(id, entity);
  }

  @del(`/${this.endpoint}/{id}`)
  async deleteById(@param.path.string('id') id: ID): Promise<boolean> {
    return await this.repository.deleteById(id);
  }
}

and use it like so:

import { repository, Filter } from '@loopback/repository';
import { Cocina } from '../models';
import { CocinaRepository } from '../repositories';
import { BaseController } from '../base';
import {
  post,
  param,
  get,
  patch,
  del,
  requestBody
} from '@loopback/rest';

export class CocinaController extends BaseController<Cocina, CocinaRepository> {
  constructor(
    @repository(CocinaRepository)
    public cocinaRepository: CocinaRepository,
  ) {
    super(cocinaRepository, 'cocinas')
  }
}

Current Behavior

NotFoundError 404 Endpoint "GET /XXX" not found

Expected Behavior

200 OK

@cuzox cuzox changed the title NotFoundError 404 Endpoint "GET /XXX" not found Endpoint not found when extending base controller Sep 15, 2018
@raymondfeng
Copy link
Contributor

Please note that decorator functions are called at module level outside of the class. You can find the transpiled code at dist*/. As a result, this.endpoint is not what you had expected. I think a mixin may make more sense.

@cuzox
Copy link
Author

cuzox commented Sep 15, 2018

@raymondfeng thank you for your prompt response, I didn't want to have to ask this here since it's more typescript than lb, but I can't really figure out how to properly implement the mixin.

now I have

import { Filter, Where, DefaultCrudRepository, Entity, } from '@loopback/repository';
import {
  post,
  param,
  get,
  patch,
  del,
  requestBody
} from '@loopback/rest';

type ID = number | string;
type Constructor<T = {}> = new (...args: any[]) => T;

export function BaseController<TBase extends Constructor, M extends Entity, R extends DefaultCrudRepository<M, ID>>(
  Base: TBase,
  endpoint: string
) {
  class BaseControllerClass extends Base {
    repository: R;

    @post(`/${endpoint}`)
    async create(@requestBody() entity: M)
      : Promise<M> {
      return await this.repository.create(entity);
    }

    @get(`/${endpoint}/count`)
    async count(@param.query.string('where') where?: Where): Promise<number> {
      return await this.repository.count(where);
    }

    @get(`/${endpoint}`)
    async find(@param.query.string('filter') filter?: Filter)
      : Promise<M[]> {
      return await this.repository.find(filter);
    }

    @patch(`/${endpoint}`)
    async updateAll(
      @requestBody() entity: M,
      @param.query.string('where') where?: Where
    ): Promise<number> {
      return await this.repository.updateAll(entity, where);
    }

    @get(`/${endpoint}/{id}`)
    async findById(@param.path.string('id') id: ID): Promise<M> {
      return await this.repository.findById(id);
    }

    @patch(`/${this.endpoint}/{id}`)
    async updateById(
      @param.path.string('id') id: ID,
      @requestBody() entity: M
    ): Promise<boolean> {
      return await this.repository.updateById(id, entity);
    }

    @del(`/${endpoint}/{id}`)
    async deleteById(@param.path.string('id') id: ID): Promise<boolean> {
      return await this.repository.deleteById(id);
    }
  }
  return BaseControllerClass
};

and

import { Filter, Where, repository } from '@loopback/repository';
import {
  post,
  param,
  get,
  patch,
  del,
  requestBody
} from '@loopback/rest';
import { Greca } from '../models';
import { GrecaRepository } from '../repositories';
import { BaseController } from '../base';

class GrecaController {
  constructor(
    @repository(GrecaRepository)
    public repository: GrecaRepository,
  ) { }
}

export const WithBaseController = BaseController<GrecaController, Greca, GrecaRepository>(GrecaController, 'grecas')

but it says:
Type 'GrecaController' does not satisfy the constraint 'Constructor<{}>'.
Type 'GrecaController' provides no match for the signature 'new (...args: any[]): {}'.

would this be the right approach?
I'm a little doubtful of

 BaseController<TBase extends Constructor, M extends Entity, R extends DefaultCrudRepository<M, ID>>

is this where I would define my other needed generics?
it seems that the problem might have more to do with the Contructor type I defined...
maybe you can point me to a place where you guys already do something similar?
I'm just really out of ideas, any help would be greatly appreciated

@raymondfeng
Copy link
Contributor

raymondfeng commented Sep 15, 2018

You should have something like:

class GrecaController extends BaseControllerMixin(...) {
  constructor(
    @repository(GrecaRepository)
    public grecaRepository: GrecaRepository,
  ) {
    super(...);
  }
}

See an example at https://github.com/strongloop/loopback-next/blob/master/examples/todo-list/src/application.ts#L12

Please note that your mixin is a function that generates a base class for extending.

@raymondfeng
Copy link
Contributor

raymondfeng commented Sep 15, 2018

For your use case, I think a better way can be adding basePath support for controller classes. This way, your base class can use relative paths for operations and the basePath can be overridden by subclasses.

See #740.

Do you think we should move forward with #740?

@cuzox
Copy link
Author

cuzox commented Sep 16, 2018

What an interesting read! It was particularly interesting to see the reason behind some design decisions I did not quite understand. Like why do we need to manually create a repository to bind the model to the datasource.

What makes LB3 special, is that you don't have to be exposed to a lot of code to get the simple things done, as opposed to Express (which I used before LB3).
When I initially went into the LB4 docs, I began to notice the similarities with .Net. I was showing a .Net Core friend around the examples, he pointed out that the implementations were uncanny. It did not particularly stand out to him because it took quite a bit to get from model definition to REST interaction, and it did not seem worth the switch from the familiar .Net (although he was all about it after he saw the where filter integration).

But I digress, IMHO, it makes much more sense to share a common base controller, where I could configure/enforce common behavior once, and have the ability to override particular methods depending on my needs.

Do you think we should move forward with #740?

Absolutely!

@raymondfeng
Copy link
Contributor

raymondfeng commented Sep 16, 2018

@cuzox Thank you for the feedback. My personal preference is to make simple things simple and keep it extensible to do more complex things. I would like to keep some of the by convention experience we have in LB3, to be particular:

  1. It should be possible to declare or discover repositories by associating a model to a datasource (like LB3 model).
  2. It should be easy to expose repository interfaces over REST by convention.

The tricky thing is how to do enforce/leverage TypeScript checks for these magic artifacts.

@cuzox
Copy link
Author

cuzox commented Sep 17, 2018

I can see how typings can make things easier, while presenting a whole new class of challenges.
I was able to implement the base controller mixin, but I was unable to make the returned class extend the superclass.

BaseControllerMixin

import { Filter, Where, DefaultCrudRepository, Entity, } from '@loopback/repository';

import {
  post,
  param,
  get,
  patch,
  del,
  requestBody
} from '@loopback/rest';

type ID = number | string;
type Class = { new(...args: any[]): any; };

export function BaseControllerMixin<M extends Entity>(endpoint: string): Class {
  class BaseControllerClass {
    constructor(public repository: DefaultCrudRepository<M, ID>) { }

    @post(`/${endpoint}`)
    async create(
      @requestBody() entity: M
    ): Promise<M> {
      return await this.repository.create(entity);
    }

    @get(`/${endpoint}/count`)
    async count(@param.query.string('where') where?: Where): Promise<number> {
      return await this.repository.count(where);
    }

    @get(`/${endpoint}`)
    async find(
      @param.query.string('filter') filter?: Filter
    ): Promise<M[]> {
      return await this.repository.find(filter);
    }

    @patch(`/${endpoint}`)
    async updateAll(
      @requestBody() entity: M,
      @param.query.string('where') where?: Where
    ): Promise<number> {
      return await this.repository.updateAll(entity, where);
    }

    @get(`/${endpoint}/{id}`)
    async findById(
      @param.path.string('id') id: ID
    ): Promise<M> {
      return await this.repository.findById(id);
    }

    @patch(`/${endpoint}/{id}`)
    async updateById(
      @param.path.string('id') id: ID,
      @requestBody() entity: M
    ): Promise<boolean> {
      return await this.repository.updateById(id, entity);
    }

    @del(`/${endpoint}/{id}`)
    async deleteById(
      @param.path.string('id') id: ID
    ): Promise<boolean> {
      return await this.repository.deleteById(id);
    }
  }
  return BaseControllerClass
};

Usage

import { Filter, Where, repository } from '@loopback/repository';
import {
  post,
  param,
  get,
  patch,
  del,
  requestBody
} from '@loopback/rest';
import { Entidad } from '../models';
import { EntidadRepository } from '../repositories';
import { BaseControllerMixin } from '../base';

export class EntidadController extends BaseControllerMixin<Entidad>('entidades') {
  constructor(
    @repository(EntidadRepository)
    public repository: EntidadRepository,
  ) {
    super(repository)
  }
}

Thank you for your help @raymondfeng, I hope your PR gets into the release version, so that what made LB3 great, continues to live on.

@cuzox cuzox closed this as completed Sep 17, 2018
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

No branches or pull requests

2 participants