fix(query-builder): add alias to embedded property only if embedded is not prefixed

[mlsm-trl]

When querying an entity that has prefixed Embedded, TableNotFoundException is thrown due to wrong generated query syntax.

Here is the reproduction and contextualization of the bug:
mlsm-trl/mikro-orm-bug-virtual-properties-required-when-seeding

The idea would be to check if property not comes from a prefixed Embedded, and then add the alias.

I don't know if we have a better/reliable way to do this than the string comparison I made.

[codecov]

Codecov Report

All modified lines are covered by tests ✅

Files	Coverage Δ
packages/knex/src/query/QueryBuilder.ts	99.40% <100.00%> (+<0.01%)	⬆️

📢 Thoughts on this report? Let us know!.

[B4nan]

Why should a prefixed embedded property be treated somehow differently? Sounds like you are too concerned with the fact that it's prefixed, but the problem will be probably something else.

Let's start by turning that repro into a test case (check the tests/issues folder), so it's easier for me to try to find a better solution, as this one seems wrong - we can't make things behave differently based on a property name...

[B4nan]

Looking at your repro, is this related to the custom types (which you use there)? As you haven't mentioned them anywhere, but I can surely see some issues with prefixing such properties - but that has nothing to do with the property name.

[mlsm-trl]

Looking at your repro, is this related to the custom types (which you use there)? As you haven't mentioned them anywhere, but I can surely see some issues with prefixing such properties

Yes, geolocation prop inside embedded is a custom type, which then sets the prop with hasConvertToDatabaseValueSQL and hasConvertToJSValueSQL, as I mentioned.

Let's start by turning that repro into a test case (check the tests/issues folder), so it's easier for me to try to find a better solution, as this one seems wrong - we can't make things behave differently based on a property name...

Yeah, it is very unreliable as I said, I don't have deep knowledge of codebase to came with a better solution... But the idea I think remains the same.

... I can surely see some issues with prefixing such properties - but that has nothing to do with the property name.
...
Why should a prefixed embedded property be treated somehow differently? Sounds like you are too concerned with the fact that it's prefixed, but the problem will be probably something else.

In v5.7.14 this same repro works. I don't know what could have changed from v5.7.14 to v5.8.0, but whatever it is affected the emitted query. I suspect that has some to do with issue #4711 as aliasing changed there, making the mapper not find the property in QueryBuilderHelper.ts#L76...L79 (I did test with some console.log's)...

let ret = field;
...
console.log(field)
// u0.addresses.geolocation
const [a, f] = this.splitField(field);
console.log(a, f)
// u0 addresses.geolocation
const prop = this.getProperty(f, a);
console.log(prop)
// undefined

and in v5.7.14 the field is found, because the u0 alias is not there:

let ret = field;
...
console.log(field)
// addresses.geolocation
const [a, f] = this.splitField(field);
console.log(a, f)
// addresses geolocation
const prop = this.getProperty(f, a);
console.log(prop)
/*
{
  name: 'addresses',
  type: 'Address',
  reference: 'embedded'
  ...
}
*/

but I don't know if the change in alias really is the culprit...

In v5.7.14, this is the produced query:

select "u0".*, "u0"."addresses" from "user" as "u0" where "u0"."id" = 1 limit 1

and then in v5.8.0...v5.8.8:

select "u0".*, "u0"."addresses"."geolocation" from "user" as "u0" where "u0"."id"

which is incorrect

[B4nan]

Yes, geolocation prop inside embedded is a custom type, which then sets the prop with hasConvertToDatabaseValueSQL and hasConvertToJSValueSQL, as I mentioned.

Your repro is using those on an embedded array property, in other words, on a JSON property - this is not supported, the SQL conversion methods will work only with a real column. So it actually did not work before either, it just didn't fail on the query level like this, but the SQL convertors weren't there - and that is the only reason why this was added to the select clause.

The correct fix is to completely omit this bit from the select clause.