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

@param.path generated with lb4 relation considers Wrapper datatypes #3711

Closed
basavarajvs opened this issue Sep 11, 2019 · 11 comments
Closed

Comments

@basavarajvs
Copy link

basavarajvs commented Sep 11, 2019

Steps to reproduce

  1. Create 2 related DB tables with table ID as a varchar or uuid
  2. Discover the DB tables with lb4 discover
  3. Create repository for both the tables ( lb4 repository)
  4. run the command "lb4 relation"

Current Behavior

The controller files are generated with methods which have reference to String wrapper under the @param.path

async find(
    @param.path.**_String_**('id') id: String,
    @param.query.object('filter') filter?: Filter<Rolesinsaas>,
  ): Promise<Rolesinsaas[]> {
    return this.accountRepository.rolesinsaas(id).find(filter);
  }

This is causing an error to be thrown on npm start

> src/controllers/account-rolesinsaas.controller.ts:42:17 - error TS2551: Property 'String' does not exist on type '{ string: (name: string, spec?: Partial<ParameterObject> | undefined) => (target: object, member: string, index: number) => void; number: (name: string, spec?: Partial<ParameterObject> | undefined) => (target: object, member: string, index: number) => void; ... 9 more ...; password: (name: string, spec?: Partial<......'. Did you mean 'string'?
> 
> 42     @param.path.String('id') id: String,

Also the parameter.decorator.d.ts file does not contain a reference to the String wrapper data type.

Expected Behavior

lb4 relations should generate @parameter.path.<type> using only the supported data types and not wrapper classes.
or
parameter.decorator.d.ts file should support wrapper data types for String, Number

Link to reproduction sandbox

Additional information

lb4 --version
@loopback/cli version: **1.21.6**

@loopback/* dependencies:
  - @loopback/authentication: ^3.0.0
  - @loopback/boot: ^1.5.5
  - @loopback/build: ^2.0.10
  - @loopback/context: ^1.22.1
  - @loopback/core: ^1.10.1
  - @loopback/metadata: ^1.3.1
  - @loopback/openapi-spec-builder: ^1.2.12
  - @loopback/openapi-v3: ^1.9.6
  - @loopback/repository-json-schema: ^1.9.7
  - @loopback/repository: ^1.13.1
  - @loopback/rest: ^1.18.1
  - @loopback/testlab: ^1.8.0
  - @loopback/docs: ^2.0.0
  - @loopback/example-hello-world: ^1.2.13
  - @loopback/example-log-extension: ^1.2.13
  - @loopback/example-rpc-server: ^1.2.13
  - @loopback/example-todo: ^1.7.6
  - @loopback/example-soap-calculator: ^1.6.14
  - @loopback/service-proxy: ^1.3.5
  - @loopback/http-caching-proxy: ^1.1.12
  - @loopback/http-server: ^1.4.12
  - @loopback/example-todo-list: ^1.9.6
  - @loopback/dist-util: ^0.4.0
  - @loopback/rest-explorer: ^1.3.6
  - @loopback/eslint-config: ^4.0.2
  - @loopback/example-express-composition: ^1.5.6
  - @loopback/example-greeter-extension: ^1.3.13
  - @loopback/booter-lb3app: ^1.2.13
  - @loopback/example-lb3-application: ^1.1.13
  - @loopback/example-greeting-app: ^1.1.13
  - @loopback/example-context: ^1.2.13
  - @loopback/repository-tests: ^0.4.4
  - @loopback/extension-health: ^0.2.5
  - @loopback/authorization: ^0.2.2
  - @loopback/rest-crud: ^0.2.0
  - @loopback/security: ^0.1.1

Related Issues

See Reporting Issues for more tips on writing good issues

@dhmlau
Copy link
Member

dhmlau commented Sep 11, 2019

@basavarajvs, what's your id property look like in your model class?
Something similar to:

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

@dhmlau dhmlau self-assigned this Sep 11, 2019
@basavarajvs
Copy link
Author

basavarajvs commented Sep 11, 2019

@dhmlau dhmlau
The Model looks as below

  @property({
    type: String,
    required: true,
    id: true,
    postgresql: {"columnName": "id", "dataType": "uuid", "dataLength": null, "dataPrecision": null, "dataScale": null, "nullable": "NO"},
  })
  id: String;

The generated controller looks like this

  @get('/rolesinsaas/{id}/account', {
    responses: {
      '200': {
        description: 'Account belonging to Rolesinsaas',
        content: {
          'application/json': {
            schema: { type: 'array', items: getModelSchemaRef(Account) },
          },
        },
      },
    },
  })
  async getAccount(
    @param.path.String('id') id: typeof Rolesinsaas.prototype.accntid,
  ): Promise<Account> {
    return this.rolesinsaasRepository.account(id);
  }

The associated repository looks like this

export class RolesinsaasRepository extends DefaultCrudRepository<
  Rolesinsaas,
  typeof Rolesinsaas.prototype.id,
  RolesinsaasRelations
  > {

  public readonly account: BelongsToAccessor<Account, typeof Rolesinsaas.prototype.accntid>;

  constructor(
    @inject('datasources.postgres') dataSource: PostgresDataSource, @repository.getter('AccountRepository') protected accountRepositoryGetter: Getter<AccountRepository>,
  ) {
    super(Rolesinsaas, dataSource);
    this.account = this.createBelongsToAccessorFor('accountAccountname', accountRepositoryGetter);
  }
}

Please do let me know if you need additional info

@freakpol
Copy link

@param.path.String('id') id: typeof Rolesinsaas.prototype.accntid,

should be

@param.path.string('id') id: typeof Rolesinsaas.prototype.accntid,

(note the uppercase S vs the lowercase one)

@basavarajvs
Copy link
Author

@param.path.String('id') id: typeof Rolesinsaas.prototype.accntid,

should be

@param.path.string('id') id: typeof Rolesinsaas.prototype.accntid,

(note the uppercase S vs the lowercase one)

Yes thats the problem with Auto generated code.. on running "lb4 relation".
I did change it to
@param.path.string('id') id: typeof Rolesinsaas.prototype.accntid,
and proceeded further.
Changes are required for "lb4 relation" command.

@bajtos
Copy link
Member

bajtos commented Sep 27, 2019

I think this is a bug in the code generating LB4 property definitions from metadata provided by discovery (which is LB3 based). I encountered a similar problem while implementing lb4 import-lb3-models - see #3688. The solution is to convert built-in types like String to all-lowercase variant (string), see here:

https://github.com/strongloop/loopback-next/blob/ce1f4b7b2304f4988b9ce5165edda23251262f81/packages/cli/generators/import-lb3-models/migrate-model.js#L112-L113

I think we should find a way how to share more code between lb4 discover and lb4 import-lb3-models, so that this particular cleanup/sanitization is applied in both cases.

We are already sharing sanitizeProperty, see

https://github.com/strongloop/loopback-next/blob/ce1f4b7b2304f4988b9ce5165edda23251262f81/packages/cli/generators/import-lb3-models/migrate-model.js#L14

and

https://github.com/strongloop/loopback-next/blob/ce1f4b7b2304f4988b9ce5165edda23251262f81/packages/cli/generators/import-lb3-models/migrate-model.js#L103

Any volunteers to contribute the changes and fix the problem?

See Contribute to LoopBack 4 and Submitting a pull request to LoopBack 4 to get started.

@bajtos
Copy link
Member

bajtos commented Sep 27, 2019

Duplicate of #3806

@dhmlau
Copy link
Member

dhmlau commented Oct 9, 2019

@bajtos, just want to make sure.. if there is a fix for this issue, the issue in #3806 will be fixed automatically?

@bajtos
Copy link
Member

bajtos commented Oct 17, 2019

I am not sure, we will need to double-check.

@dhmlau dhmlau added this to the Nov 2019 milestone milestone Nov 1, 2019
@dhmlau dhmlau removed their assignment Nov 14, 2019
@jannyHou jannyHou self-assigned this Nov 21, 2019
@jannyHou
Copy link
Contributor

Verified with the new merged fix for #3806 :
If the model definition is generated correctly, then lb4 relation generates the correct relation controller file, see my gist in https://gist.github.com/jannyHou/d27d7ff2aaa4f184421e1afc57579c8b

@jannyHou
Copy link
Contributor

jannyHou commented Nov 21, 2019

So the related code is the Typescript type:

@property({
    // The type here doesn't affect
    type: 'number',
    id: true,
  })
  // The ts type specified for the property affects the generated controller file
  // if you put `id?: Number` here, `lb4 relation` generates `@param.path.Number` which is invalid
  id?: number;

I believe we can close this story as a dup of #3806. cc @bajtos WDYT?
Number, String, ...etc are invalid usage as primitive types, see official explanation in https://www.typescriptlang.org/docs/handbook/declaration-files/do-s-and-don-ts.html#general-types, therefore we shouldn't support signature like id?: Number when id is in primitive type number.

This issue should be fixed with a new release(coming soon).

@jannyHou
Copy link
Contributor

Fixed in the latest release of @loopback/cli@1.26.0, I am closing this issue 👏
Feel free to reopen it if you still run into the problem

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants