feat: handle multiple ResponseInfo obj in Query response #606
Conversation
| if (responseInfos.size() == 1) { | ||
| infoResource = responseInfos.get(0); | ||
| } else if (responseInfos.size() > 1) { | ||
| List<Resource> infos_sameURI = responseInfos.stream().filter(ri -> ri.getURI().equals(query.getQueryUrl())).toList(); |
There was a problem hiding this comment.
would it make more sense to replace "equals" with startsWith?
There was a problem hiding this comment.
Well, there is no strict rule on the ordering of query params, so the substring check does not have a strong meaning. I was thinking to just cut off all the query params and compare the URLs with all params stripped but I wanted to hear from Ana how her testing goes before altering the URIs.
There was a problem hiding this comment.
Hi Andrew, don't know if you saw it already, but please check my feedback on the forum.
Thanks,
Ana
There was a problem hiding this comment.
@anamrosu, thanks! Sad to hear a direct check did not succeed.
I am thinking of keeping the current checks and adding a substring check as a fallback.
Unfortunately, I am really busy this week and will not be able to work on the PR (I do not work on Lyo during my day job). Also, we are now in the release freeze period after Jad published the Lyo 6 release candidate. We won't merge any PRs except critical bugfixes until next week.
@anamrosu if you are up for it, I would be glad if you fork the repo, update my code and submit a new PR. Thank you for helping us improve Lyo!
There was a problem hiding this comment.
@anamrosu just updated the logic, please check.
There was a problem hiding this comment.
Hi Andrew, sorry for not giving you any updates, I was also pretty busy the past weeks. I will be on vacation starting tomorrow for 2 weeks, so I will test your changes after I come back. Thanks!
There was a problem hiding this comment.
Hi @berezovskyi , I tested the changes a few days ago and it was still not working for us. So, I thought about an alternative solution which, in my opinion, should work better. I also tested it in our environment and it works.
In this solution I'm trying to directly look for the responseInfo containg "nextPage" in it. There should be exactly one responseInfo matching this case. I created a new pull request with my proposal, please have a look: #646
There was a problem hiding this comment.
Thanks @anamrosu. I updated my PR. Now the logic goes through the following:
- exactly one ResponseInfo
- otherwise, exactly one ResponseInfo with the nextPage (added in the update)
- otherwise, exactly one ResponseInfo with the exact URI match
- otherwise, exactly one ResponseInfo with the prefix URI match
I could not use your PR directly because:
- you did not sign Eclipse CLA yet: https://api.eclipse.org/git/eca/status/gh/eclipse-lyo/lyo/646 - please create an account with Eclipse and sign the CLA.
- there is a risk that multiple response infos will have the next page (paging is part of OSLC Core, not Query - any valid OSLC Core response may have paging)
- the last page will not have the nextPage but we still need to locate the response info because it contains extra info than just the next page (totalCount, for example - I exposed it in the "sister" PR for oslc4net fix: correct OSLC Query result handling OSLC/oslc4net#203)
- I wanted to add some logging to help with future improvements - your one-shot iteration was a bit hard to use for that.
berezovskyi
force-pushed
the
b605-multiple-responseinfos
branch
2 times, most recently
from
229bf40
to
2d38c0d
Compare
|
berezovskyi
force-pushed
the
b605-multiple-responseinfos
branch
from
2d38c0d
to
8317ffc
Compare
|
Also fixed in OSLC/oslc4net#203 |
…prefix while anomalous, it's not forbidden
|
@Jad-el-khoury @anamrosu please approve the PR if you think we are ready to merge and release this as SNAPSHOT |
|
The logic looks ok to me, assuming it is tested. |
|
@Jad-el-khoury thanks, there is a unit test inside using a real OSLC query response I got from IBM CLM Nordic instance we have access to. @anamrosu please check if 7.0.0-SNAPSHOT works for you as expected. If not, please comment/reopen #605 and we will investigate further. |
Description
Allows Query responses to be paged even in presence of multiple ResponseInfo objects.
Checklist
Issues
Closes #605
See: