feat(query-builder): respect EntityManager schema

[EmmEm]

• QueryBuilder will inherit schema from EntityManager (if set)
• QueryBuilder can now join entities that belong in a different schema than the main entity.

Previously if you had an entity that belongs to schema n2 which have a FK to an entity that does not exist in n2 the query will fail since it will use n2.SubDomain. If you used withSchema('n5'), then it will fail since n5.Domain does not exist.

@Entity({ schema: 'n2' })
class Domain {

  @PrimaryKey()
  id!: number;

  @OneToMany(() => SubDomain, e => e.domain)
  subDomain = new Collection<SubDomain>(this);
}

@Entity({ schema: '*' })
class SubDomain {

  @PrimaryKey()
  id!: number;

  @ManyToOne(() => Domain, { nullable: true })
  domain?: Domain;
}

Im wondering what you think about this approach. One thing I'm a bit divided about is the fallback for joinSchema should it be

const joinSchema = this._schema ?? this.em?.schema ?? this.em?.config.get('schema');
// OR
// This approach will be a minor change since the only time it will differ from `schema` is if `this.em.schema` is set.
const joinSchema = this._schema ?? this.em?.schema ?? schema;

v6 questions?

Should schema: '*' be treated the same way as schema: undefined?

I've kept the QueryBuilder.WithSchema logic that will make sure it will triumph all other schema settings. But I think it will be problematic to use, since it will not handle cases when entities are placed in different schemas.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

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

... and 8 files with indirect coverage changes

📢 Thoughts on this report? Let us know!.

[B4nan]

Im wondering what you think about this approach. One thing I'm a bit divided about is the fallback for joinSchema should it be

I am not entirely sure here, but I would prefer the less intrusive change. One thing that would be good to test more carefully is how this affects working with the em.find when it comes to joined strategy, as we need that to behave the same as the default select-in strategy.

Should schema: '*' be treated the same way as schema: undefined?

Not sure what you mean by that, like when you have a wildcard entity it would always prefer the EM schema (or the global one from config)? It already works like that, or not?

I believe it's important to preserve that the (wildcard) owning entity schema is respected in the relations (if the relation target is also wildcard entity).

[EmmEm]

I don't think schema: '*' and schema: undefined is treated the same.

// QueryBuilderHelper.ts ~152
const schema = prop.targetMeta?.schema === '*' ? '*' : this.driver.getSchemaName(prop.targetMeta);

I added the same logic to ~167 and ~185. If undefined is treated the same it should check for that as well. There are more examples in the code where * is treated differently from undefined.

I will change to the less intrusive schema change. It will mean that if '*' is set as schema on the joined entity and WithSchema is not used and em.schema is not used it will fall back to same schema as for the main table in the query.

I will add more tests for em.find to make sure it behaves the same.

[EmmEm]

As far as I can see em.find will not change its behaviour since it uses withSchema which will set schema for all joins except for those who have a schema set on the entity. And changing that behaviour will be a breaking change.

This PR focuses on using the QueryBuilder directly and to make use of em.schema if it's set.

I would like to take a shoot at refactoring the usage of withSchema so the main table is not the one that decides the schema for joined tables. It will be a breaking change and I will try to create a draft for it with a discussion.

[B4nan]

The current implementation is indeed pretty messy, I am having a hard time reasoning about the code...

Do you plan to add something to this PR or can we merge? It looks good to me, and I'd like to ship new stable later today so would be good to include it there.

[EmmEm]

I don't plan to add anything if you are happy. I will get back to you about schema prioritisation and use cases.

All the best!

[B4nan]

All right, thank you!