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

ID automatically generated by PostgreSQL becomes undefined #4751

Closed
3 tasks done
mi-tsu-ha-ru opened this issue Feb 26, 2020 · 8 comments
Closed
3 tasks done

ID automatically generated by PostgreSQL becomes undefined #4751

mi-tsu-ha-ru opened this issue Feb 26, 2020 · 8 comments
Assignees
Labels
bug db:PostgreSQL Topics specific to PostgreSQL Model Discovery Issues related to discovery of model definitions from (SQL) databases
Milestone

Comments

@mi-tsu-ha-ru
Copy link

mi-tsu-ha-ru commented Feb 26, 2020

A situation similar to #3749

Steps to reproduce

  1. lb4 datasource
  2. lb4 discover
  3. lb4 repository
? Please select the datasource TestDbDatasource
? Select the model(s) you want to generate a repository TestTable
? Please select the repository base class DefaultCrudRepository (Legacy juggler bridge)
  1. lb4 controller
? Controller class name: Test
Controller Test will be created in src/controllers/test.controller.ts

? What kind of controller would you like to generate? REST Controller with CRUD functions
? What is the name of the model to use with this CRUD repository? TestTable
? What is the name of your CRUD repository? TestTableRepository
? What is the name of ID property? testId
? What is the type of your ID? number
? Is the id omitted when creating a new instance? Yes
? What is the base HTTP path name of the CRUD operations? /test-tables
  1. Modify model
@model({
  settings: {
    idInjection: false,
    postgresql: {schema: 'public', table: 'testTable'},
  },
})
export class TestTable extends Entity {
  @property({
    type: 'number',
    required: false, // true -> false
    scale: 0,
    id: 1,
    postgresql: {
      columnName: 'testId',
      dataType: 'bigint',
      dataLength: null,
      dataPrecision: null,
      dataScale: 0,
      nullable: 'NO',
    },
  })
  testId: number;

  @property({
    type: 'string',
    required: true,
    length: 100,
    postgresql: {
      columnName: 'testContent',
      dataType: 'character varying',
      dataLength: 100,
      dataPrecision: null,
      dataScale: null,
      nullable: 'NO',
    },
  })
  testContent: string;

  // Define well-known properties here

  // Indexer property to allow additional data
  // eslint-disable-next-line @typescript-eslint/no-explicit-any
  [prop: string]: any;

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

Current Behavior

Do the following

const test = await this.testTableRepository.create({testContent: "test"});
console.log(test);

Result

TestTable { testId: undefined, testContent: 'test' }

The instance is being created in the database, but it somehow failed to map to the property.

Expected Behavior

I want it to look like the following

TestTable { testId: 1, testContent: 'test' }

Additional information

The following is a guess

I think the problem is that it is toLowerCase() in the function here.
https://github.com/strongloop/loopback-connector-postgresql/blob/v3.9.0/lib/postgresql.js#L369

After rewriting as below, it worked.

PostgreSQL.prototype.dbName = function(name) {
  if (!name) {
    return name;
  }
  // PostgreSQL default to lowercase names
  // return name.toLowerCase();
  return name;
};
  1. PostgreSQL.prototype.getInsertedId
    https://github.com/strongloop/loopback-connector-postgresql/blob/v3.9.0/lib/postgresql.js#L553

  2. SQLConnector.prototype.idColumn
    https://github.com/strongloop/loopback-connector/blob/v4.4.0/lib/sql.js#L423

