feat: IDS catalog request filtering #2015

maciejkizlich-zf · 2022-09-26T06:49:08Z

What this PR changes/adds

Extend existing CatalogRequest object with list of criteria used to filter Assets within Catalog response.

Why it does that

In a large scale environment, with multiple EDCs holding the information about hundreds of thousands of Assets, instead of looping thought all of the ContractOffers, there is a need to narrow the results down to even single offer. For example, an Asset might hold the 'special' type of data (like registry or database) and should be searchable by its type.

Linked Issue(s)

Closes #1966

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 · 2022-09-26T07:05:55Z

Codecov Report

Base: 63.73% // Head: 64.33% // Increases project coverage by +0.60% 🎉

Coverage data is based on head (4a98a77) compared to base (d194b9a).
Patch coverage: 64.86% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2015      +/-   ##
==========================================
+ Coverage   63.73%   64.33%   +0.60%     
==========================================
  Files         772      780       +8     
  Lines       16370    16537     +167     
  Branches     1055     1076      +21     
==========================================
+ Hits        10433    10639     +206     
+ Misses       5494     5448      -46     
- Partials      443      450       +7

Impacted Files	Coverage Δ
...type/MultipartCatalogDescriptionRequestSender.java	`7.84% <0.00%> (+0.15%)`	⬆️
...api/multipart/IdsMultipartApiServiceExtension.java	`100.00% <ø> (+23.07%)`	⬆️
.../eclipse/dataspaceconnector/spi/message/Range.java	`54.54% <0.00%> (-31.17%)`	⬇️
...clipse/dataspaceconnector/spi/query/QuerySpec.java	`78.46% <ø> (ø)`
...nnector/spi/contract/offer/ContractOfferQuery.java	`0.00% <0.00%> (ø)`
...ector/contract/offer/ContractOfferServiceImpl.java	`100.00% <100.00%> (ø)`
...tor/catalog/cache/query/BatchedRequestFetcher.java	`100.00% <100.00%> (+11.11%)`	⬆️
...i/multipart/handler/DescriptionRequestHandler.java	`72.00% <100.00%> (-0.55%)`	⬇️
...econnector/ids/api/multipart/util/RequestUtil.java	`80.00% <100.00%> (ø)`
...connector/ids/core/service/CatalogServiceImpl.java	`100.00% <100.00%> (ø)`
... and 48 more

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.

...ct/src/main/java/org/eclipse/dataspaceconnector/contract/offer/ContractOfferServiceImpl.java

...s-core/src/main/java/org/eclipse/dataspaceconnector/ids/core/service/CatalogServiceImpl.java

...java/org/eclipse/dataspaceconnector/ids/api/multipart/handler/DescriptionRequestHandler.java

...ector/ids/api/multipart/dispatcher/sender/type/MultipartCatalogDescriptionRequestSender.java

paullatzelsperger

Please add tests for this new feature. At the very minimum the ContractOfferServiceImplTest needs to be extended. New functionality should always be accompanied by additional tests.

Also, please use var wherever possible, and make use of fluent statements, e.g. with Builders.

paullatzelsperger · 2022-09-27T15:23:39Z

...oint-v1/src/main/java/org/eclipse/dataspaceconnector/ids/api/multipart/util/RequestUtil.java

