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.
There was a problem hiding this comment.
Hi @berezovskyi, thanks for the detailed explanation and for taking my solution into account when merging the pull request. Do you have an estimate date when these changes will be officially released?
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. |
|
I tested just now and for us it works well. |
|
@anamrosu great to hear! The release of Lyo 7 is tied to the upgrade to Jena 5. We are on Jena 4 now and no versions below Jena 5 are receiving security updates any more. If we get any help with it, we can release it sooner. We have a PR open to upgrade to Jena 4.10 that needs testing and then we can do the remaining updates needed for Jena 5. #389 I can maybe release a 7.0.0-alpha1 for you if it will help you and if I can find some time to do it. @Jad-el-khoury what do you think about doing an alpha for this patch and Jena 4.10? |
|
Hi @berezovskyi, an Alpha version would be helpful, thanks! |
Description
Allows Query responses to be paged even in presence of multiple ResponseInfo objects.
Checklist
Issues
Closes #605
See: