Skip to content

Fix ebook.de#3967

Merged
stefan-kolb merged 6 commits into
masterfrom
fetcher-tests
Apr 20, 2018
Merged

Fix ebook.de#3967
stefan-kolb merged 6 commits into
masterfrom
fetcher-tests

Conversation

@stefan-kolb

@stefan-kolb stefan-kolb commented Apr 20, 2018

Copy link
Copy Markdown
Member

Refs #3854

Unfortunately not a very clean fix, but it is fine again.
Using the normal InputStream does not return anything. Probably as it is detected as automated crawler?
The interface with InputStream is just not really fitting here.

@stefan-kolb stefan-kolb requested a review from tobiasdiez April 20, 2018 08:21
@stefan-kolb stefan-kolb added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Apr 20, 2018
try (InputStream stream = new BufferedInputStream(getURLForID(identifier).openStream())) {
List<BibEntry> fetchedEntries = getParser().parseEntries(stream);
try {
HttpResponse<String> response = Unirest.get(getURLForID(identifier).toString()).asString();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Well, this now has the disdvantage that the Stream does not get closed. That's why try(....) is preferable.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

There is no Stream anymore?!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah sorry, got confused by the diff.
Why is this ByteArrayInputStream needed? Is there no way to directly get the response with unirest?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Caused by the interface

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not sure whats better but we should stick to one: unirest vs our UrlDownloader

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We could directly use jsoup. We use that in dfiferent cases

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Jsoup is for a DOM that needs to be parsed. I don't need that here. I just need the body contents.

@Siedlerchr

Copy link
Copy Markdown
Member

The ISBN fetcher tries ebook.de first and if no result is found, it switches to the chimbori/amazon fetcher.
Is this still valid?

@stefan-kolb

Copy link
Copy Markdown
Member Author

Yes.

@LinusDietz LinusDietz added this to the v4.2 milestone Apr 20, 2018
@stefan-kolb stefan-kolb removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Apr 20, 2018
This reverts commit 02d12e5.
@stefan-kolb stefan-kolb merged commit 3dab429 into master Apr 20, 2018
@stefan-kolb stefan-kolb deleted the fetcher-tests branch April 20, 2018 10:31
Siedlerchr added a commit that referenced this pull request Apr 21, 2018
* upstream/master:
  Fix IEEE Fetcher by enabling cookie support (#3968)
  Update flowless from 0.6 -> 0.6.1
  Update wiremock from 2.16.0 -> 2.17.0
  Fix ebook.de (#3967)
Siedlerchr added a commit that referenced this pull request Apr 25, 2018
* upstream/master: (47 commits)
  Fix xmp exporter (#3964)
  Update JUnit from 5.2.0-M1 -> 5.2.0-RC1
  Update xmlunit from 2.5.1 -> 2.6.0
  Update mockito-core from 2.18.0 -> 2.18.3
  Fixieee (#3970)
  Fix IEEE Fetcher by enabling cookie support (#3968)
  Update flowless from 0.6 -> 0.6.1
  Update wiremock from 2.16.0 -> 2.17.0
  Fix ebook.de (#3967)
  LOC test #3854
  Fix arxiv tests
  New translations JabRef_en.properties (French) (#3963)
  Upgrade modernizer plugin
  New Crowdin translations (#3962)
  Fix test
  Improve test
  New Crowdin translations (#3961)
  New translations JabRef_en.properties (French) (#3960)
  Remove brackets before checking equality
  Replace all IEEE URLs with https #3930 (#3944)
  ...

# Conflicts:
#	build.gradle
#	src/main/java/org/jabref/gui/BasePanel.java
#	src/main/java/org/jabref/gui/actions/CleanupAction.java
#	src/main/java/org/jabref/gui/collab/ChangeScanner.java
#	src/main/java/org/jabref/gui/exporter/ExportAction.java
#	src/main/java/org/jabref/gui/fieldeditors/LinkedFilesEditorViewModel.java
#	src/main/java/org/jabref/gui/openoffice/DetectOpenOfficeInstallation.java
#	src/main/java/org/jabref/gui/openoffice/OpenOfficeSidePanel.java
#	src/main/java/org/jabref/gui/preftabs/PreferencesDialog.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants