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

Fetcher OttoBib #5125

Closed
wants to merge 10 commits into from
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
import org.slf4j.LoggerFactory;

/**
* Fetcher for ISBN trying ebook.de first and then chimbori.com
* Fetcher for ISBN trying ebook.de first, chimbori.com and then ottobib
*/
public class IsbnFetcher implements EntryBasedFetcher, IdBasedFetcher {

Expand Down Expand Up @@ -47,12 +47,19 @@ public Optional<BibEntry> performSearchById(String identifier) throws FetcherExc

IsbnViaEbookDeFetcher isbnViaEbookDeFetcher = new IsbnViaEbookDeFetcher(importFormatPreferences);
Optional<BibEntry> bibEntry = isbnViaEbookDeFetcher.performSearchById(identifier);

// nothing found at ebook.de, try chimbori.com
if (!bibEntry.isPresent()) {
LOGGER.debug("No entry found at ebook.de try chimbori.com");
IsbnViaChimboriFetcher isbnViaChimboriFetcher = new IsbnViaChimboriFetcher(importFormatPreferences);
bibEntry = isbnViaChimboriFetcher.performSearchById(identifier);
}

//nothing found at ebook.de and chimbori.com, try ottobib
if (!bibEntry.isPresent()) {
LOGGER.debug("No entry found at ebook.de and chimbori.com try ottobib");
IsbnViaOttoBibFetcher isbnViaOttoBibFetcher = new IsbnViaOttoBibFetcher(importFormatPreferences);
bibEntry = isbnViaOttoBibFetcher.performSearchById(identifier);
}

return bibEntry;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
package org.jabref.logic.importer.fetcher;

import java.net.MalformedURLException;
import java.net.URISyntaxException;
import java.net.URL;
import java.util.List;
import java.util.Optional;

import org.jabref.logic.importer.FetcherException;
import org.jabref.logic.importer.ImportFormatPreferences;
import org.jabref.logic.importer.ParseException;
import org.jabref.model.entry.BibEntry;
import org.jabref.model.strings.StringUtil;

import com.mashape.unirest.http.HttpResponse;
import com.mashape.unirest.http.Unirest;
import com.mashape.unirest.http.exceptions.UnirestException;

/**
* Fetcher for ISBN using https://www.ottobib.com
*/
public class IsbnViaOttoBibFetcher extends AbstractIsbnFetcher {

public IsbnViaOttoBibFetcher(ImportFormatPreferences importFormatPreferences) {
super(importFormatPreferences);
}

@Override
public String getName() {
return "ISBN (OttoBib)";
}

/**
* @return null, because the identifier is passed using form data. This method is not used.
*/
@Override
public URL getURLForID(String identifier) throws URISyntaxException, MalformedURLException, FetcherException {
Copy link
Member

Choose a reason for hiding this comment

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

Please remove these exceptions. They can never be thrown.

Copy link
Member

Choose a reason for hiding this comment

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

@LinusDietz You are right that those exceptions can never be thrown, but the Exception declaration is coming from the interface IdBasedParserFetcher

Copy link
Member

Choose a reason for hiding this comment

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

Why is it not possible to implement the ottobib fetcher by properly overwriting getURLForID as e.g here:

@Override
public URL getURLForID(String identifier) throws URISyntaxException, MalformedURLException, FetcherException {
this.ensureThatIsbnIsValid(identifier);
URIBuilder uriBuilder = new URIBuilder(BASE_URL);
uriBuilder.addParameter("isbn", identifier);
return uriBuilder.build().toURL();
}

I don't really see why the performSearchById has to be overwritten (which is more work as you have to manually post the request and parse the response).

return null;
}

@Override
public Optional<BibEntry> performSearchById(String identifier) throws FetcherException {
if (StringUtil.isBlank(identifier)) {
return Optional.empty();
}

this.ensureThatIsbnIsValid(identifier);

HttpResponse<String> postResponse;

String BASE_URL = "https://www.ottobib.com/isbn/" + identifier + "/bibtex";
Copy link
Member

Choose a reason for hiding this comment

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

I would propose to make BASE_URL final.

Also, I would prefer a String.format(): String.format("https://www.ottobib.com/isbn/%s/bibtex", identifier);


try {
postResponse = Unirest.post(BASE_URL)
.asString();
Copy link
Member

Choose a reason for hiding this comment

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

Please put this into the line before

} catch (UnirestException e) {
throw new FetcherException("Could not retrieve data from ottobib.com", e);
}
if (postResponse.getStatus() != 200) {
throw new FetcherException("Error while retrieving data from ottobib.com: " + postResponse.getBody());
}

List<BibEntry> fetchedEntries;
try {
fetchedEntries = getParser().parseEntries(postResponse.getRawBody());
} catch (ParseException e) {
throw new FetcherException("An internal parser error occurred", e);
}
if (fetchedEntries.isEmpty()) {
return Optional.empty();
} else if (fetchedEntries.size() > 1) {
LOGGER.info("Fetcher " + getName() + "found more than one result for identifier " + identifier
+ ". We will use the first entry.");
}

BibEntry entry = fetchedEntries.get(0);

// ottobib does not return an ISBN.
entry.setField("isbn", identifier);

return Optional.of(entry);
}
}
2 changes: 2 additions & 0 deletions src/test/java/org/jabref/logic/importer/WebFetchersTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import org.jabref.logic.importer.fetcher.AbstractIsbnFetcher;
import org.jabref.logic.importer.fetcher.IsbnViaChimboriFetcher;
import org.jabref.logic.importer.fetcher.IsbnViaEbookDeFetcher;
import org.jabref.logic.importer.fetcher.IsbnViaOttoBibFetcher;
import org.jabref.logic.importer.fetcher.MrDLibFetcher;

import org.junit.jupiter.api.BeforeEach;
Expand Down Expand Up @@ -36,6 +37,7 @@ void getIdBasedFetchersReturnsAllFetcherDerivingFromIdBasedFetcher() throws Exce
// Remove special ISBN fetcher since we don't want to expose them to the user
expected.remove(IsbnViaChimboriFetcher.class);
expected.remove(IsbnViaEbookDeFetcher.class);
expected.remove(IsbnViaOttoBibFetcher.class);
assertEquals(expected, getClasses(idFetchers));
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
package org.jabref.logic.importer.fetcher;

import java.util.Optional;

import org.jabref.logic.importer.FetcherException;
import org.jabref.logic.importer.ImportFormatPreferences;
import org.jabref.model.entry.BibEntry;
import org.jabref.model.entry.BiblatexEntryTypes;
import org.jabref.testutils.category.FetcherTest;

import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.mockito.Answers;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.mockito.Mockito.mock;

@FetcherTest
public class IsbnViaOttoBibFetcherTest extends AbstractIsbnFetcherTest {

@BeforeEach
public void setUp() {
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest to convert this into a static method createSampleBibEntry(). See Java by Comparison, "Favor Standalone Tests".

bibEntry = new BibEntry();
bibEntry.setType(BiblatexEntryTypes.BOOK);
bibEntry.setField("bibtexkey", "9782819502746");
bibEntry.setField("title", "Les mots du passé : roman");
bibEntry.setField("publisher", "́Éd. les Nouveaux auteurs");
bibEntry.setField("year", "2012");
bibEntry.setField("author", "Denis");
bibEntry.setField("isbn", "978-2-8195-02746");
bibEntry.setField("url", "https://www.ottobib.com/isbn/9782819502746/bibtex");

fetcher = new IsbnViaOttoBibFetcher(mock(ImportFormatPreferences.class, Answers.RETURNS_DEEP_STUBS));
Copy link
Member

Choose a reason for hiding this comment

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

Move fetcher into the respective tests to adhere to the 'given-then-when' structure of tests.

Copy link
Member

Choose a reason for hiding this comment

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

Just out of interest to learn something, but doesn't this give (needless ?) code duplication especially since all tests are of the form "for-input-x-verify-output-y". If the state of the fetcher needs to be varied then a test of the form

    @Test
    public void test() throws FetcherException {
        fetcher.setSomething(value);

        Entry fetchedEntry = fetcher.fetch(input);

        assertEquals(expected, fetchedEntry);
    }

still perfectly adheres to the usual test pattern.

}

@Test
@Override
public void testName() {
assertEquals("ISBN (OttoBib)", fetcher.getName());
}

@Test
@Override
public void testHelpPage() {
assertEquals("ISBNtoBibTeX", fetcher.getHelpPage().get().getPageName());
}

@Test
@Override
public void searchByIdSuccessfulWithShortISBN() throws FetcherException {
Optional<BibEntry> fetchedEntry = fetcher.performSearchById("0321356683");
bibEntry.setField("bibtexkey", "0321356683");
bibEntry.setField("isbn", "0321356683");
assertEquals(Optional.of(bibEntry), fetchedEntry);
}

@Test
@Override
public void searchByIdSuccessfulWithLongISBN() throws FetcherException {
Optional<BibEntry> fetchedEntry = fetcher.performSearchById("9780321356680");
bibEntry.setField("bibtexkey", "9780321356680");
bibEntry.setField("isbn", "9780321356680");
assertEquals(Optional.of(bibEntry), fetchedEntry);
}

@Test
@Override
public void authorsAreCorrectlyFormatted() throws Exception {
BibEntry bibEntry = new BibEntry();
bibEntry.setType(BiblatexEntryTypes.BOOK);
bibEntry.setField("bibtexkey", "9782819502746");
bibEntry.setField("title", "Les mots du passé : roman");
bibEntry.setField("publisher", "́Éd. les Nouveaux auteurs");
bibEntry.setField("year", "2012");
bibEntry.setField("author", "Denis");
bibEntry.setField("isbn", "978-2-8195-02746");
bibEntry.setField("url", "https://www.ottobib.com/isbn/9782819502746/bibtex");

Optional<BibEntry> fetchedEntry = fetcher.performSearchById("9782819502746");
assertEquals(Optional.of(bibEntry), fetchedEntry);
}
}