Using EntityManager.map() with Query Builder

[thekevinbrown]

Is your feature request related to a problem? Please describe.
We're using QueryBuilder because we need some relatively complex join logic. What we'd like is to be able to take this query, then load entities from it. We are using EntityManager.map() but hit two things with it that I'm not sure are meant to be supported or not, so I'm assuming it's a feature request.

Describe the solution you'd like
We'd like to be able to await query builder's execute() then EntityManager.map() to get entities back. Our current implementation (which we think we can improve) is:

const query = [our query builder];
const driver = orm.em.getDriver();
const meta = orm.em.getMetadata().get(this.mikroType.name);

// 1:1 relations that aren't on the owning side need to get populated so the references get set.
// This method is protected, but we need to use it from here, hence the `as any`.
const populate = (driver as any).autoJoinOneToOneOwner(meta, []);
return (await query.populate(populate).execute()).map((entity: any) =>
	orm.em.map(this.mikroType, driver.mapResult(entity, meta) || {})
);

We're needing to do more than just a call to .map for three reasons:

1. When we call .map with the raw query builder result, we get an error converting numbers to strings. driver.mapResult fixes this, but then we need to grab the metadata for the entity to do that process.
2. We use @OneToOne relationships, so are currently using the autoJoinOneToOneOwner helper, but we can see this isn't meant to be part of the API.
3. driver.mapResult is typed so that it can return null, which EntityManager.map does not accept, hence the || {} that we know won't actually be used here, this is just to get TypeScript to calm down.

Describe alternatives you've considered
Continue with this implementation as is, because it works fine for the moment. Not ideal as it's quite tightly coupled to Mikro internals, but would be absolutely fine.

Additional context

• To me it feels like driver.mapResult should happen within EntityManager.map itself. If map is meant to take a raw database result from a query builder then this is a step that we should probably do within .map, no?
• On the @OneToOne front, it feels like we're probably going about this the wrong way. Is there a supported API way to tell a query builder to do these joins before we run execute()? If not, should there be for this use case? Should this method become public?

As always, happy to do the PR to make .map easier to use, just wondering about the intended developer experience here and if we're just using it wrong.

[B4nan]

Why don't use use qb.getResult() instead? Also qb.execute() has a mapResults parameter that defaults to true - you should not need to call it explicitly.

autoJoinOneToOneOwner is handled for EM only, as with QB you should rather join yourself to make things explicit - I didn't want to introduce too much magic to QB. The whole qb.populate api is quite magical and I would very much like to get rid of it in the long run.

mapResult can return null if you try to use it with a non-entity (or with a falsy result argument, e.g. when the entity was not found). It is API on driver level, and you can use driver with any table, without entity for that table defined. So it should be safe to use non-null assertion operator (!).

Lastly, em.map already calls driver.mapResult, see the code:

https://github.com/mikro-orm/mikro-orm/blob/master/packages/core/src/EntityManager.ts#L484

[thekevinbrown]

Interesting, I'll have a look. Thanks!

[thekevinbrown]

Yup, much simpler. I'm not sure how I missed getResult(). That solves the weirdest bits.

We're still depending on autoJoinOneToOneOwner for the moment (as without it we'd need to just copy and paste the identical logic into our code) but getResult() does exactly what we wanted, so thanks!

[B4nan]

Maybe we could move the autoJoinOneToOneOwner logic to query builder under a flag.

[thekevinbrown]

Yup, that'd work well I think.

Do you want me to explore a PR that does that?

[B4nan]

Sure, feel free to give it a try. Should be just about moving the logic inside QB (finalize method seems like a good fit on the first sight). Should be disabled by default, and the places that use the method should just enable the flag for given QB.

[thekevinbrown]

Yup, cool, I'll give it a try.

[thekevinbrown]

It's a bit too tangled up to pull it into the query builder I think.

In findOne, joinedProps needs to know about the populate, which was coming from the driver before this line:

this.joinedProps(meta, populate);

If we move this into QueryBuilder, then a bunch of tests (like EntityManagerMySql › 1:1 relationships with an inverse side primary key of 0 should link) fail because the joining isn't happening early enough. I can't figure out how we'd keep the knowledge of this separate until after the construction step to then do it in finalize in QB. It needs to happen before we build out all the pivot stuff.

I think it'd make more sense to expose a method on the query builder called autoJoinOneToOneOwner similar to how we have autoJoinPivotTable, which would just be a publicly supported way to call the autoJoinOneToOneOwner on the driver but with all the metadata and stuff loaded already, which would then add to the populate parameter.

Does that make sense and sound like a good idea?

In other words, leave things how they are, but also add a way for users to explicitly ask QueryBuilder to do the auto join, leaving the logic in the AbstractSQLDriver.

[B4nan]

But qb.autoJoinPivotTable is a private method. I would still prefer to do this via QueryFlag rather than a new (public) method with weird name :]

[B4nan]

4.5.4-dev.15