-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Preserve custom type of auto-generated id property #3602
Comments
What db connector are you using? Do you have an example repo to demonstrate this issue? |
I use MySQL connector, I don't know if I can create an example at codesandbox or codepen? |
@frbuceta you can create one on GitHub itself. Please share a very basic app which reproduces the problem you are reporting. |
Example repo: https://github.com/frbuceta/loopback-issue3602 |
someone? |
[@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 |
I faced the problem with postgresql connector too. |
MySQL supports UUID --> https://mysqlserverteam.com/mysql-8-0-uuid-support/ |
@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 |
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? |
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: 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 This is causing problems for MongoDB too, where a property defined as 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:
@strongloop/loopback-next thoughts? |
Couldn't we make a warning message or do something to implement this first idea?
In case of not being able, I also think that this would be the best change |
@frbuceta can you share the structure details of the Which version of Your sample app autogenerated the |
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: |
We can add to the list to MsSQL @bajtos |
As soon as I have a moment, I update in code. |
@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 As for Mongo... I'm not quite sure what a good solution for that would be. |
For anybody watching this issue: loopbackio/loopback-datasource-juggler#1783 introduced a new property-level flag I am thinking that perhaps we can modify legacy juggler bridge to enable that flag automatically when the property metadata includes Would you consider that new behavior as a breaking change? /cc @strongloop/loopback-next |
I am in favor of this change |
@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 |
I think LB4 is able to auto-generate 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, (Out of scope: if a property is not the identifier, the flag |
@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 ( |
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)). |
useDefaultIdType: false seems doesn't work in mongodb connector, mongodb still generate default string id value |
Steps to reproduce
generated: true
Current Behavior
As I describe above, when the model is in
generated: true
it has to return the normal id, not NaNExpected Behavior
Link to reproduction sandbox
When I put generated: false ...
![Captura de pantalla 2019-08-25 a las 16 26 37](https://user-images.githubusercontent.com/13822438/63651536-f77fb480-c755-11e9-8847-267abc0ae9ef.png)
![Captura de pantalla 2019-08-25 a las 16 26 47](https://user-images.githubusercontent.com/13822438/63651526-dd45d680-c755-11e9-8e2e-30d0239d4a5b.png)
Additional information
OS: MacOS 10.14.6
Node: v12.9.0
Npm: v6.11.2
Loopback: latest ->
Related Issues
See Reporting Issues for more tips on writing good issues
The text was updated successfully, but these errors were encountered: