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

Fix ebook.de #3967

Merged
merged 6 commits into from
Apr 20, 2018
Merged

Fix ebook.de #3967

merged 6 commits into from
Apr 20, 2018

Conversation

stefan-kolb
Copy link
Member

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

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
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
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
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
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
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
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
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
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
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