loopbackio / loopback-next

LoopBack makes it easy to build modern API applications that require complex integrations.
https://loopback.io
Other
4.93k stars 1.06k forks source link

[CLI] Prevent usage of already used keywords #2052

Open Yaty opened 5 years ago

Yaty commented 5 years ago

Description / Steps to reproduce / Feature proposal

When using the CLI I can create a model named Entity, or Model. The model is generated but no warning will be emitted (this keyword is already used by Loopback).

Current Behavior

The model/controller/datasource/... is generated.

import {Model, model, property} from '@loopback/repository';

@model()
export class Model extends Model {
  constructor(data?: Partial<Model>) {
    super(data);
  }
}

Expected Behavior

Multiple options here:

  1. add warnings in the creation process
  2. prevent the component (model/controller/...) from being created
    1. by retrying
    2. by exiting
  3. modify the generated file , for a model e.g:
import * as repository from '@loopback/repository';

@repository.model()
export class Model extends repository.Model {
  constructor(data?: Partial<Model>) {
    super(data);
  }
}

Keywords to consider

For controllers: nothing (overwriting existing controllers is already handled)

For datasources: nothing (overwriting existing datasources is already handled)

For models:

For repositories:

For services: ?

For openapi: nothing

Anything else ?

Acceptance criteria

Add a Yeoman prompt validator to reject model/repository/controller names that are the same as one of the built-in ones. This way, when a user enters invalid name, Yeoman will print an error and repeat the prompt.

For each of the following generators, provide validation rule that will reject problematic names:


πŸŽ† Hacktoberfest 2020

Greetings :wave: to all Hacktoberfest 2020 participants!

Here are few tips πŸ‘€ to make your start easier, see also #6456:

bajtos commented 5 years ago

Personally, I would prefer the following option:

Ideally, we should check not only for built-in names like Model/Entity, but also for entities already defined in the project.

@Yaty would you like to contribute these improvements?

Yaty commented 5 years ago

I'm on it ! This option looks good to me.

erogleva commented 3 years ago

I'd like to work on this

bajtos commented 3 years ago

@erogleva awesome, let us know if you need any help to get started!

erogleva commented 3 years ago

It would be good for me to summarize the keywords which should be rejected for the different generators

For lb4 model -This seems clear and I've started implementing it (reject the names Model and Entity as well as all names of models already defined in the project)

For lb4 repository - Repository? But If you actually specify this keyword the generator will create a RepositoryRepository class which is weird but technically not an issue :smile: It would be an issue though if something like DefaultCrud is entered :smile:

For lb4 observer - LifeCycle.

For lb4 relation - It seems that creating a relation with the same foreign key name as a field which is already defined on the model removes the old definition and leaves only the new relation. If this is the desired behavior, perhaps I could add a hint that the current field will be overridden.

For lb4 controller, lb4 service and lb4 interceptor - Overriding existing controllers and services is already handled (as pointed out above). So are there any other problematic names here?

bajtos commented 3 years ago

Thank you @erogleva for the summary. I don't have detailed-enough knowledge, but your conclusions looks reasonable to me πŸ‘πŸ»

For lb4 repository - Repository? But If you actually specify this keyword the generator will create a RepositoryRepository class which is weird but technically not an issue πŸ˜„ It would be an issue though if something like DefaultCrud is entered πŸ˜„

I think that means we should reject DefaultCrud as a user-provided name. Probably also KeyValue?

For lb4 relation - It seems that creating a relation with the same foreign key name as a field which is already defined on the model removes the old definition and leaves only the new relation. If this is the desired behavior, perhaps I could add a hint that the current field will be overridden.

@agnes512 I think you are most knowledgeable about relations, what's your opinion on the desired behavior of lb4 relation?

For lb4 controller, lb4 service and lb4 interceptor - Overriding existing controllers and services is already handled (as pointed out above). So are there any other problematic names here?

To be honest, I don't know. It's possible there are no "reserved" names to avoid. Maybe we can ignore these three CLI commands for now, and wait to see if there are any bug reports coming from our users?

agnes512 commented 3 years ago

@erogleva thanks for bring up the issue! I think for current lb4 relation, duplicate foreign keys or relation names would be rejected. Could you provide more details about the old definition getting removed? Cause it looks a bug to me. Thanks!

erogleva commented 3 years ago

I meant a case such as the following:

For example, if we have a model named Company with "address" as a string property

@model()
export class Company extends Entity {
  @property({
    type: 'string',
    required: true,
  })
  address: string;

  @property({
    type: 'string',
    id: true,
    generated: true,
  })
  id?: string;

  constructor(data?: Partial<Company>) {
    super(data);
  }
}

and then we also decide to create an "Address" model:

@model()
export class Address extends Entity {
  @property({
    type: 'string',
    id: true,
    generated: true,
  })
  id?: string;

  constructor(data?: Partial<Address>) {
    super(data);
  }
}

Using the generator to create a relation Company hasOne Address might look like this:

? Please select the relation type hasOne ? Please select source model Company ? Please select target model Address ? Foreign key name to define on the target model companyId ? Source property name for the relation getter (will be the relation name) address

So as a result of the generation the source property name for the relation becomes address and this overrides the current string property:

@model()
export class Company extends Entity {
  @property({
    type: 'string',
    id: true,
    generated: true,
  })
  id?: string;

  @hasOne(() => Address)
  address: Address;

  constructor(data?: Partial<Company>) {
    super(data);
  }
}