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

Incorrect propagation of one-to-one relation when using BigInt PKs and BigIntType #578

Closed
nikolaylaz opened this issue May 17, 2020 · 5 comments
Assignees
Labels
bug Something isn't working
Milestone

Comments

@nikolaylaz
Copy link

@Entity()
export class Company {

  @PrimaryKey({ type: BigIntType })
  id: string;

  @Property({ onUpdate: () => new Date() })
  dateLastModified = new Date();

  @Property({length: 255})
  name: string;

  @OneToOne({owner: true, cascade: [Cascade.ALL], orphanRemoval: true})
  video: Video;
}
@Entity()
export class Video {

  @PrimaryKey({type: BigIntType})
  id: string;

  @Property({ onUpdate: () => new Date() })
  dateLastModified = new Date();

  @Property()
  name: string;

  @OneToOne()
  company: Company;
}
const company = await this.companyRepository.findOne('5', {
      populate: ['video']
    });

    company.name = 'new comppany nam2se';

    await this.companyRepository.persistAndFlush(company);

Stack trace / Logs

[0] [Nest] 10732   - 2020-05-17 18:10:55   [MikroORM] [query] select `e0`.* from `company` as `e0` where `e0`.`id` = '5' limit 1 [took 3 ms]
[0] [Nest] 10732   - 2020-05-17 18:10:55   [MikroORM] [query] select `e0`.* from `video` as `e0` where `e0`.`id` in (3) order by `e0`.`id` asc [took 2 ms]
[0] [Nest] 10732   - 2020-05-17 18:10:55   [MikroORM] [query] begin
[0] [Nest] 10732   - 2020-05-17 18:10:55   [MikroORM] [query] update `company` set `name` = 'new comdppany nam2se', `video_id` = '3', `date_last_modified` = '2020-05-17 18:10:55.493' where `id` = '5' [took 3 ms]
[0] [Nest] 10732   - 2020-05-17 18:10:55   [MikroORM] [query] delete from `video` where `id` = '3' [took 3 ms]
....

There is no need for delete query, becase only the company name is changed.

After doing some debugging i found that in the method diffEntities in Utils class arguments a and b has the following values:

a = {
  "id": "5",
  "dateLastModified": "2020-05-16T18:55:39.000Z",
  "name": "new comppany nam2se",
  "video": 3
}

b = {
  "id": "5",
  "dateLastModified": "2020-05-16T18:55:39.000Z",
  "name": "new comdppany nam2se",
  "video": {
    "id": "3",
    "dateLastModified": null,
    "name": "test video"
  }
}

Video property in the first object is of type number, but in the second is string.
I believe this is the source of the problem.

Expected behavior
Shouldn't apply any propagation on one-to-one relation when there are no modifications on it.

Versions

Dependency Version
node 12.16.1
typescript 3.7.5
mikro-orm 3.6.13
mariadb 2.3.1
@nikolaylaz nikolaylaz added the bug Something isn't working label May 17, 2020
@B4nan
Copy link
Member

B4nan commented May 18, 2020

Just a guess, but is your FK column (Company.video) correctly typed (as a bigint)?

@nikolaylaz
Copy link
Author

Yes, both are BIGINT(20)

@nikolaylaz
Copy link
Author

As a workaround/fix i added

driverOptions: {
        connection: {
          supportBigNumbers: true,
          bigNumberStrings: true
        }
}

to the mikro orm config.

I saw that for mariadb, the framework is using mysql knex dialect and overriding the driver with the one from mariadb package

dialect.prototype._driver = () => require('mariadb/callback');
but this is causing some problems, because knex.js for mysql has a default value equals to true for supportBigNumbers option, but the implementation for it is different in mysql and mariadb packages
MariaDB - https://github.com/mariadb-corporation/mariadb-connector-nodejs/blob/master/documentation/connection-options.md#big-integer-support

MySQL - https://github.com/mysqljs/mysql#connection-options
As per documentation in mysql package it will return string if the integer is not in the safe range and cannot be parsed as javascript number, but in mariadb package it will return Long object from long.js package.

Is it necessary to use mariadb driver there, because in knex.js website there is a text saying
‘’ If you want to use a MariaDB instance, you can use the mysql driver.” - in the node.js section.

(if you decide you can close the issue - maybe it is nice to have these options in the documentation somewhere :) )

@B4nan
Copy link
Member

B4nan commented May 20, 2020

Interesting. Well the purpose of the MariaDbDriver is to use the mariadb connector - if one wants to use mysql driver, they can do so.

Btw supportBigNumbers: true is set implicitly, in both mysql and mariadb (as it extends it) drivers:

ret.supportBigNumbers = true;

I didn't really understand why is there the bigNumberStrings option - so I guess we should also enable that flag by default. If MikroORM users want to use some wrapper like long.js, it should be implemented via custom type, and the raw/database value should be represented as string, always.

I will be doing some changes there in v4, basically narrowing it to strings for numeric/decimal types, as for example postgres will convert them to numbers by default, while mysql probably not.

@B4nan B4nan added this to the 4.0 milestone May 20, 2020
B4nan added a commit that referenced this issue May 21, 2020
@B4nan
Copy link
Member

B4nan commented May 21, 2020

Closing as fixed in dev branch, subscribe to #527 for updates.

@B4nan B4nan closed this as completed May 21, 2020
B4nan added a commit that referenced this issue May 21, 2020
B4nan added a commit that referenced this issue May 21, 2020
B4nan added a commit that referenced this issue May 21, 2020
B4nan added a commit that referenced this issue Jun 1, 2020
B4nan added a commit that referenced this issue Jun 5, 2020
B4nan added a commit that referenced this issue Jun 16, 2020
B4nan added a commit that referenced this issue Aug 2, 2020
B4nan added a commit that referenced this issue Aug 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants