Implemented PagedSearchBasedFetcher Of ScholarApi for List API - #16293
Implemented PagedSearchBasedFetcher Of ScholarApi for List API#16293thesauravpoddar wants to merge 19 commits into
Conversation
…ey method relying on the pre-defined method
github-actions
Bot
added
good first issue
PR Summary by QodoAdd ScholarApi-backed paged “Scholar” web search fetcher
AI Description
Diagram
High-Level Assessment
Files changed (11)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
github-actions
Bot
added
the
status: changes-required
|
fetcher tests are failing not just for the ScholarApi fetcher but others too i tried to look for this i think it has to do something with as it should be |
|
heyy @kopper, @Siedlerchr |
|
Will look at this soon. Tagging our student team as well in the meanwhile as it seems to be a larger PR. |
Ok, sure, i will review it |
|
Hii @subhramit i just want to know if you've had any chance to review it? If there's anything to change or update I'd glad to do that |
Hey Saurav, I'll get to it soon. The team of maintainers is currently focusing on a set of milestones for an upcoming release, hence the delay. |
Okay got it |
| authorsList.add(authors.getString(i)); | ||
| } | ||
| if (!authorsList.isEmpty()) { | ||
| entry.setField(StandardField.AUTHOR, String.join(" and ", authorsList)); |
There was a problem hiding this comment.
| entry.setField(StandardField.AUTHOR, String.join(" and ", authorsList)); | |
| entry.withField(StandardField.AUTHOR, String.join(" and ", authorsList)); |
There was a problem hiding this comment.
But why did you recommend to use the withField? Because we don't really use the result of the withField, so setField should be fine also
| entry.withField(StandardField.YEAR, publishedDateOnly.split("-")[0]); | ||
|
|
||
| if (scholarJsonEntry.has("id")) { | ||
| entry.setField(new UnknownField("scholarapi-id"), scholarJsonEntry.getString("id")); |
There was a problem hiding this comment.
Same for this. Use withField to keep the pattern consistent.
| if (scholarJsonEntry.has("id")) { | ||
| entry.setField(new UnknownField("scholarapi-id"), scholarJsonEntry.getString("id")); | ||
| } | ||
| // doi |
There was a problem hiding this comment.
No need to document self explanatory code. Remove these comments
| try { | ||
| URLDownload urlDownload = new URLDownload(getTestUrl()); | ||
| urlDownload.addHeader("X-API-Key", apiKey); | ||
| urlDownload.asInputStream().close(); |
| throw new URISyntaxException(LIST_URL, | ||
| "Page " + pageNumber + " was requested before its cursor was available; pages must be fetched sequentially"); |
There was a problem hiding this comment.
Here the list URL is not the problem so change the exception message or throw Fetcher Exception
|
|
||
| @Override | ||
| protected String handleJournal(String journalTitle) { | ||
| LOGGER.warn("ScholarAPI has no journal scoped search"); |
There was a problem hiding this comment.
| LOGGER.warn("ScholarAPI has no journal scoped search"); | |
| LOGGER.debug("ScholarAPI has no journal scoped search"); |
| import static org.mockito.Mockito.when; | ||
|
|
||
| @FetcherTest | ||
| public class ScholarApiFetcherTest implements SearchBasedFetcherCapabilityTest, PagedSearchFetcherTest { |
There was a problem hiding this comment.
Change class name to ScholarFetcherTest. We name classes according to the class under test and I believe here that is Scholar Fetcher
| | [Zentralblatt Math](https://www.zbmath.org) | (none) | (none) | Depending on the current network | | ||
|
|
||
| "Depending on the current network" means that it depends on whether your request is routed through a network having paid access. For instance, some universities have subscriptions to MathSciNet. | ||
| | [ScholarApi](https://scholarapi.net/) | [ScholarApi User Account](https://scholarapi.net/auth/register) | `ScholarApiKey` | 1000 free credits per key | |
There was a problem hiding this comment.
@faneeshh check if the placement is fixed, I don't see this comment outdated as well - I'm having to fish for unaddressed comments
There was a problem hiding this comment.
Yeah it's fine now. However, this PR got too long for me to keep up.
LoayTarek5
left a comment
LoayTarek5
left a comment
There was a problem hiding this comment.
@thesauravpoddar good work, keep going.
the issue #16233 also asks for a FulltextFetcher (GET /pdf/{id}), or i miss something in past conversation?
also missing CHANGELOG.md entry.
| protected String handleJournal(String journalTitle) { | ||
| LOGGER.warn("ScholarAPI has no journal scoped search"); | ||
| return ""; |
There was a problem hiding this comment.
i think that "" breaks the query, try use return StringUtil.quoteStringIfSpaceIsContained(journalTitle)
| } | ||
| // Journal issue | ||
| if (scholarJsonEntry.has("journal_issue")) { | ||
| entry.withField(StandardField.NUMBER, scholarJsonEntry.getString("journal_issue")); |
There was a problem hiding this comment.
it seems that journal_volume : VOLUME looks missing, right?, also pick setField or withField, not both.
There was a problem hiding this comment.
@LoayTarek5 there is no specific filed for journal volume thats why it is missing
There was a problem hiding this comment.
i have checked it, the journal_issue returns "Volume 60, Issue 3" on every record, so volume isnot missing, it's inside journal_issue, but you map that whole string into NUMBER, so entries get number = "Volume 60, Issue 3" and empty volume.
try to split it, like parse volume => VOLUME, issue => NUMBER, also i see that still mixing setField/withField
There was a problem hiding this comment.
i have checked it, the journal_issue returns "Volume 60, Issue 3" on every record, so volume isnot missing, it's inside journal_issue, but you map that whole string into NUMBER, so entries get number = "Volume 60, Issue 3" and empty volume. try to split it, like parse volume => VOLUME, issue => NUMBER, also i see that still mixing setField/withField
@LoayTarek5
yeah about that volume i too saw that but in schema that is give on scholar api there is no any journal volume specific field adding that won't break it? because when json came from ScholarApi converts to BibText it will expect a journal "Volume 60, Issue 3 for this i just show 3
There was a problem hiding this comment.
maybe i miss something, i dont konw, but keeping just the 3 still loses the "60", you don't need a schema field for volume, put it in JabRef's own StandardField.VOLUME, the issue in NUMBER so "Volume 60, Issue 3" => VOLUME = 60, NUMBER = 3, ithink it won't break anything, just fills a field that's empty now( the MAINTAINERS can confirm this)
There was a problem hiding this comment.
Okay understood i will parse that and add separate fields for journal volume
| import org.slf4j.LoggerFactory; | ||
|
|
||
| public class ScholarFetcher implements PagedSearchBasedFetcher, CustomizableKeyFetcher { | ||
| public static final String FETCHER_NAME = "Scholar"; |
There was a problem hiding this comment.
i think it is wrong, yse "ScholarAPI" / ScholarApi*(correct me if i am wrong),also ScholarQueryTransformerTest already exists, so your tests filter runs the old one
There was a problem hiding this comment.
@LoayTarek5 the class name is ScholarApiQueryTransformerTest not ScholarQueryTransformerTest and I'll update the fetcher name
There was a problem hiding this comment.
yeah, you're right i was wrong about that, i am get confused by names
| entry.withField(StandardField.YEAR, publishedDateOnly.split("-")[0]); | ||
|
|
||
| if (scholarJsonEntry.has("id")) { | ||
| entry.setField(new UnknownField("scholarapi-id"), scholarJsonEntry.getString("id")); |
There was a problem hiding this comment.
also ZbMATH uses "zbmath", no -id, suggest "scholarapi"
| @Test | ||
| @Override | ||
| @DisabledOnCIServer("Unstable on CI") | ||
| public void pageSearchReturnsUniqueResultsPerPage() { |
There was a problem hiding this comment.
mmm, i think Empty override runs nowhere, and itis the only test of this feature, maybe try add offline cursor tests
There was a problem hiding this comment.
still an empty override, so it runs nowhere, also the cursor logic still has no test
yes you are right FulltextFetcher (GET /pdf/{id}) it was there in issue but is it okay to open another pr for that? separate pr's for separate work and i will make the CHANGELOG.md entry |
yeah, ok, very good 💯 |
| try { | ||
| URLDownload urlDownload = new URLDownload(getTestUrl()); | ||
| urlDownload.addHeader("X-API-Key", apiKey); | ||
| urlDownload.asInputStream().close(); |
There was a problem hiding this comment.
i think it is like off pattern, and it uses up an API credit on every check, maybe better to verify the key by checking the HTTP status code instead, like SpringerNatureWebFetcher/MedlineFetcher do
There was a problem hiding this comment.
Sure i will do that
|
@kopper @subhramit whenever you get time can i get a review and can you explain about these fails |
|
For checkstyle, did you set up your IDE as per our contributing guidelines/steps to set up local development environment? |
|
If you click on a failing workflow, it shows details of its run and why it failed. For example on line 622 of the checkstyle test
|
|
Similarly figure out the rest. |
Okay understood |
|
@subhramit hii I know you guys must be busy and sorry for bothering but whenever you get time please do review this |
| String publishedDateOnly = publishedDate.split("T")[0]; | ||
| entry.withField(StandardField.DATE, publishedDateOnly); | ||
| entry.withField(StandardField.YEAR, publishedDateOnly.split("-")[0]); | ||
| entry.withField(new UnknownField("scholarApiHasText"), String.valueOf(scholarJsonEntry.getBoolean("has_text"))); |
There was a problem hiding this comment.
What are these two for? they put "true"/"false" on every entry, right?, so if they are groundwork for the fulltext fetcher, a short comment would help
There was a problem hiding this comment.
yes they are on every entry i will add a comment
There was a problem hiding this comment.
I'm not sure it's okay to add them
| @Test | ||
| @Override | ||
| @DisabledOnCIServer("Unstable on CI") | ||
| public void pageSearchReturnsUniqueResultsPerPage() { |
There was a problem hiding this comment.
still an empty override, so it runs nowhere, also the cursor logic still has no test
LoayTarek5
left a comment
LoayTarek5
left a comment
There was a problem hiding this comment.
Great work so far, keep going @thesauravpoddar
|
|
||
| private static final int NO_YEAR_BOUND = Integer.MIN_VALUE; | ||
|
|
||
| private static final Pattern JOURNAL_VOLUME = Pattern.compile("Volume\\s+([^,]+)", Pattern.CASE_INSENSITIVE); |
There was a problem hiding this comment.
I think [^,]+ is somewhat greedy and only stops at a comma, like "Volume 9 Issue 4" (no comma) gives VOLUME = "9 Issue 4".
so using (\d+) or ([^,\s]+) maybe better
|
|
||
| @Test | ||
| @Override | ||
| public void pageSearchReturnsUniqueResultsPerPage() throws FetcherException { |
There was a problem hiding this comment.
The body is identical to the interface default, so you can just delete the override
There was a problem hiding this comment.
so i should delete this method from my test?
There was a problem hiding this comment.
Or maybe remove it, if it's identical to default?
| } | ||
|
|
||
| @Test | ||
| @Disabled("ScholarAPI has no journal scoped search") |
There was a problem hiding this comment.
remove the @disabled now, handleJournal returns "Nature" and the inherited test expects getJournalPrefix() + "Nature", where the default prefix is ""
|
Thank you for persevering @thesauravpoddar I only went through the PR in a shallow way, so will wait for your approvals. |
Looks good to me! Great job on not giving up and addressing the feedback with patience @thesauravpoddar |
| authorsList.add(authors.getString(i)); | ||
| } | ||
| if (!authorsList.isEmpty()) { | ||
| entry.withField(StandardField.AUTHOR, String.join(" and ", authorsList)); |
There was a problem hiding this comment.
I think more idiomatic JabRef way is to do something like this:
List<Author> authors = List.of(
new Author("John", "", null, "Smith", null),
new Author("Peter", "", null, "Black Brown", null)
);
entry.setField(StandardField.AUTHOR, AuthorList.of(authors).getAsFirstLastNamesWithAnd());(you can leave the withField)
There was a problem hiding this comment.
@thesauravpoddar please take this suggestion as well
There was a problem hiding this comment.
so here the the format is something like
List<Author> authors = List.of(
new Author(firstName, "", null, lastName, null)
);according to example give for john smith and peter Black brown
but
"authors": [
"Feder, Jacob S.",
"Soloway, Benjamin S."
]
above is one of response for authors give on ScholarApi docs so there is a comma then how we will decide which is lastName and which is firstName?
There was a problem hiding this comment.
In standard bibtex (which is also Jabref's format), a comma is explicitly used to separate the last name from the first name. The format is "LastName, FirstName".
So, in the response "Feder, Jacob S." the first name is Jacob S. and lastname is Feder.
Because the API is already returning the names in this standard format, you can join the authors with "and" (the way you are doing now) and let the built-in AuthorList parser normalize to it how Ruslan is suggesting..
String rawAuthors = String.join(" and ", authorsList);
AuthorList parsedAuthors = AuthorList.parse(rawAuthors);
entry.withField(StandardField.AUTHOR, parsedAuthors.getAsFirstLastNamesWithAnd());There was a problem hiding this comment.
Okayy understood I'll do this way
There was a problem hiding this comment.
Okay, I think everything is good now!
Though I only don't think that these fields should be added in the entry:
Maybe leave only scholarapi but rename it to something with id so that user could understand what is this.
UPD: Sorry, read the discussion, and it seems that just leaving the api name is an established norm, so it's okay to leave it, but I think those booleans shouldn't be added.
Looking forward to the full text fetcher! (if you want to continue to work on it, of course)
LoayTarek5
left a comment
LoayTarek5
left a comment
There was a problem hiding this comment.
Great work @thesauravpoddar so far, and thanks for your prsistence.
just small things left
| Optional<String> cursor = Optional.ofNullable(response.optString("next_indexed_after", null)) | ||
| .filter(StringUtil::isNotBlank); | ||
| if (cursor.isEmpty()) { | ||
| throw new FetcherException(url, "More results are available but returned no pagination cursor", null); |
There was a problem hiding this comment.
This breaks searches returning 20 results or more, like isLastPage = 20 < 20 is false, so the code expect anther page, the API sends no cursor and this throws, so i think it better to go back to .ifPresent, keep the isNotBlank filter
| * `FulltextFetcher`: Searches for a PDF for an exiting bibliography entry | ||
| * `SearchBasedFetcher`: Searches providers using a given query and returns a set of (new) bibliography entry. The user-facing side is implemented in the UI described at [https://docs.jabref.org/collect/import-using-online-bibliographic-database](https://docs.jabref.org/collect/import-using-online-bibliographic-database). | ||
|
|
||
| `JournalInformationFetcher` supplies the Entry Editor's journal-information popup. It uses Crossref for journal identity (title, publisher, ISSNs) and OpenAlex for the h-index and yearly work and citation counts. If enabled in the preferences, it uses the configured OpenAlex API key. |
There was a problem hiding this comment.
I think that paragraph looks unrelated to ScholarAPI, is it leftover from a rebase?
There was a problem hiding this comment.
so should be removed please
There was a problem hiding this comment.
No this js good here i was experiencing errors with fetchers.md whenever i was fixing so i kept the new changes from main branch and added new so this is right here just when i started working on this pr this text was not there but this is not breaking anything here
There was a problem hiding this comment.
There was a problem hiding this comment.
If it is already on main, it should not appear in your diff at all.
merge main into your branch instead, and the paragraph stays without being part of this PR, becuase showing up there means it is being readded by hand, right?
There was a problem hiding this comment.
correct, this was somehow manually added
|
|
||
| private static final int NO_YEAR_BOUND = Integer.MIN_VALUE; | ||
|
|
||
| private static final Pattern JOURNAL_VOLUME = Pattern.compile("Volume\\s+([^,\s]+)", Pattern.CASE_INSENSITIVE); |
There was a problem hiding this comment.
the Single backslsh \s in a string literal means a literal space,right? so this compiles to [^, ]+, try use [^,\\s]+, worth a test with a tab or comma less variant
| @Override | ||
| public List<String> getTestAuthors() { | ||
| return List.of(); | ||
| } | ||
|
|
||
| @Override | ||
| public String getTestJournal() { | ||
| return ""; | ||
| } |
There was a problem hiding this comment.
| @Override | |
| public List<String> getTestAuthors() { | |
| return List.of(); | |
| } | |
| @Override | |
| public String getTestJournal() { | |
| return ""; | |
| } | |
| @Override | |
| public List<String> getTestAuthors() { | |
| return List.of("unsupported"); | |
| } | |
| @Override | |
| public String getTestJournal() { | |
| return "unsupported"; | |
| } |
or put comments
github-actions
Bot
added
status: no-bot-comments
and removed
status: changes-required
5 participants
Related issues and pull requests
Closes #16233
PR Description
ScholarApi fetcher is visible in web search
test connection passed successfully with the real API KEY taken from ScholarApi website
Following are the results i got using
graphenekeyword and the results are 20 per page and next page is loading as we can seeSteps to test
AI usage
AI CHECKLIST.md walkthrough
Checklist
CHANGELOG.mdin a way that can be understood by the average user (if change is visible to the user)