fix(ContractOfferServiceImpl): check for existing criteria list in AssetSelectorExpression

[juliapampus]

What this PR changes/adds

Moves/Adds business logic to ContractOfferServiceImpl:
(1) Query Assets with QuerySpec.none() if AssetSelectorExpression == AssetSelectorExpression.SELECT_ALL
(2) Filter ContractDefinitions with empty or null criteria list if AssetSelectorExpression != AssetSelectorExpression.SELECT_ALL

Removes check (1) from InMemoryAssetIndex and CosmosAssetIndex.

Why it does that

For database queries, no filter criteria means "select all" (equal to "*"). In terms of data sovereignty and our domain model, ContractDefinitions that provide no selector expression are not pointing to an Asset and should therefore not result in a valid ContractOffer as part of the conntector's Catalog.

Further notes

Unit tests added for InMemoryAssetIndex. As the logic is added to the ContractOfferServiceImpl, this behavior will exist for all AssetIndex implementations.

Tested locally with sample 4.0: Registering a ContractDefinition in FileTransferExtension with an empty or null criteria list results in a catalog with 0 contractOffers during catalog querying. (This was not the case before the added filter.)

Local style check hinted to missing license headers and redundant line spaces --> fixed this.

Linked Issue(s)

Closes #2009

Checklist

• added appropriate tests?
• performed checkstyle check locally?
• added/updated copyright headers?
• documented public classes/methods?
• added/updated relevant documentation?
• assigned appropriate label? (exclude from changelog with label no-changelog)
• formatted title correctly? (take a look at the CONTRIBUTING and styleguide for details)

[codecov-commenter]

Codecov Report

Base: 64.33% // Head: 64.33% // Decreases project coverage by -0.00% ⚠️

Coverage data is based on head (044cded) compared to base (4e8252e).
Patch coverage: 88.88% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2019      +/-   ##
==========================================
- Coverage   64.33%   64.33%   -0.01%     
==========================================
  Files         780      780              
  Lines       16537    16539       +2     
  Branches     1076     1076              
==========================================
+ Hits        10639    10640       +1     
  Misses       5448     5448              
- Partials      450      451       +1     

Impacted Files	Coverage Δ
...lplane/defaults/assetindex/InMemoryAssetIndex.java	92.45% <ø> (-0.28%)	⬇️
...am/oauth2/core/Oauth2DefaultServicesExtension.java	0.00% <ø> (ø)
...oauth2/core/rule/Oauth2AudienceValidationRule.java	100.00% <ø> (ø)
...e/rule/Oauth2ExpirationIssuedAtValidationRule.java	93.75% <ø> (ø)
...auth2/core/rule/Oauth2NotBeforeValidationRule.java	100.00% <ø> (ø)
...agement/contractdefinition/model/CriterionDto.java	100.00% <ø> (ø)
...ceconnector/assetindex/azure/CosmosAssetIndex.java	88.57% <ø> (-0.62%)	⬇️
...ector/contract/offer/ContractOfferServiceImpl.java	96.55% <88.88%> (-3.45%)	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[juliapampus]

As discussed, potentially blocked by #2015 and blocking #2018, as all changes address the same method.

[paullatzelsperger]

LGTM. I'll just rebase #2018 and apply your added checks analogously.

[ndr-brt]

I'm absolutely in favor of pull out the AssetSelectorExpression evaluation out of the repository (I would also inline the Stream<Asset> queryAssets(AssetSelectorExpression expression); method, as the AssetSelectorExpression is something related to the ContractDefinition and that it should be converted to a QuerySpec before being used to fetch assets.

But the SELECT_ALL is only a constant that is actually not used (it's impossible to generate it a runtime), so personally I would remove it completely.

[ndr-brt]

I think this filter does nothing, as when the criteria field is empty, then the selectorExpression equals to SELECT_ALL. Because SELECT_ALL means no criterion are provided.
(this pass:)

    @Test
    void equality() {
        assertThat(AssetSelectorExpression.Builder.newInstance().build()).isEqualTo(AssetSelectorExpression.SELECT_ALL);
    }

Furthermore, the selectorExpression of the definition cannot be null.

[juliapampus]

The selectorExpression cannot be null but the criteriaList can. There is no additional check in the builder class that validates whether the expresion is SELECT_ALL and otherwise the criteria list cannot be empty or null.

[juliapampus]

Because SELECT_ALL means no criterion are provided.

This should not be the case. SELECT_ALL means "select all". No criteria means "select none".

[ndr-brt]

Pay attention on using == to equal on object instances, however I see no use for this if block

[juliapampus]

This if block adds the logic that SELECT_ALL overwrites any filters as it applies a ContractDefinition to any Asset. As above, the method adds logic that the AssetSelectorExpression does not provide itself.

[juliapampus]

I'm absolutely in favor of pull out the AssetSelectorExpression evaluation out of the repository (I would also inline the Stream<Asset> queryAssets(AssetSelectorExpression expression); method, as the AssetSelectorExpression is something related to the ContractDefinition and that it should be converted to a QuerySpec before being used to fetch assets.

But the SELECT_ALL is only a constant that is actually not used (it's impossible to generate it a runtime), so personally I would remove it completely.

As I originally did not implement this AssetSelectorExpression, I did not want to remove provided functionality within this PR. However, I agree that this class could be implemnted differently. Nevertheless, as we interpret missing criteria as "select none", we should provide a possibility to explicitly "select all" without the need to provide a list of assetIds.

[github-actions]

Test Results

     431 files  ±0       431 suites  ±0   18m 5s ⏱️ -8s
12 692 tests +2  12 656 ✔️ +2  36 💤 ±0  0 ❌ ±0 
12 755 runs  +3  12 718 ✔️ +3  37 💤 ±0  0 ❌ ±0 

Results for commit 044cded. ± Comparison against base commit 4e8252e.

[juliapampus]

Did not manage to get the QL workflow working again.... new PR: #2044