Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: add decision record about IDS filtering #1983

Merged
merged 6 commits into from
Sep 22, 2022
Merged

docs: add decision record about IDS filtering #1983

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
249dce5
docs: add decision record about IDS filtering
maciejkizlich-zf Sep 18, 2022
45ee4de
extend decision record with additional info
maciejkizlich-zf Sep 19, 2022
3395ea6
add considerations and limitations section
maciejkizlich-zf Sep 19, 2022
d1604e9
apply proposed changes
maciejkizlich-zf Sep 21, 2022
5533d85
apply proposed changes
maciejkizlich-zf Sep 21, 2022
71cf183
add bit about reusing ContractOfferQuery
maciejkizlich-zf Sep 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions docs/developer/decision-records/2022-09-18-ids-requests-filtering/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
# Add filtering to IDS requests

## Decision

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

## Rationale

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, and
`Asset` might hold the 'special' type of data (like registry or database) and should be searchable by its type.

## Approach
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO this section can be shortened because only two things need to be done:

  • replace the use of Range (for paging) with QuerySpec, for full querying capabilities
  • swap out the (flattened) serialization of the Range with object serialization of the QuerySpec

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Member

@paullatzelsperger paullatzelsperger Sep 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

nope, not quite :) Please highlight that transmitting the Range needs to be replaced by transmitting the entire QuerySpec. Filtering is only one aspect of it. Sorting is another.

Copy link
Member

@ndr-brt ndr-brt Sep 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@paullatzelsperger I'm not sure that introducing QuerySpec is the way to go, since the "range" part actually doesn't limit ContractOffers, but ContractDefinition (if you request a catalog with 50 item you could end up receiving more than 50 contract offers, because of the flatMap behavior in the service), while the criteria part would filter only the Assets.
In my opinion the ContractOfferQuery object should be used, adding definitionsRange and assetCriteria fields, this will make the process explicit

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@paullatzelsperger what do you think about @ndr-brt reasoning? I agree with such argument.

Copy link
Member

@paullatzelsperger paullatzelsperger Sep 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ndr-brt @maciejkizlich-zf here's a weird thing: ContractOfferQuery.getCriteria(), .getOffset() and .getLimit() appear not to be used anywhere, at least IntelliJ doesn't report any findings.
Could we just use those fields? We might even be able to remove the range parameter from the ContractOfferServiceImpl.queryContractOffers then.

I agree with the opinion that query parameters should affect the list of assets/offers, not the definitions. If that's the case it needs to get fixed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@maciejkizlich-zf could you adapt your PR as per my last comment regarding the ContractOfferQuery?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@paullatzelsperger please take a look now, I've added a part regarding ContractOfferQuery.

And yes, we should be able to remove Range from the queryContractOffers(). Also, I'd replace .getOffset and getLimit() with just getRange().


As the logic for querying `AssetIndex` by `QuerySpec` is already in place, all we have to do is to pass filtering criteria via IDS Api:


`filter` property has to be added to the IDS message:

```
// in MultiPartCatalogDescriptionRequestSender.java
message.setProperty(CatalogRequest.FILTER, request.getFilter());
```

Then, on the receiving end, it's simply extracted and converted to the list of `Criteria`:

```
// in DescriptionRequestHandler.java
Optional<Object> maybeFilter = Optional.ofNullable(message.getProperties().get(CatalogRequest.FILTER));

if (maybeFilter.isPresent()) {
criterions = objectMapper.convertValue(maybeFilter.get(), new TypeReference<>() {});
}
```

And now, they can be applied to `AssetIndex` search query when constructing the `Catalog` response:

1) pass Criterions to the `CatalogServiceImpl.getDataCatalog()`
2) attach to the existing `ContractOfferQuery` object
3) in `ContractOfferServiceImpl.queryContractOffers()`, merge with `AssetSelectorExpressions` from contract definitions.


1 change: 1 addition & 0 deletions docs/developer/decision-records/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,3 +36,4 @@
- [2022-08-09 Project structure review](2022-08-09-project-structure-review/)
- [2022-08-11 Versioning and Artifacts](2022-08-11-versioning_and_artifacts/)
- [2022-08-17 Remove H2 Database Tests](2022-08-17-remove_h2_database_tests/)
- [2022-09-18 IDS requests filtering](2022-09-18-ids-requests-filtering/)