chore: refactor findOneComplete to avoid dup

[minottic]

Description

Avoids logic duplication and reuses existing more generic function

Changes:

• refactor findOneComplete

Tests included

• Included for each change/fix?
• Passing?

Documentation

• swagger documentation updated (required for API changes)
• official documentation updated

official documentation info

[emigun]

Looks good to me! Great to avoid duplicated functionality.

(However, I don't fully understand the consequences of your type changes, so I'll let someone else approve)

[HayenNico]

All type changes look good, but I had a couple questions (see below).

This also conflicts with fixes to findOneComplete in #2212 so we need to pick an approach

[HayenNico]

Why remove the fields projection as a valid queryFilter?

[minottic]

I think this test (as the other) was wrong. It does not make sense to ask for these fields and then expect different fiedls to be present in the response IMO

[sbliven]

@HayenNico The 'fields' filter is still present, as tested in other tests (eg 0207). It's used by findAllComplete, so it doesn't need special treatment in findOneComplete.

[sbliven]

Maybe we should keep this test though to document how fields interacts with includes. I guess the previous test was trying to show that the fields property only applied to dataset fields, and is overridden by includes

[HayenNico]

I also think this test case should stay in. It is true that the test doesn't work as expected for fields based on the current master branch, since the lookup was done differently in findOneComplete and findAllComplete (different ordering in aggregation pipeline, definitely a bug). This part was adressed in #2212, so at least the fixes for tests from that PR should be carried over to here.

[HayenNico]

Removing the default changes the behavior in an edge case for the /api/v4/datasets (former fullquery) endpoint, when queryFilter.limits exists but does not include queryFilter.limits.limit. Before the response would always be limited to 10 datasets, with this the response could have arbitrarily many datasets.

I'm not opposed to the change, but then we should be consistent about not assuming default filters

[minottic]

All type changes look good, but I had a couple questions (see below).

This also conflicts with fixes to findOneComplete in #2212 so we need to pick an approach

In principle my PR (a part from the defaults use case that I'll address) only reuses findAllComplete in findOneComplete. This avoids duplication and is conceptually more consistent I think as findOneComplete is just a special case of the findAllComplete. I also think is more intuitive for the user that the 2 endpoints work the same. The difference was only the order of the project stage, which in findOne was applied before the relation, thus I had to change the test. I think for a user this would not make sense. If I ask for the "instrument" relation and then don't include "instrument" in the fields it is expected I believe "instruments" will not be there.

So I would lean towards my PR (a bit biased :) ), as a start and then apply the changes from #2212 as I added in the comment there

[Junjiequan]

If the findOneComplete query can be done with the findAllComplete query, I don’t see why we need it at all.
From the code, they look exactly the same. For findOne, I’d expect to see { where: { _id: pid } }, and for findAll, I’d expect { where: { some set of filters } }.

[minottic]

it's just a utility that sets the filters and this allows me not to change where findOneComplete was historically used. I am not sure why this was ever implemented, but if you follow the old logic (or check the changes of this PR), it does exactly the same. BTW the add addLookupFields is also duplicated in a lot of services and can/should be reused, in a later PR.

Since it's a findOne and not findById, it does not set the ID. I agree the findById could reuse this. When reading the code, I think it would be way easier if for every controller, for the GETs we had essentially 1 function in service reused in all controllers, i.e. this findAllComplete with mongo aggregate. Then we can use it everywwhere, have 1 single entrypoint and even generalize it to work in v3 and v4 (see here). I think this would massively improve maintainability

[HayenNico]

All type changes look good, but I had a couple questions (see below).
This also conflicts with fixes to findOneComplete in #2212 so we need to pick an approach

In principle my PR (a part from the defaults use case that I'll address) only reuses findAllComplete in findOneComplete. This avoids duplication and is conceptually more consistent I think as findOneComplete is just a special case of the findAllComplete. I also think is more intuitive for the user that the 2 endpoints work the same. The difference was only the order of the project stage, which in findOne was applied before the relation, thus I had to change the test. I think for a user this would not make sense. If I ask for the "instrument" relation and then don't include "instrument" in the fields it is expected I believe "instruments" will not be there.

So I would lean towards my PR (a bit biased :) ), as a start and then apply the changes from #2212 as I added in the comment there

I'm on board with this refactor, but I think at least part of #2212 (the bugfix for findOneComplete which led to inconsistent behaviour) should be merged first. A refactor should ideally not touch the tests at all to ensure full compatibility.

@nitrosx @Junjiequan @abdimo101 Do you think it would be better to split #2212 into two PRs, one for the findOneComplete bugfix + testing (which can be merged quickly), and a second for the implicit include logic added? The discussion between using scopes or dot-notation seems like it may take some extra time, which shouldn't block this PR

[Junjiequan]

@HayenNico @nitrosx @abdimo101 I think it's better to merge the PR with bug fix (use correct lookup sequence for all controllers), add corresponding tests and then open an issue to discuss enhanced include logic later on