-                .orElse(defaultValue);
+
+    public static QuerySpec getQuerySpec(@NotNull DescriptionRequestMessage message) {
+        Map specsMap = (Map) ofNullable(message.getProperties()).map(map -> map.get(QuerySpec.QUERY_SPEC)).orElse(Map.of());


PLease use var and inline the next few lines. We have implemented fluency for a reason.

paullatzelsperger · 2022-09-27T15:25:13Z

...-spi/src/main/java/org/eclipse/dataspaceconnector/spi/contract/offer/ContractOfferQuery.java

-
-    public long getLimit() {
-        return limit;
+    public Range getDefinitionsRange() {


as stated before, please rename to range

...rc/test/java/org/eclipse/dataspaceconnector/contract/offer/ContractOfferServiceImplTest.java

paullatzelsperger · 2022-09-29T12:16:22Z

...oint-v1/src/main/java/org/eclipse/dataspaceconnector/ids/api/multipart/util/RequestUtil.java

     */
-    public static int getInt(@NotNull DescriptionRequestMessage message, String propertyName, int defaultValue) {
+    public static QuerySpec getQuerySpec(@NotNull DescriptionRequestMessage message) {


This method needs to be changed. Currently it deserialized the queryspec field into another Map, only to obtain just the RANGE and FILTER_EXPRESSION parameters from it. That is

incorrect, because it ignores the sort field and -order

not necessary, because you could've just deserialized the QuerySpec.

Let's not waste any more time, so here's the working solution:

return ofNullable(message.getProperties()) .map(map -> map.get(QuerySpec.QUERY_SPEC)) .map(specEntry -> objectMapper.convertValue(specEntry, QuerySpec.class)) .orElse(QuerySpec.none());

The objectMapper must be obtained from the runtime wide TypeManager, and QuerySpec#getRange() must be annotated with @JsonIgnore as it is computed property.

You clearly have not tested this, or even tried it out.

ad 1) supporting sort field and order is not in the scope of this change - I wanted to keep it to minimum.
ad 2) you are right, I was just not aware of possibility to use ObjectMapper here - my bad, should've asked/checked.

There is no reason why we would on purpose limit the capability and at the same time increase the impact on the code base. That is the reason why I repeatedly asked to use a QuerySpec and not singular range and filter fields. The QuerySpec was designed for that purpose.

...pi/src/main/java/org/eclipse/dataspaceconnector/spi/types/domain/catalog/CatalogRequest.java

paullatzelsperger · 2022-09-30T09:31:26Z

spi/common/core-spi/src/main/java/org/eclipse/dataspaceconnector/spi/query/QuerySpec.java

@@ -29,6 +29,12 @@
 * is tunnelled through to the database level.
 */
 public class QuerySpec {
+
+    public static final String QUERY_SPEC = "querySpec";
+    public static final String FILTER_EXPRESSION = "filterExpression";


These constants (except QUERY_SPEC) are only used in the DescriptionRequestHandlerTest. Please move them out of the production code base.

paullatzelsperger

please add tests for the RequestUtil#getQuerySpec, then we should be good to go.

maciejkizlich-zf requested review from paullatzelsperger, ronjaquensel and juliapampus as code owners September 26, 2022 06:49

maciejkizlich-zf temporarily deployed to Azure-dev September 26, 2022 06:49 Inactive

github-actions bot added this to In progress in Connector Sep 26, 2022

maciejkizlich-zf temporarily deployed to Azure-dev September 26, 2022 06:55 Inactive

maciejkizlich-zf added 3 commits September 26, 2022 09:03

feat: IDS catalog request filtering

ecd229d

code review changes

d670cc3

code review changes eclipse-edc#2

eb163d6

maciejkizlich-zf force-pushed the feature/idc-catalog-request-filtering branch from 3f52c53 to e9b8161 Compare September 26, 2022 07:04

maciejkizlich-zf temporarily deployed to Azure-dev September 26, 2022 07:06 Inactive

maciejkizlich-zf marked this pull request as draft September 26, 2022 07:07

maciejkizlich-zf force-pushed the feature/idc-catalog-request-filtering branch from e9b8161 to 5da9a9e Compare September 26, 2022 07:33

maciejkizlich-zf temporarily deployed to Azure-dev September 26, 2022 07:34 Inactive

maciejkizlich-zf marked this pull request as ready for review September 26, 2022 07:50

feat: IDS catalog request filtering - fix checks

b4ee3d5

maciejkizlich-zf force-pushed the feature/idc-catalog-request-filtering branch from 5da9a9e to b4ee3d5 Compare September 26, 2022 07:55

maciejkizlich-zf temporarily deployed to Azure-dev September 26, 2022 07:55 Inactive

juliapampus added the enhancement New feature or request label Sep 26, 2022

Connector automation moved this from In progress to Review in progress Sep 26, 2022

ndr-brt approved these changes Sep 26, 2022

View reviewed changes

paullatzelsperger requested changes Sep 26, 2022

View reviewed changes

paullatzelsperger mentioned this pull request Sep 26, 2022

fix: use the range to limit offers, not definitions #2018

Merged

7 tasks

juliapampus mentioned this pull request Sep 27, 2022

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

Closed

7 tasks

maciejkizlich-zf temporarily deployed to Azure-dev September 27, 2022 12:30 Inactive

code review changes

650620b

maciejkizlich-zf force-pushed the feature/idc-catalog-request-filtering branch from 9f4132d to 650620b Compare September 27, 2022 13:09

maciejkizlich-zf temporarily deployed to Azure-dev September 27, 2022 13:09 Inactive

maciejkizlich-zf removed the request for review from juliapampus September 27, 2022 13:20

maciejkizlich-zf requested review from ndr-brt and paullatzelsperger and removed request for ronjaquensel and ndr-brt September 27, 2022 13:20

paullatzelsperger requested changes Sep 27, 2022

View reviewed changes

maciejkizlich-zf temporarily deployed to Azure-dev September 28, 2022 10:40 Inactive

further code review fixes

a58cea0

maciejkizlich-zf force-pushed the feature/idc-catalog-request-filtering branch from b536c23 to a58cea0 Compare September 28, 2022 10:43

maciejkizlich-zf temporarily deployed to Azure-dev September 28, 2022 10:43 Inactive

maciejkizlich-zf requested a review from paullatzelsperger September 28, 2022 11:04

paullatzelsperger requested changes Sep 29, 2022

View reviewed changes

further code review fixes

663f276

maciejkizlich-zf temporarily deployed to Azure-dev September 29, 2022 19:11 Inactive

maciejkizlich-zf requested a review from paullatzelsperger September 29, 2022 19:50

paullatzelsperger reviewed Sep 30, 2022

View reviewed changes

further code review fixes

19d2882

maciejkizlich-zf temporarily deployed to Azure-dev September 30, 2022 10:21 Inactive

maciejkizlich-zf requested a review from paullatzelsperger September 30, 2022 10:37

paullatzelsperger reviewed Sep 30, 2022

View reviewed changes

maciejkizlich-zf temporarily deployed to Azure-dev October 3, 2022 07:42 Inactive

add tests for RequestUtil

4a98a77

maciejkizlich-zf force-pushed the feature/idc-catalog-request-filtering branch from 58eb3be to 4a98a77 Compare October 3, 2022 07:54

maciejkizlich-zf temporarily deployed to Azure-dev October 3, 2022 07:55 Inactive

maciejkizlich-zf requested a review from paullatzelsperger October 3, 2022 08:16

paullatzelsperger approved these changes Oct 4, 2022

View reviewed changes

paullatzelsperger merged commit 4e8252e into eclipse-edc:main Oct 4, 2022

Connector automation moved this from Review in progress to Done Oct 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: IDS catalog request filtering #2015

feat: IDS catalog request filtering #2015

maciejkizlich-zf commented Sep 26, 2022

codecov-commenter commented Sep 26, 2022 •

edited

Loading

paullatzelsperger left a comment

paullatzelsperger Sep 27, 2022

maciejkizlich-zf Sep 28, 2022

paullatzelsperger Sep 27, 2022

maciejkizlich-zf Sep 28, 2022

paullatzelsperger Sep 29, 2022

maciejkizlich-zf Sep 29, 2022 •

edited

Loading

paullatzelsperger Sep 30, 2022

paullatzelsperger Sep 30, 2022

maciejkizlich-zf Sep 30, 2022

paullatzelsperger left a comment

feat: IDS catalog request filtering #2015

feat: IDS catalog request filtering #2015

Conversation

maciejkizlich-zf commented Sep 26, 2022

What this PR changes/adds

Why it does that

Linked Issue(s)

Checklist

codecov-commenter commented Sep 26, 2022 • edited Loading

Codecov Report

paullatzelsperger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maciejkizlich-zf Sep 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paullatzelsperger left a comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 26, 2022 •

edited

Loading

maciejkizlich-zf Sep 29, 2022 •

edited

Loading