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

Virtual property causes incorrect MySQL query #5261

Closed
5 tasks done
Asesjix opened this issue Feb 20, 2024 · 6 comments
Closed
5 tasks done

Virtual property causes incorrect MySQL query #5261

Asesjix opened this issue Feb 20, 2024 · 6 comments

Comments

@Asesjix
Copy link

Asesjix commented Feb 20, 2024

Describe the bug

When specifying a virtual property in the field options of the find function, the virtual property is erroneously included in the MySQL database query. This is problematic because virtual properties do not exist in the database, leading to incorrect query results or errors. Additionally, when the virtual property is omitted from the field options, the return type of the find function is no longer accurate, potentially causing issues with type checking and data handling downstream.

select `p0`.`id`, `p0`.`rateSum`, `p0`.`rateCount`, `p0`.`rating` from `my_entity` as `p0` where `p0`.`id` = 1 limit 1 - Unknown column 'rating' in 'field list'

Reproduction

Define a virtual property in the model.
Use the find function with the virtual property specified in the field options.
Observe that the virtual property is included in the MySQL query.

export class MyEntity {
  id!: number
  rateSum?: number = 0
  rateCount?: number = 0
  likeCount = 0
  mtime!: Date

  get rating() {
    if (
      !this.rateCount ||
      this.rateCount <= MIN_VOTES_FOR_RATING ||
      !this.rateSum ||
      Math.abs(this.rateSum) > this.rateCount
    ) {
      return 0
    }
    const average = (this.rateSum / this.rateCount) * 100
    return Math.round(average)
  }
}

export const MyEntitySchema = new EntitySchema({
  class: MyEntity,
  properties: {
    id: { primary: true, type: 'number' },
    rateSum: {
      type: 'number',
      columnType: 'mediumint',
      nullable: true,
      default: 0,
    },
    rateCount: {
      type: 'number',
      columnType: 'mediumint',
      nullable: true,
      default: 0,
    },
    rating: {
      type: 'method',
      persist: false,
      getter: true,
    },
    mtime: {
      type: 'Date',
      defaultRaw: `CURRENT_TIMESTAMP`,
    },
    likeCount: { type: 'number', default: 0 },
  },
})

async getMyEntity(id: number): Promise<MyEntityDto | null> {
  const myEntity = await this.#entityRepo.findOne(
    { id },
    {
      fields: ['id', 'rateSum', 'rateCount', 'rating']
    },
  )

  return myEntity ? mapToMyEntityDto(myEntity) : null
}

What driver are you using?

@mikro-orm/mysql

MikroORM version

6.1.4

Node.js version

18.18.2

Operating system

Debian

Validations

@B4nan
Copy link
Member

B4nan commented Feb 20, 2024

Why do you put it into the fields hint? I don't think we prune that array, and it feels like a breaking change to start doing so. Sounds like a wontfix.

@Asesjix
Copy link
Author

Asesjix commented Feb 20, 2024

I added this info in the description:

When the virtual property is omitted from the field options, the return type of the find function is no longer accurate, potentially causing issues with type checking and data handling downstream.

async getMyEntity(id: number): Promise<MyEntityDto | null> {
  const myEntity = await this.#entityRepo.findOne(
    { id },
    {
      fields: ['id', 'rateSum', 'rateCount']
    },
  )

  // myEntity.rating does not exist

  return myEntity ? mapToMyEntityDto(myEntity) : null
}

This broke while migrating to version 6.

@B4nan
Copy link
Member

B4nan commented Feb 20, 2024

I see, and I'm afraid this might also mean it will not be part of the serialized form.

And this worked in v5 or you had to add it when trying to migrate to v6?

@StevenKowalzik
Copy link

StevenKowalzik commented Feb 20, 2024

Yes, in v5 the returned type contained the virtual property rating without it being defined in the fields array. We had to add it while migrating because the returned Loaded type did not contain it anymore.

@B4nan
Copy link
Member

B4nan commented Feb 20, 2024

I am asking whether it was possible to have the virtual property in fields hint, as I don't think that's the case. I understand why you've added there during the migration, that's indeed a valid point to address somehow.

edit: it works the same in v5 and serialization is not a problem

@Asesjix
Copy link
Author

Asesjix commented Feb 21, 2024

I have checked this again. Previously, the rating was not specified in the fields and was therefore not included in the database query. However, the return type of find was not a problem because the property was there, just like all the others.

Now, with version 6, the property is no longer in the return type of find. Therefore, we tried to include the property in the fields, but this causes the error with the query.

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

No branches or pull requests

3 participants