  3. PostgreSQL.prototype.dbName
    https://github.com/strongloop/loopback-connector-postgresql/blob/v3.9.0/lib/postgresql.js#L369

lb4 --version

lb4 --version
@loopback/cli version: 1.30.1

@loopback/* dependencies:
  - @loopback/authentication: ^3.3.3
  - @loopback/boot: ^1.7.4
  - @loopback/build: ^3.1.1
  - @loopback/context: ^2.1.1
  - @loopback/core: ^1.12.4
  - @loopback/metadata: ^1.4.1
  - @loopback/openapi-spec-builder: ^1.3.1
  - @loopback/openapi-v3: ^2.0.0
  - @loopback/repository-json-schema: ^1.12.2
  - @loopback/repository: ^1.19.1
  - @loopback/rest: ^2.0.0
  - @loopback/testlab: ^1.10.3
  - @loopback/docs: ^2.11.0
  - @loopback/example-hello-world: ^1.2.25
  - @loopback/example-log-extension: ^1.2.25
  - @loopback/example-rpc-server: ^1.2.25
  - @loopback/example-todo: ^2.0.0
  - @loopback/example-soap-calculator: ^1.7.7
  - @loopback/service-proxy: ^1.3.17
  - @loopback/http-caching-proxy: ^1.3.0
  - @loopback/http-server: ^1.5.4
  - @loopback/example-todo-list: ^2.0.0
  - @loopback/dist-util: ^0.4.0
  - @loopback/rest-explorer: ^1.4.10
  - @loopback/eslint-config: ^5.0.3
  - @loopback/example-express-composition: ^1.10.4
  - @loopback/example-greeter-extension: ^1.3.25
  - @loopback/booter-lb3app: ^1.3.12
  - @loopback/example-lb3-application: ^1.1.26
  - @loopback/example-greeting-app: ^1.2.12
  - @loopback/example-context: ^1.3.2
  - @loopback/repository-tests: ^0.10.1
  - @loopback/extension-health: ^0.2.17
  - @loopback/authorization: ^0.4.10
  - @loopback/rest-crud: ^0.6.6
  - @loopback/security: ^0.1.13
  - @loopback/authentication-passport: ^1.1.3
  - @loopback/example-metrics-prometheus: ^0.1.7
  - @loopback/extension-metrics: ^0.1.6
  - @loopback/model-api-builder: ^1.1.4
  - @loopback/extension-logging: ^0.1.0

Related Issues

Acceptance Criteria

  • if the property has defined its column name, Postgres shouldn't use the default naming convention (lowercase), should keep as it is
  • besides Postgres, Oracle has the same issue ( uppercase as default case)
  • add tests for discover
@achrinza achrinza added the db:PostgreSQL Topics specific to PostgreSQL label Feb 26, 2020
@agnes512 agnes512 self-assigned this Feb 26, 2020
@agnes512 agnes512 added the Model Discovery Issues related to discovery of model definitions from (SQL) databases label Feb 26, 2020
@agnes512
Copy link
Contributor

@raw-ham Hi, the model definition looks legit to me. I think the problem is caused by id auto-generation. Could you check what's your default value for the identifier of table testTable with query:

SELECT column_name, column_default, data_type FROM information_schema.columns WHERE table_schema = 'public' and table_name = 'testTable';

If the id property in your table is not autogenerated (e.g default value of identifier is SERIAL), the result of the operation

const test = await this.testTableRepository.create({testContent: "test"});

will be testId: undefined.

There are three ways to fix it:
1.

  • set required to true and include value of id in POST request.
  • if you use the controller, the setting needs to be modified:
? Controller class name: Test
Controller Test will be created in src/controllers/test.controller.ts

? What kind of controller would you like to generate? REST Controller with CRUD functions
? What is the name of the model to use with this CRUD repository? TestTable
? What is the name of your CRUD repository? TestTableRepository
? What is the name of ID property? testId
? What is the type of your ID? number
? Is the id omitted when creating a new instance? No // this should be no instead
? What is the base HTTP path name of the CRUD operations? /test-tables

ALTER the testTable to set default value of the identity column to SERIAL

Define your model first, then use npm run migration ( notice that migration and discover are different):

@model()
export class testTable extends Entity {
  @property({
    type: 'number',
    id: true,
    required: false // not necessary, but it cannot be true
    generated: true, // this will set the default value of the identifier for you
    postgresql: {  // this setting allows you to have different names for model and db column
      columnName: 'testid', // this needs to be in lowercase
      dataType: 'integer',
      dataLength: null,
      dataPrecision: null,
      dataScale: 0,
      nullable: 'NO',
    },
  })
  testId?: number;

  // other props..

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

@mi-tsu-ha-ru
Copy link
Author

mi-tsu-ha-ru commented Feb 26, 2020

@agnes512 Thanks for your answer

Could you check what's your default value for the identifier of table testTable with query:

image

There are three ways to fix it:

I can't do either fix.
Because I uses an already running DB.

No problem occurs if the DB column name is lowercase or snake case.
(No problem when using lb4 discover)

I think the problem is that idColName becomes lowercase.
https://github.com/strongloop/loopback-connector-postgresql/blob/v3.9.0/lib/postgresql.js#L554

Or is there a restriction that ID columns can only be used in lowercase or snakecase?


I use Google Translate.
Sorry if it was difficult to read.

@agnes512
Copy link
Contributor

agnes512 commented Feb 26, 2020

@raw-ham I can reproduce the issue, and it only happens in Postgres. I think it's still because of the identifier issue. And weird thing is that the workaround doesn't work anymore D:

(workaround:

export class TestTable extends Entity {
  @property({
    type: 'number',
    required: false, 
    scale: 0,
    id: 1,
    postgresql: {
      columnName: 'testId',
      dataType: 'bigint',
      dataLength: null,
      dataPrecision: null,
      dataScale: 0,
      nullable: 'NO',
    },
  })
  testid: number; // in lowercase
...

)

The only way to make it work now is to set the identifier column name to lowercase for Postgres, which seems a unpleasant bug to me. issue #3343 is for column name mapping. It doesn't solve this specific issue in Postgres. I will discuss with the team and hopefully fix it soon. Thanks for reporting the bug!

@mi-tsu-ha-ru
Copy link
Author

@agnes512

The only way to make it work now is to set the identifier column name to lowercase for Postgres,

Yes, there is no other solution. So I am in trouble.

I will discuss with the team and hopefully fix it soon. Thanks for reporting the bug!

Thank you very much! Please comment if you need more information.

@agnes512
Copy link
Contributor

Debug note:
juggler-model.definition: ModelDefinition.prototype.idColumnName -(testId)> juggler-DataSource.prototype.idColumnName -(testId) > connector-SQLConnector.prototype.idColumn -( it calls dbName function of each connector)> postgres-connector-postgresql.js line 280 -> testid

@jannyHou
Copy link
Contributor

For code change in loopback-connector/sql/idColumnName(), it will affect other sql connectors, but if that function is overridden in the other connector, it's fine.

@agnes512
Copy link
Contributor

agnes512 commented Mar 4, 2020

@raw-ham a fix is released in loopback-connector@4.01.2, the identifier column should be testId
with definition:

  @property({
    type: 'number',
    required: false,
    scale: 0,
    id: 1,
    postgresql: {
      columnName: 'testId',
      dataType: 'bigint',
      dataLength: null,
      dataPrecision: null,
      dataScale: 0,
      nullable: 'NO',
    },
  })
  testId: number;

Could you try it out to see if it works on your end? thanks

@mi-tsu-ha-ru
Copy link
Author

@agnes512

I confirmed that it was solved in my project.

Thank you for correspondence!

@agnes512 agnes512 added this to the Mar 2020 milestone Mar 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug db:PostgreSQL Topics specific to PostgreSQL Model Discovery Issues related to discovery of model definitions from (SQL) databases
Projects
None yet
Development

No branches or pull requests

4 participants