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.

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

Conversation

maciejkizlich-zf
Copy link
Contributor

What this PR changes/adds

Add decision record about filtering ContractOffers by Asset's properties.

Linked Issue(s)

Filter catalog by assets properties

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)

Copy link
Member

@paullatzelsperger paullatzelsperger left a comment

Choose a reason for hiding this comment

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

comments inline.

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

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

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

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().

@paullatzelsperger paullatzelsperger added the documentation Improvements or additions to documentation label Sep 19, 2022
@maciejkizlich-zf maciejkizlich-zf requested review from paullatzelsperger and removed request for juliapampus September 19, 2022 11:12
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

@paullatzelsperger paullatzelsperger Sep 19, 2022

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
Contributor

@juliapampus juliapampus left a comment

Choose a reason for hiding this comment

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

@paullatzelsperger We will just ignore the fact that there is an IDS message type in place that is designed for passing query statements, right? Paging was an issue, but querying has already been considered, even before to the current discussions.

@paullatzelsperger
Copy link
Member

paullatzelsperger commented Sep 20, 2022

@paullatzelsperger We will just ignore the fact that there is an IDS message type in place that is designed for passing query statements, right? Paging was an issue, but querying has already been considered, even before to the current discussions.

@juliapampus Which message would that be? Generally though, we're adding features to a stack that will get replaced in the very near future (i.e. IDS Multipart), so I would like to keep the change small and simple

@juliapampus
Copy link
Contributor

@paullatzelsperger We will just ignore the fact that there is an IDS message type in place that is designed for passing query statements, right? Paging was an issue, but querying has already been considered, even before to the current discussions.

@juliapampus Which message would that be? Generally though, we're adding features to a stack that will get replaced in the very near future (i.e. IDS Multipart), so I would like to keep the change small and simple

For the record: We discussed this bilateral. Correct IDS messages would be QueryMessage with ResultMessage as response. This would end up in a separate sender and handler class which would be a cleaner approach, but goes beyond the current scope.

@maciejkizlich-zf maciejkizlich-zf requested review from juliapampus and paullatzelsperger and removed request for juliapampus September 21, 2022 09:23
@paullatzelsperger paullatzelsperger merged commit 3e46d70 into eclipse-edc:main Sep 22, 2022
@maciejkizlich-zf maciejkizlich-zf deleted the feature/decision-record-ids-filtering branch September 22, 2022 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants