OneToMany property find with populate + fields

[tsangste]

Describe the bug
When performing a find query where I set the populate on a OneToMany property and select certain fields on find options I get the error "Cannot read properties of undefined (reading '0')"

Stack trace

Cannot read properties of undefined (reading '0')
2022-01-19T19:56:45.953712800Z     at QueryBuilderHelper.fieldName (/usr/app/node_modules/@mikro-orm/knex/query/QueryBuilderHelper.js:446:37)
2022-01-19T19:56:45.953715600Z     at QueryBuilderHelper.prefix (/usr/app/node_modules/@mikro-orm/knex/query/QueryBuilderHelper.js:400:36)
2022-01-19T19:56:45.953717700Z     at QueryBuilderHelper.mapper (/usr/app/node_modules/@mikro-orm/knex/query/QueryBuilderHelper.js:43:24)
2022-01-19T19:56:45.953719800Z     at /usr/app/node_modules/@mikro-orm/knex/query/QueryBuilder.js:428:34
2022-01-19T19:56:45.953721900Z     at Array.forEach (<anonymous>)
2022-01-19T19:56:45.953724100Z     at QueryBuilder.prepareFields (/usr/app/node_modules/@mikro-orm/knex/query/QueryBuilder.js:421:16)
2022-01-19T19:56:45.953727200Z     at QueryBuilder.getQueryBase (/usr/app/node_modules/@mikro-orm/knex/query/QueryBuilder.js:469:32)
2022-01-19T19:56:45.953734000Z     at QueryBuilder.getKnexQuery (/usr/app/node_modules/@mikro-orm/knex/query/QueryBuilder.js:233:25)
2022-01-19T19:56:45.953739500Z     at QueryBuilder.execute (/usr/app/node_modules/@mikro-orm/knex/query/QueryBuilder.js:300:28)
2022-01-19T19:56:45.953743700Z     at PostgreSqlDriver.find (/usr/app/node_modules/@mikro-orm/knex/AbstractSqlDriver.js:46:46)
...

To Reproduce
Steps to reproduce the behavior:
Entities -

@Entity()
export class Author {
  @PrimaryKey()
  id!: number;

  @Property()
  name!: string;

 @Property()
  email!: string;

 @OneToMany(() => Book, (book) => book.author)
  books = new Collection<Book>(this);
}

@Entity()
export class Book {
 @PrimaryKey()
  id!: number;

  @Property()
  title!: string;

  @ManyToOne(() => Author)
  author!: Author;
}

Fail Example 1 - throws above exception

const author =  await this.authorRepository.find(
      {},
      {
        populate: ['books'],
        fields: ['name', 'books', 'books.title'],
      },
    );

Fail Example 2 - throw no exception but returns empty books

const author =  await this.authorRepository.find(
      {},
      {
        populate: ['books'],
        fields: ['name', 'books.title'],
      },
    );

Working Example - Returns data but returns all fields

const author =  await this.authorRepository.find(
      {},
      {
        populate: ['books'],
      },
    );

I've created a repo which recreates the problem https://github.com/tsangste/mikro-orm-populate-fields

Expected behavior
Expected to populate the OneToMany field and select the appropriate fields to return back

Versions

Dependency	Version
node	16.13.1
typescript	4.5
mikro-orm	4.5.10
postgres	14.1

[evantrimboli]

It might be worth trying this in 5.x. I know some changes went in to be able to infer the populate statement via the fields, so you could try:

const author =  await this.authorRepository.find({}, {
  fields: ['name', 'books.title'],
});

[tsangste]

const author =  await this.authorRepository.find({}, {
  fields: ['name', 'books.title'],
});

I updated to 5.x and ran your example but it returns the same results as example 2 where it throws no exception but returns empty books. The fields argument matches similar to example 2 so decide to try using code example 1 in 5.x without the populate:

const author =  await this.authorRepository.find({}, {
  fields: ['name', 'books', 'books.title'],
});

This also returns the same error as what I reported

