Transparently ensure foreign key target field in inclusion resolvers

[InvictusMB]

findByForeignKeys adds the foreign key to the filter fields if it's not there.
Concerned inclusion resolvers prune the foreign key value from entities if it has not been requested in the original filter.

Fixes: #5314
Reasoning: comment
Obsoletes: #3455
Also addresses: comment

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

• npm test passes on your machine
• New tests added or existing tests modified to cover all changes
• Code conforms with the style guide
• API Documentation in code was updated
• Documentation in /docs/site was updated
• Affected artifact templates in packages/cli were updated
• Affected example projects in examples/* were updated

👉 Check out how to submit a PR 👈

[agnes512]

Thank you for the PR! I think once it is landed it would filter.fields more straightforward 👍 Before that, I have several questions would like to discuss.

[agnes512]

I'd like hear from @bajtos before landing. Except the normalizeFields function, the PR looks good to me in general. It is more intuitive when handling fields filter with nested relations.

Do you mind also update the bottom part of the docs https://loopback.io/doc/en/lb4/Fields-filter.html that excluding the source key for relations might cause errors? Thanks!

[InvictusMB]

Do you mind also update the bottom part of the docs https://loopback.io/doc/en/lb4/Fields-filter.html that excluding the source key for relations might cause errors? Thanks!

That's not necessary anymore. The latest commit addresses source keys too.

[InvictusMB]

@raymondfeng @agnes512 @hacksparrow
Could you guys check what else do we need for this to land?

[agnes512]

Sorry for the delay. Good progress 👍 I think the API docs could be more descriptive.

[agnes512]

I am not sure if we should have || null here. I think the empty value is handled before retuning here?

[InvictusMB]

Not really, we can get an empty array and we guard against undefined here.

[bajtos]

Thank you @InvictusMB for the pull request 👏 and @agnes512 for reviewing the changes 💪

I need to find out more time to build a better understanding of the wider context and asses the implications of the proposed changes. I'll try to do that early next week 🤞 Sorry for the delay! 🙈

[bajtos]

Thank you @InvictusMB for the pull request. I like the idea of improving inclusion resolves to include primary/foreign key properties in the fields used to build the database query 👍

I have difficulties understanding your proposed implementation. You are changing a lot of files and there is no high-level description of the target design you are aiming for. Can you please explain the proposed change in English? In particular, I am confused about pruneFields and why we need resolvers to expose relationMeta. I would expect resolvers to be fully encapsulated and if they need to do some processing of the query filter, then such step should happen inside the resolvers (ideally).

[InvictusMB]

@bajtos Sorry, the PR grew out of control. Initially it was only about targets of a relation.
Here are the key points:

• We have to detect all the fields that are involved in all of the inclusions of a particular filter and ensure those fields are included in a filter we use to resolve both the target entity and the inclusions.
• We fill the targetKey in a resolver.
• We fill the sourceKey in find* methods of a repository.
• Pruning empties the fields that were added to the filter here but had not been requested originally or had been disabled intentionally.
• We must expose relation metadata because there are two sides to the relation. Resolver only knows about the target entity it resolves and it can only fill the targetKey. It receives the source entities as is and if the sourceKey was filtered out there is no way to get it back without an extra query. That's why right now sourceKey has to be handled in find* methods of a repository before passing entities into a resolver.

I don't think we can fully encapsulate this behavior inside a resolver the way you wish.
We could probably do something very clever and delay the source entity resolution until we collect all the filter amendments from inclusion resolvers via callbacks. But I'm not sure this async ping pong would be comprehensible.

Very clever code

  async find(
    filter?: Filter<T>,
    options?: Options,
  ): Promise<(T & Relations)[]> {
    const include = filter?.include;

    const entityResolver = async (newFilter?: Filter<T>) => {
      const models = await ensurePromise(
        this.modelClass.find(this.normalizeFilter(newFilter), options),
      );
      return this.toEntities(models);
    }

    let amendFilter;
    const entityPromise = new Promise(resolve => {
      let count = 0;
      amendFilter = (newFilter: Filter<T>) => {
        if (count === include?.length) {
          resolve(entityResolver(newFilter));
        }
        count++;
      };
    });

    return this.includeRelatedModels(
      entityPromise,
      amendFilter,
      include,
      options,
    );
  }

[InvictusMB]

BTW would you mind to extract the implementation of ensureFields including the tests to a new pull request, so that we can land that building block first and then have less code to review when discussing how to fix inclusion resolvers?

Separated into #6495

[InvictusMB]

FK source part split into #6496

[InvictusMB]

@agnes512 @hacksparrow
Could you review once more? I think it's good to merge now.

#6495 has to land first though.

[stale]

This pull request has been marked stale because it has not seen activity within two months. It will be closed within 14 days of being stale unless there is new activity.

[stale]

This pull request has been marked stale because it has not seen activity within two months. It will be closed within 14 days of being stale unless there is new activity.

[InvictusMB]

Not stale. I'll have time to refresh this soon.

[stale]

This pull request has been marked stale because it has not seen activity within two months. It will be closed within 14 days of being stale unless there is new activity.

[stale]

This pull request has been closed due to continued inactivity. If you are interested in finishing the proposed changes, then feel free to re-open this pull request or open a new one.