loadItems fails with invalid field name exception

[sualko]

Describe the bug

During a call to loadItems I get an InvalidFieldNameException with either Unknown column 'xxxx' in 'field list' or Column 'xxxx' in field list is ambiguous.

What I noticed is that in both cases there is a property with the lazy attribute, so the field list gets expanded. So instead of b0.*, all attributes are listed, but the field names from relations are without alias. E.g. @ManyToOne(() => User, { fieldName: "userId" }) in this case the field list would contain something like ..., b0.foo, userId, b0.bar, ...

Reproduction

https://github.com/sualko/reproduction/tree/load-items

I created an example which shows that not all fields in the selection list are aliased, even if the example does not exactly print the error message from above.

select
    `b0`.`book_id`,
    `b0`.`title`,
    `user_id`, -- This is the issue
    `p1`.`page_id` as `p1__page_id`,
    `p1`.`content` as `p1__content`,
    `p1`.`book_book_id` as `p1__book_book_id`,
    `p1`.`user_user_id` as `p1__user_user_id`
from
    `book` as `b0`
    left join `page` as `p1` on `b0`.`book_id` = `p1`.`book_book_id`
where
    `b0`.`userId` in (1)
    and `b0`.`title` = '42'

If you set the lazy attribute to false in the example, everything works.

What driver are you using?

@mikro-orm/mysql

MikroORM version

6.0.7

Node.js version

18.13.0

Operating system

Ubuntu 22.04

Validations

• Read the Contributing Guidelines.
• Read the docs.
• Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
• Check that this is a concrete bug. For Q&A open a GitHub Discussion or join our Slack.
• The provided reproduction is a minimal reproducible example of the bug.

[B4nan]

Just a side note, as I see this way too often: you don't need that problematic persist: false column, all you get with that is the value you can already read without populating anything from the relation property. Curious to hear why you want it? For a long time I just kept fixing those things without asking, so maybe its the right time to ask again.

[sualko]

Curious to hear why you want it?

I think it all boils down to personal preference. In our case we converted a large code base from typeorm to mikro-orm and therefore we need the relation id inside the entity to keep the change set as small as possible. Especially on the frontend, where we use relation ids a lot. Also for our team it feels more common to access the relation id instead of the relation directly. I think there is no benefit from one solution over the other, it's just nice if a ORM supports both patterns.

While we talk about relation ids: We really like the forceObject serialization option, but since we use relation ids we would also love to see a possibility to exclude unloaded relations. Would you accept an pr for that? I'm happy to open a new issue to discuss the backstory behind this.

[B4nan]

Also, your repro seems wrong, you are setting fieldName: 'userId' but it should be user_id, then it all works fine.

@ManyToOne(() => User, { fieldName: 'user_id' })
user!: User;

And in fact this also does not need to be there, this is the default mapping already.

Especially on the frontend, where we use relation ids a lot.

I see, so the motivation is to have the userId value next to user when you populate the relation, that makes sense.

[B4nan]

While we talk about relation ids: We really like the forceObject serialization option, but since we use relation ids we would also love to see a possibility to exclude unloaded relations. Would you accept an pr for that? I'm happy to open a new issue to discuss the backstory behind this.

The point of forceObject is that you have stable results regardless of the population state, so you don't need the duplicated FK property with persist: false. If you adopt this, the problem you want to solve is already solved without having that extra property, or did I miss something?

I don't mind opt-in improvements, so PR welcome if you see a need for it, it just feels weird.

[B4nan]

Back to your repro, as I mentioned, I believe its just a misusage, as you define two properties for the FK and they dont have matching column names. If the column is supposed to be called user_id, just drop the fieldName option or fix it to be user_id:

@ManyToOne(() => User)
user!: User;

@Property({ persist: false })
userId!: number & Opt;

Alternatively, if you want it to be userId, you need to add it to the userId property too.

@ManyToOne(() => User, { fieldName: 'userId' })
user!: User;

@Property({ persist: false, fieldName: 'userId' })
userId!: number & Opt;

[sualko]

Also, your repro seems wrong, you are setting fieldName: 'userId' but it should be user_id, then it all works fine.

It is a bit hard for me to create a nice reproducible case here. I tried to show you that the alias is missing in some cases. This indicates a larger issue for me. Now we saw similar issues with the find method. Something like the following throws an Column 'xxxx' in field list is ambiguous due to a missing alias.

em.find(User, {book: {title: 'Foo'}}, {fields: ['some prop which exists on both entities']});

If I print the sql statement I see that the alias is missing for the key defined in the fields array. If you need a full reproducible case, I'm happy to construct one.

I mentioned, I believe its just a misusage, as you define two properties for the FK and they dont have matching column names.

You are right, I did a mistake in this repo, but it should show that the alias is missing. Can you confirm this? Otherwise I have to try to fix the repo.

I see, so the motivation is to have the userId value next to user when you populate the relation, that makes sense.

Yes, but also if I do not populate the relation. I always need it, especially on the frontend.

If you adopt this, the problem you want to solve is already solved without having that extra property, or did I miss something?

I think it's better to discuss this in another thread.

[B4nan]

It is a bit hard for me to create a nice reproducible case here. I tried to show you that the alias is missing in some cases. This indicates a larger issue for me. Now we saw similar issues with the find method. Something like the following throws an Column 'xxxx' in field list is ambiguous due to a missing alias.

It's missing on purpose on virtual properties. Those are not added to the select clause by the ORM, but due to the misconfigured virtual column, you end up with it selected - because the fieldName of what is selected automatically (the relation prop) is the same as a virtual property name. I get your point, adding a virtual property shouldn't break anything, I can try to fix that part - but it still does not mean it will work as you wanted, as the entity definition is misconfigured, the column userId won't be mapped to userId property this way as they don't share the field name.

em.find(User, {book: {title: 'Foo'}}, {fields: ['some prop which exists on both entities']});

This is surely not as simple as you think, partial loading works for nested entities and requires dot paths. So yes, provide a complete repro.

[sualko]

but due to the misconfigured virtual column

In our production code, the field is not misconfigured and we have the same issue. I do not really understand why virtual properties are added to the select and why you are saying that they are added on purpose without alias. What's the idea behind this?

This is surely not as simple as you think

I looked at the code and I can guarantee you that I don't have wrong assumptions on that. The code is awesome and you put a lot of effort into it. Fixing such things is obviously not easy and I appreciate every help. I will try to add a case to the repo, but probably not today. I send you a message if I'm done.

[B4nan]

I do not really understand why virtual properties are added to the select and why you are saying that they are added on purpose without alias. What's the idea behind this?

That's the thing, they are not added to the select by the ORM, in your repro this happens because of the misconfiguration I pointed out. They should not have alias, because they are not supposed to be representing a real column - the way you use them for the FKs was always just a workaround. The use case for those is something you compute and alias.

https://mikro-orm.io/docs/defining-entities#virtual-properties

Maybe worth noting that there is a way to enforce alias on virtual property - use a formula instead.