[B4nan]

That's weird, we even have pretty much the same test case in the codebase:

https://github.com/mikro-orm/mikro-orm/blob/master/tests/features/partial-loading/partial-loading.mysql.test.ts#L40

Maybe try dropping the books from your fields definition. It is virtual property, it does not have any sense in the scope of fields option as it does not represent any actual db column. That seems to be the only difference from the test we already have.

(definitely something to fix, agreed this is quite confusing)

[tsangste]

That's weird, we even have pretty much the same test case in the codebase:

https://github.com/mikro-orm/mikro-orm/blob/master/tests/features/partial-loading/partial-loading.mysql.test.ts#L40

Maybe try dropping the books from your fields definition. It is virtual property, it does not have any sense in the scope of fields option as it does not represent any actual db column. That seems to be the only difference from the test we already have.

(definitely something to fix, agreed this is quite confusing)

Thanks for clarifying that when using a virtual property for a OneToMany that I should not need to have the property name in in fields. So I went back and did some further investigation on this and it seems that it works fine using the JOINED loading strategy (which is support from 5.x onwards).

const author =  await this.authorRepository.find({}, {
  fields: ['name', 'books.title'],
  strategy: LoadStrategy.JOINED,
});

As SELECT_IN is default (also only supported for partial loading in 4.5.x) it seems to fail to load the children into the entity after debugging. Due to lack of knowledge I was able to track that it was querying the data correctly back from postgres within the function populateMany in EntityLoader but when it returns the data back to populateField. The data returned doesnt seemed to be used or there is a bug within populateMany where it is not hydrating the entity?

[B4nan]

That test I linked is using select-in strategy, it works just fine there. You can even see the assertions for queries issued.

The data returned doesnt seemed to be used or there is a bug within populateMany where it is not hydrating the entity?

So if you see some problem with that, provide a reproduction for it. I consider this issue to be just about the 'ignorance of 1:m property in fields'.

But it sounds like you already had that entity loaded in the context. You can't expect partial loading to "erase what was already loaded". If you had that entity fully loaded, partial loading won't strip those "extra properties".

[tsangste]

That test I linked is using select-in strategy, it works just fine there. You can even see the assertions for queries issued.

The data returned doesnt seemed to be used or there is a bug within populateMany where it is not hydrating the entity?

So if you see some problem with that, provide a reproduction for it. I consider this issue to be just about the 'ignorance of 1:m property in fields'.

But it sounds like you already had that entity loaded in the context. You can't expect partial loading to "erase what was already loaded". If you had that entity fully loaded, partial loading won't strip those "extra properties".

I have investigated this further and discovered that when using select_in we have to explicitly include the link property in the children for it to work.

working example

const author =  await this.authorRepository.find(
      {},
      {
        populate: ['books'],
        fields: ['name', 'books.author', 'books.title'],
      },
    );

not working example

const author =  await this.authorRepository.find(
      {},
      {
        populate: ['books'],
        fields: ['name', 'books.title'],
      },
    );

So when not including books.author it was unable to load the items as it was unable to find that field. Also for your tests it included 'books.author' in the field which is why it worked but I guess the main question is that should it continue to work when not including 'books.author' in field? But just a note in 5.x it does work for joined strategy when not including so I would assume you would want both to work the same?

[B4nan]

Well, it is not needed for joined strategy to work, as there is just one query, you dont work with the FK explicilty. For select-in, you need to explicitly load that FK, so we have its value that is required in the following population query.

[tsangste]

Well, it is not needed for joined strategy to work, as there is just one query, you dont work with the FK explicilty. For select-in, you need to explicitly load that FK, so we have its value that is required in the following population query.

If that is the case I am happy with the answer but could you add that part into the documentation that select-in requires its dependent field when using the field find option.

[B4nan]

Let's keep this open, at least the problem in the OP is definitely someting we can easily fix just by ignoring the 1:m property in fields. I'll also look into the other problem, but I remember already trying and it gets much more complicated than it sounds.