Loopback-next: Preserve custom type of auto-generated id property

[frbuceta]

Steps to reproduce

1. Create a model with an id property with the option generated: true
   
2. You make a find and the answer is ... uuid: NaN or id: NaN
   

Current Behavior

As I describe above, when the model is in generated: true it has to return the normal id, not NaN

Expected Behavior

Link to reproduction sandbox

When I put generated: false ...

Additional information

OS: MacOS 10.14.6
Node: v12.9.0
Npm: v6.11.2
Loopback: latest ->

@loopback/cli version: 1.21.4

@loopback/* dependencies:
  - @loopback/authentication: ^2.1.11
  - @loopback/boot: ^1.5.3
  - @loopback/build: ^2.0.8
  - @loopback/context: ^1.21.4
  - @loopback/core: ^1.9.3
  - @loopback/metadata: ^1.2.10
  - @loopback/openapi-spec-builder: ^1.2.10
  - @loopback/openapi-v3: ^1.9.4
  - @loopback/repository-json-schema: ^1.9.5
  - @loopback/repository: ^1.12.0
  - @loopback/rest: ^1.17.0
  - @loopback/testlab: ^1.7.4
  - @loopback/docs: ^1.29.3
  - @loopback/example-hello-world: ^1.2.11
  - @loopback/example-log-extension: ^1.2.11
  - @loopback/example-rpc-server: ^1.2.11
  - @loopback/example-todo: ^1.7.4
  - @loopback/example-soap-calculator: ^1.6.12
  - @loopback/service-proxy: ^1.3.3
  - @loopback/http-caching-proxy: ^1.1.10
  - @loopback/http-server: ^1.4.10
  - @loopback/example-todo-list: ^1.9.4
  - @loopback/dist-util: ^0.4.0
  - @loopback/rest-explorer: ^1.3.4
  - @loopback/eslint-config: ^4.0.1
  - @loopback/example-express-composition: ^1.5.4
  - @loopback/example-greeter-extension: ^1.3.11
  - @loopback/booter-lb3app: ^1.2.11
  - @loopback/example-lb3-application: ^1.1.11
  - @loopback/example-greeting-app: ^1.1.11
  - @loopback/example-context: ^1.2.11
  - @loopback/repository-tests: ^0.4.2
  - @loopback/extension-health: ^0.2.3
  - @loopback/authorization: ^0.2.0

Related Issues

_See Reporting Issues for more tips on writing good issues_

[dougal83]

What db connector are you using? Do you have an example repo to demonstrate this issue?

[frbuceta]

I use MySQL connector, I don't know if I can create an example at codesandbox or codepen?

[hacksparrow]

@frbuceta you can create one on GitHub itself. Please share a very basic app which reproduces the problem you are reporting.

[frbuceta]

Example repo: https://github.com/frbuceta/loopback-issue3602

[frbuceta]

someone?

[dougal83]

[@frbuceta] I suppose the obvious question is to ask how mysql is generating an id with a guid signature? I was under the impression that mysql sequences generated ids as integers.

Can you confirm that npm run migrate has been executed correctly to update database structure and that was performed on a clean db (or use npm run migrate -- --rebuild that can lose data)? You shouldn't be able to return 'uuid' = null if the database was clean and migrate has been run correctly.

[basavarajvs]

What db connector are you using? Do you have an example repo to demonstrate this issue?

I faced the problem with postgresql connector too.

[frbuceta]

@fabien I suppose the obvious question is to ask how mysql is generating an id with a guid signature? I was under the impression that mysql sequences generated ids as integers.

Can you confirm that npm run migrate has been executed correctly to update database structure and that was performed on a clean db (or use npm run migrate -- --rebuild that can lose data)? You shouldn't be able to return 'uuid' = null if the database was clean and migrate has been run correctly.

MySQL supports UUID --> https://mysqlserverteam.com/mysql-8-0-uuid-support/
And so you force me to use it --> https://dba.stackexchange.com/questions/149059/how-to-insert-a-record-with-generated-identifiers-using-uuid

[dougal83]

[@frbuceta] I suppose the obvious question is to ask how mysql is generating an id with a guid signature? I was under the impression that mysql sequences generated ids as integers.

MySQL supports UUID --> https://mysqlserverteam.com/mysql-8-0-uuid-support/
And so you force me to use it --> https://dba.stackexchange.com/questions/149059/how-to-insert-a-record-with-generated-identifiers-using-uuid

@frbuceta Thanks for info. Personally I would avoid using features like that as it could be _more difficult to debug_. Could using the non default id be the issue for the connector? Perhaps you could confirm by switching to standard id(runs correctly for me)?

[frbuceta]

[@frbuceta] I suppose the obvious question is to ask how mysql is generating an id with a guid signature? I was under the impression that mysql sequences generated ids as integers.

MySQL supports UUID --> https://mysqlserverteam.com/mysql-8-0-uuid-support/
And so you force me to use it --> https://dba.stackexchange.com/questions/149059/how-to-insert-a-record-with-generated-identifiers-using-uuid

@frbuceta Thanks for info. Personally I would avoid using features like that as it could be _more difficult to debug_. Could using the non default id be the issue for the connector? Perhaps you could confirm by switching to standard id(runs correctly for me)?

With the standard id, if it works
On the other hand in postgres if what I am using here works
Is this a bug or is it my thing?

[dougal83]

Is this a bug or is it my thing?

As id is NaN, it seems to be looking for an integer and getting your guid. I'm thinking it could be the connector's ability to handle types of id... Saying that there is an issue with the connector atm, not sure if it is related. We need someone smart to have a look.

@hacksparrow, what do you think?

[bajtos]

I am afraid this is a limitation of the current juggler. If the PK property is defined as autogenerated, juggler always overwrite the type with the default id type supplied by the connector. See here:

https://github.com/strongloop/loopback-datasource-juggler/blob/a90dc0eac4abb17f2335d0803e0828fa79e444dc/lib/datasource.js#L722-L728

    if (idProp && idProp.generated && this.connector.getDefaultIdType) {
      // Set the default id type from connector's ability
      const idType = this.connector.getDefaultIdType() || String;
      idProp.type = idType;
      modelClass.definition.rawProperties[idName].type = idType;
      modelClass.definition.properties[idName].type = idType;
    }

SQL connectors like MySQL and PostgreSQL use number as the default id type. So if the database generates a unique string id, juggler will try to convert that value to a number, and thus return NaN.

This is causing problems for MongoDB too, where a property defined as {type: 'string', mongdb: {dataType: 'ObjectID'}} is converted to {type: ObjectID}.

I think the best way forward is to modify juggler to stop overwriting user-provided id type with connector-provided type. However, I am not able to predict what can such change break.

I think the safest option is to preserve backwards compatibility and introduce a new property-definition metadata to disable type rewrite.

 if (
  idProp && 
  idProp.generated && 
  this.connector.getDefaultIdType && 
  idProp.useDefaultIdType !== false
) {
  // Set the default id type from connector's ability
  // ...
}

This can be implemented as a semver-minor change of juggler.

Then we can decide what to do in LB4:

• We can detect generated: true and add useDefaultIdType: false to the property definition sent to juggler. This will require a semver-major release of @loopback/repository.
• We can pass this task to app developers and require them to set useDefaultIdType: false. Personally, I dislike this option a lot - it's leaking juggler-specific ugliness to LB4 model metadata and also makes it difficult for new LB developers to discover what's the correct way of setting up a generated property.

@strongloop/loopback-next thoughts?

[frbuceta]

I think the best way forward is to modify juggler to stop overwriting user-provided id type with connector-provided type. However, I am not able to predict what can such change break.

Couldn't we make a warning message or do something to implement this first idea?

• We can detect generated: true and add useDefaultIdType: false to the property definition sent to juggler. This will require a semver-major release of @loopback/repository.

In case of not being able, I also think that this would be the best change

[hacksparrow]

@frbuceta can you share the structure details of the uuid field in your database? Mine is int(11) with AUTO_INCREMENT.

Which version of loopback-connector-mysql are you using?

Your sample app autogenerated the id starting with 1 and incremented it with each new user. generated model property made no difference.

[frbuceta]

@frbuceta can you share the structure details of the uuid field in your database? Mine is int(11) with AUTO_INCREMENT.

Which version of loopback-connector-mysql are you using?

Your sample app autogenerated the id starting with 1 and incremented it with each new user. generated model property made no difference.

User Table

create table users
(
    uuid       varchar(36)                        not null
        primary key,
    first_name varchar(20)                        not null,
    last_name  varchar(20)                        not null,
    email      varchar(45)                        not null,
    password   char(60)                           null,
    updated_at datetime                           null on update CURRENT_TIMESTAMP,
    created_at datetime default CURRENT_TIMESTAMP not null
);

UUID Trigger

CREATE DEFINER = CURRENT_USER TRIGGER `classdrive`.`users_BEFORE_INSERT` BEFORE INSERT ON `users` FOR EACH ROW
BEGIN
    SET NEW.uuid = UUID();
END

Versions:
@loopback/core 1.10.4
loopback-connector-mysql 5.4.2

[frbuceta]

We can add to the list to MsSQL @bajtos

[frbuceta]

As soon as I have a moment, I update in code.

[richardpringle]

@strongloop/loopback-next thoughts?

@bajtos, I completely agree. Leaking the implementation details into LB4 would be a mistake IMO. Using UUIDs as the primary key is pretty common in my experience. I also think that @loopback/repository could possibly offer UUID generation (in-app generation instead of in the db) if the majority of cases are centred around UUIDs.

As for Mongo... I'm not quite sure what a good solution for that would be.

[bajtos]

For anybody watching this issue: https://github.com/strongloop/loopback-datasource-juggler/pull/1783 introduced a new property-level flag useDefaultIdType that instructs juggler to ignore the PK-type defined by the connector and use the type provided by the developer.

I am thinking that perhaps we can modify legacy juggler bridge to enable that flag automatically when the property metadata includes type field. I mean if the LB4 user wrote @property({type: 'string', id: true, generated: true}), then I think it's safe to assume they wanted to use string, not ObjectID (in Mongo) or number (in SQL).

Would you consider that new behavior as a breaking change?

/cc @strongloop/loopback-next

[frbuceta]

For anybody watching this issue: strongloop/loopback-datasource-juggler#1783 introduced a new property-level flag useDefaultIdType that instructs juggler to ignore the PK-type defined by the connector and use the type provided by the developer.

I am thinking that perhaps we can modify legacy juggler bridge to enable that flag automatically when the property metadata includes type field. I mean if the LB4 user wrote @property({type: 'string', id: true, generated: true}), then I think it's safe to assume they wanted to use string, not ObjectID (in Mongo) or number (in SQL).

Would you consider that new behavior as a breaking change?

/cc @strongloop/loopback-next

I am in favor of this change

[bajtos]

@frbuceta would you like to contribute that change yourself?

[frbuceta]

@frbuceta would you like to contribute that change yourself?

Of course, lately I haven't had much time but tomorrow I start with it. Sorry for the delay

[agnes512]

I think LB4 is able to auto-generate uuid for MySQL with auto-migration. Here is my model definition:

export class Customer extends Entity {
  @property({
    id: true,
    type: 'string'
    defaultFn: 'uuidv4',
    // generated: true,  -> not needed
    // useDefaultIdType -> not needed
  })
  uuid_id: string;

  @property({
    generated: true,
    type: 'number',
  })
  int_id: number;

  @property({
    type: 'string',
  })
  name: string;
}

Ref: General property properties

As what we discussed above, for int default type DBs such as MySQL, PlstgreSQL, generated=true only auto-generates int type id.
@frbuceta Does defaultFn here meet your requirement?

(Out of scope: if a property is not the identifier, the flag generated has no effect. ( e.g int_id))

[bajtos]

@agnes512 I believe that we want to let the database generate the UUID values, not LoopBack. The suggested property definition will generate the id value at LoopBack side.

  @property({
    id: true,
    type: 'string'
    defaultFn: 'uuidv4',
    // generated: true,  -> not needed
    // useDefaultIdType -> not needed
  })
  uuid_id: string;

What we want to achieve is the following property definition inside the juggler layer:

{
  id: true,
  type: 'string',
  generated: true,
}

Now because juggler is replacing any user-provided type with connector-provided default type whenever a property is an auto-generated primary key ({id: true, generated: true}), i.e. replacing type: 'string' to type: 'number' for MySQL connector, we want to add useDefaultIdType flag to disable that change.

[agnes512]

Note that even #4270 is merged, the flag is only preserved at the juggler level. MySQL connector needs to be updated, see https://github.com/strongloop/loopback-connector-mysql/blob/master/lib/migration.js#L629 ( PostgreSQL is able to auto generate uuid now)).

[lemchen]

useDefaultIdType: false seems doesn't work in mongodb connector, mongodb still generate default string id value