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

postgres connector not returning generated identifier value on create of entity for camelCased identifiers in table #3749

Closed
basavarajvs opened this issue Sep 16, 2019 · 8 comments
Assignees
Labels

Comments

@basavarajvs
Copy link

basavarajvs commented Sep 16, 2019

If a Postgres table has camelCased identifier then auto-generated code of loopback does not return the identifier on creating the entity.

Steps to reproduce

  1. Create 2 tables in postgres with the identifier field with camel casing .
CREATE TABLE test."Customer" (
	"**customerId**" uuid NOT NULL DEFAULT gen_random_uuid(),
	"name" varchar(50) NOT NULL,
	CONSTRAINT "Customer_pkey" PRIMARY KEY ("customerId")
);
CREATE UNIQUE INDEX "PK_Customer" ON "Customer" USING btree ("customerId");

-- Drop table

-- DROP TABLE test."Order";

CREATE TABLE test."Order" (
	id uuid NOT NULL DEFAULT gen_random_uuid(),
	"customerId" uuid NOT NULL,
	"name" varchar(50) NOT NULL,
	"isDelivered" bool NOT NULL,
	CONSTRAINT "Order_pkey" PRIMARY KEY (id)
);
CREATE UNIQUE INDEX "PK_Order" ON "Order" USING btree (id);
CREATE INDEX "fkIdx_431" ON "Order" USING btree ("customerId");

ALTER TABLE test."Order" ADD CONSTRAINT "FK_431" FOREIGN KEY ("customerId") REFERENCES "Customer"("customerId");

Please note that the identifier field for Cutomer table is customerId in camelCasing

  1. Run loopback discover to create the models.
    loopback discover --schema test

  2. Generate repository and controller.

  3. Run "npm start"

  4. In Swagger execute a post request for Customer.

Current Behavior

The Customer entity is created but the response does not have the identifier field populated.

Expected Behavior

On creating an entity with a camelCased identifier the id should be returned in the post request response.

The problem is not reproduced when the identifiers are lowercased.

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

@basavarajvs basavarajvs changed the title postgres connector using lower case column names for table identifiers and hence not returning identifier value on create of entity postgres connector not returning generated identifier value on create of entity for camelCased identifiers in table Sep 16, 2019
@dhmlau
Copy link
Member

dhmlau commented Sep 18, 2019

@agnes512, I believe you've looked into similar topic before. Could you PTAL? Thanks.

@agnes512
Copy link
Contributor

agnes512 commented Sep 19, 2019

Hi @basavarajvs , I just reproduced the issue.

The Customer entity is created but the response does not have the identifier field populated.
The problem is not reproduced when the identifiers are lowercased.

Have you checked the mode Customer and Order in src/models/? I think the issue is caused by the way LB4 converts column names from database to model properties. The naming convention we are using currently is lowercase.
If you check these two model files, those property names should be generated as lowercase. If you change them from customerid to-> customerId , isdelivered -> isDelivered, it should work as expected.

We actually have been seeing this issue for years (loopbackio/loopback-connector-mysql#57). Sorry that we don't have more naming options available for discover and migration at the moment.
We don't consider it as a bug for now as the reason is lack of naming convention options. The functionality itself is working.
Our team had some discussion about it in the past. We admit that it should be improved. But the priority of this issue is relatively low.

Please let me know if the workaround works, and if you need more help. Thank you!

@bajtos
Copy link
Member

bajtos commented Sep 27, 2019

I think this may be a bug in lb4 discover. I vaguely remember that in LB3, properties imported from SQL schema used to have connector-specific configuration, for example:

{
  "name": "Inventory",
  "properties": {
     "productId": {
       "length": 20,
       "memory": {
        "columnName": "PRODUCT_ID",
        "dataLength": 20,
        "dataPrecision": null,
        "dataScale": null,
        "dataType": "varchar",
        "nullable": 0,
      },    
    },
  },
}

If you take a closer look, you'll see that the LB4 property name productId is different from the database column name PRODUCT_ID and yet everything should work as expected.

IMO, lb4 discover should preserve connector-specific property metadata in the generated output, and include the correct columnName field to ensure the values are correctly mapped between LB4 property name (that's generated as lower-case) and the actual database column.

@agnes512 can you please double check that lb4 discover is not accidentally discarding the connector-specific metadata from property definitions?

@agnes512
Copy link
Contributor

agnes512 commented Sep 27, 2019

@bajtos I think the generated columnName is correct, here is my psql table Order:

   Column    |         Type          | Collation | Nullable | Default 
-------------+-----------------------+-----------+----------+---------
 id          | uuid                  |           | not null | 
 customerId  | uuid                  |           | not null | 
 name        | character varying(20) |           | not null | 
 isDelivered | boolean               |           | not null | 

And the Order model generated by lb4 discover:

@model({settings: {idInjection: false, postgresql: {schema: 'public', table: 'Order'}}})
export class Order extends Entity {
  @property({
    type: String,
    required: true,
    id: 1,
    postgresql: {"columnName":"id","dataType":"uuid",..},
  })
  id: String;

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

// name property

  @property({
    type: Boolean,
    required: true,
    postgresql: {
      "columnName":"isDelivered",
      "dataType":"boolean",
      "dataLength":null,
      "dataPrecision":null,
      "dataScale":null,
      "nullable":"NO"},
  })
  isdelivered: Boolean;
...
}

I think it does preserve connector-specific property metadata. And LB4 property names are generated in lower-case.
I think users are expecting the generated model property names are the same as db column names. So they got errors when they try to use the db column names to send requests.
Please correct me if my understanding is wrong.

@bajtos bajtos added the db:PostgreSQL Topics specific to PostgreSQL label Sep 30, 2019
@bajtos
Copy link
Member

bajtos commented Sep 30, 2019

@agnes512 thank you for checking!

And the Order model generated by lb4 discover:
(...)
I think it does preserve connector-specific property metadata.

Great! This is an important part, it's good to know that it's working.

And LB4 property names are generated in lower-case.
I think users are expecting the generated model property names are the same as db column names.

I see two problems:

(1)
I agree that ideally, generated property names should be camelCase, not all-lower-case. To me, this is a nice-to-have feature. (I suspect the code converting column names to property names is expecting snake-case or underscore_case names, but is not prepared to deal with camelCase names.)

(2)
What I find more troubling is the behavior described in the issue description at the top:

  • Run "npm start"
  • In Swagger execute a post request for Customer.
  • Current Behavior: The Customer entity is created but the response does not have the identifier field populated.

This looks like a bug to me. My expectation is that the discovery mechanism should produce code that works out of the box with the existing database. That's why I was asking about connector-specific metadata - if it was not present, the the connector would not know that customerid in LB data is represented as customerId in the database.

@dhmlau
Copy link
Member

dhmlau commented Oct 15, 2019

@agnes512, could you please add the acceptance criteria? Thanks.

@agnes512
Copy link
Contributor

@bajtos @basavarajvs I doubly checked it. I still couldn't reproduce with LB4 and API Explorer.

Here's the response I got from the API Explorer:
Screen Shot 2019-10-16 at 10 59 52 AM

and the datasource:

image

The postgres connector seems working fine to me. I am not sure if the issue is caused by Swagger.

@bajtos
Copy link
Member

bajtos commented Nov 19, 2019

I agree that ideally, generated property names should be camelCase, not all-lower-case. To me, this is a nice-to-have feature. (I suspect the code converting column names to property names is expecting snake-case or underscore_case names, but is not prepared to deal with camelCase names.)

This feature is tracked by #3343

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

No branches or pull requests

4 participants