SLR : Adding Routing Dispatch to StudyFetcher#16098
Conversation
PR Summary by QodoAdd catalog-specific routing dispatch to StudyFetcher (raw query bypass) Description
Diagram
High-Level Assessment
Files changed (2)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| String catalogOverride = searchQuery.getCatalogSpecific().get(fetcher.getName()); | ||
| if (fetcher instanceof PagedSearchBasedFetcher basedFetcher) { | ||
| int limit = resultLimits.getOrDefault(fetcher.getName(), StudyRepository.DEFAULT_RESULT_LIMIT); | ||
| int pages = (int) Math.ceil((double) limit / basedFetcher.getPageSize()); | ||
| for (int page = 0; page < pages; page++) { | ||
| fetchResult.addAll(basedFetcher.performSearchPaged(searchQuery.getQuery(), page).getContent()); | ||
| } | ||
| if (fetchResult.size() > limit) { | ||
| fetchResult = new ArrayList<>(fetchResult.subList(0, limit)); | ||
| if (catalogOverride != null) { | ||
| fetchResult.addAll(basedFetcher.performRawSearchQueryPaged(catalogOverride, 0).getContent()); | ||
| } else { | ||
| int limit = resultLimits.getOrDefault(fetcher.getName(), StudyRepository.DEFAULT_RESULT_LIMIT); | ||
| int pages = (int) Math.ceil((double) limit / basedFetcher.getPageSize()); | ||
| for (int page = 0; page < pages; page++) { | ||
| fetchResult.addAll(basedFetcher.performSearchPaged(searchQuery.getQuery(), page).getContent()); | ||
| } | ||
| if (fetchResult.size() > limit) { | ||
| fetchResult = new ArrayList<>(fetchResult.subList(0, limit)); | ||
| } | ||
| } | ||
| } else { | ||
| fetchResult = fetcher.performSearch(searchQuery.getQuery()); | ||
| if (catalogOverride != null) { | ||
| fetchResult = fetcher.performRawSearchQuery(catalogOverride); | ||
| } else { | ||
| fetchResult = fetcher.performSearch(searchQuery.getQuery()); | ||
| } | ||
| } | ||
| return new FetchResult(fetcher.getName(), new BibDatabase(fetchResult)); | ||
| } catch (FetcherException e) { |
There was a problem hiding this comment.
3. Uncaught raw-query exceptions 🐞 Bug ☼ Reliability
StudyFetcher calls performRawSearchQuery/performRawSearchQueryPaged for catalogSpecific overrides but only catches FetcherException; the default interface implementations throw UnsupportedOperationException, which will escape and abort crawl processing. This makes a study.yml override capable of crashing a crawl instead of cleanly skipping/falling back for an unmigrated fetcher.
Agent Prompt
## Issue description
`StudyFetcher.performSearchOnQueryForFetcher` routes catalog-specific overrides to `performRawSearchQuery(...)` / `performRawSearchQueryPaged(...)`, but those raw methods default to throwing `UnsupportedOperationException` unless the fetcher overrides them. `StudyFetcher` only catches `FetcherException`, so the unchecked exception can propagate and abort the crawl.
## Issue Context
- `SearchBasedFetcher.performRawSearchQuery` and `PagedSearchBasedFetcher.performRawSearchQueryPaged` have default implementations that throw `UnsupportedOperationException`.
- A user can add `catalog-specific` entries in `study.yml` for a catalog that isn’t migrated to raw-query methods.
## Fix Focus Areas
- Add guarded execution + fallback/skip on `UnsupportedOperationException` (and possibly other runtime exceptions coming from fetchers) around the raw-query dispatch.
- Decide on policy: either **fallback to standard search path** when raw is unsupported, or **log and skip only that fetcher** (but do not crash the whole crawl).
- Update/add a unit test to cover this scenario (override present + fetcher not implementing raw => no crash).
### Code pointers
- jablib/src/main/java/org/jabref/logic/crawler/StudyFetcher.java[59-87]
- jablib/src/main/java/org/jabref/logic/importer/SearchBasedFetcher.java[42-45]
- jablib/src/main/java/org/jabref/logic/importer/PagedSearchBasedFetcher.java[31-34]
- jablib/src/test/java/org/jabref/logic/crawler/StudyFetcherTest.java[1-79]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
LoayTarek5
left a comment
LoayTarek5
left a comment
There was a problem hiding this comment.
small things to address, great works as usual
| private FetchResult performSearchOnQueryForFetcher(StudyQuery searchQuery, SearchBasedFetcher fetcher) { | ||
| try { | ||
| List<BibEntry> fetchResult = new ArrayList<>(); | ||
| String catalogOverride = searchQuery.getCatalogSpecific().get(fetcher.getName()); |
There was a problem hiding this comment.
this case sensitive lookup, but catalog and fetcher names match ignoring case, for example an override keyed ieeexplore silently misses and falls back to the translated query, i think it's good to addcase insensitive lookup to stay consistent with how catalog names are matched everywhere else
| } | ||
| return new FetchResult(fetcher.getName(), new BibDatabase(fetchResult)); | ||
| } catch (FetcherException e) { | ||
| } catch (FetcherException | UnsupportedOperationException e) { |
There was a problem hiding this comment.
I think it wraps the whole try so a "UnsupportedOperationException" on the standard path also gets hidden as "API request failed", maybe better to scope it to just the raw call
also an override on unmigrated catalog returns zero results with only a log line is that intended, or should it fall back to the normal query?
There was a problem hiding this comment.
also an override on unmigrated catalog returns zero results with only a log line is that intended, or should it fall back to the normal query?
It's intentional behavior since falling back silently would be misleading to the user
github-actions
Bot
added
status: changes-required
LoayTarek5
left a comment
LoayTarek5
left a comment
There was a problem hiding this comment.
Also i notice that two behaviors are not covered
one for an override fetching multiple pages, respecting the limit
and other of an override on an unmigrated fetcher not crashing the crawl
|
I still need to update the tests to use the same pattern as |
github-actions
Bot
added
status: no-bot-comments
and removed
status: changes-required
| .findFirst() | ||
| .orElse(null); |
There was a problem hiding this comment.
what if a study.yml entry has a key but no value, i think this throws NullPointerException, Optional can't wrap null, also blank value ("") is non-null too, so it would send an empty raw query and return nothing
| if (catalogOverride != null) { | ||
| int limit = resultLimits.getOrDefault(fetcher.getName(), StudyRepository.DEFAULT_RESULT_LIMIT); | ||
| int pages = (int) Math.ceil((double) limit / basedFetcher.getPageSize()); | ||
| try { | ||
| for (int page = 0; page < pages; page++) { | ||
| fetchResult.addAll(basedFetcher.performRawSearchQueryPaged(catalogOverride, page).getContent()); | ||
| } | ||
| } catch (UnsupportedOperationException e) { | ||
| LOGGER.warn("{} does not support raw search queries", fetcher.getName(), e); | ||
| fetchResult = new ArrayList<>(); | ||
| } | ||
| if (fetchResult.size() > limit) { | ||
| fetchResult = new ArrayList<>(fetchResult.subList(0, limit)); | ||
| } | ||
| } else { | ||
| int limit = resultLimits.getOrDefault(fetcher.getName(), StudyRepository.DEFAULT_RESULT_LIMIT); | ||
| int pages = (int) Math.ceil((double) limit / basedFetcher.getPageSize()); | ||
| for (int page = 0; page < pages; page++) { | ||
| fetchResult.addAll(basedFetcher.performSearchPaged(searchQuery.getQuery(), page).getContent()); | ||
| } | ||
| if (fetchResult.size() > limit) { | ||
| fetchResult = new ArrayList<>(fetchResult.subList(0, limit)); | ||
| } |
There was a problem hiding this comment.
These almost the same,only the method call differs, so maybe it better like collapsing into one loop that just picks the call.
There was a problem hiding this comment.
I think it's better to keep these loops separate for easier readability. The code duplication is minor anyways.
| when(pagedFetcher.performRawSearchQueryPaged(anyString(), anyInt())) | ||
| .thenReturn(new Page<>("native:query", 0, List.of())); | ||
| when(pagedFetcher.performRawSearchQueryPaged("native:query", 0)) | ||
| .thenReturn(new Page<>("native:query", 0, List.of(new BibEntry()))); | ||
|
|
There was a problem hiding this comment.
The paged tests only page 0, so the multi-page loop (the thing that was fixed) isn't actually verified
| } catch (UnsupportedOperationException e) { | ||
| LOGGER.warn("{} does not support raw search queries", fetcher.getName(), e); | ||
| fetchResult = new ArrayList<>(); | ||
| } |
There was a problem hiding this comment.
when an override a catalog not migrated to raw, it returns zero results for that query, for slr, should it fall back to the standard query instead of dropping the catalog?
There was a problem hiding this comment.
you are right, i agree that silent fallback would mislead, but the current path which zero results with just a log line means the since the user can't tell it happened(from what i understand)
for example let it throw a FetcherException to be visible, or record it in the study output, either way the user should know the catalog couldn't run their raw query.
There was a problem hiding this comment.
feel free to correct me, i may be missing something
github-actions
Bot
added
status: changes-required
github-actions
Bot
added
status: no-bot-comments
status: changes-required
faneeshh
force-pushed
the
slr-routing-dispatch
branch
from
edcd873 to
e48d353
Compare
github-actions
Bot
added
status: no-bot-comments
and removed
status: changes-required
| .map(Map.Entry::getValue) | ||
| .findFirst() | ||
| .filter(s -> !s.isBlank()) | ||
| .orElse(null); |
There was a problem hiding this comment.
the blank string case is handled now, but the null case is still there: findFirst() throws the NPE before the .filter runs, so study.yml entry with a key and no value still crashes,
the guard needs to be in the stream filter, before findFirst()
There was a problem hiding this comment.
How did I miss that... Should be good now!
github-actions
Bot
added
status: changes-required
github-actions
Bot
added
status: no-bot-comments
and removed
status: changes-required
LoayTarek5
left a comment
LoayTarek5
left a comment
There was a problem hiding this comment.
LGTM to me, great and solid work, @faneeshh
| .map(Map.Entry::getValue) | ||
| .filter(v -> v != null && !v.isBlank()) | ||
| .findFirst() | ||
| .orElse(null); |
There was a problem hiding this comment.
no null in the code. -- Use Optionals. ispresent check seems to be ok in this case.
The blocks below could be split in sub methods.
github-actions
Bot
added
status: changes-required
github-actions
Bot
added
status: no-bot-comments
status: changes-required
|
Your pull request conflicts with the target branch. Please merge with your code. For a step-by-step guide to resolve merge conflicts, see https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/addressing-merge-conflicts/resolving-a-merge-conflict-using-the-command-line. |
3 participants
Related issues and pull requests
SLR related routing work.
PR Description
StudyFetcher previously sent every query through the standard search path regardless of catalog. This PR adds routing dispatch in
performSearchOnQueryForFetcher: if aStudyQueryhas acatalogSpecificoverride for the active fetcher, the raw query string is sent directly viaperformRawSearchQuery/performRawSearchQueryPaged, bypassing the query transformer.If no override is present the standard path runs unchanged. This makes the
catalogSpecificfield instudy.ymlactually functional end-to-end.Analogies
Like honey, this PR is specific to its source — each catalog gets exactly the query it was meant to receive, not a generic one-size-fits-all version. Like chocolate, it works in two forms (paged and plain) while tasting the same to the researcher writing their study definition. Like the moon, it only shows one face at a time — either the raw path or the standard path fires, never both.
Steps to test
AI usage
Claude Sonnet 4.6
Checklist
CHANGELOG.mdin a way that can be understood by the average user (if change is visible to the user)