Skip to content

9817 adding HTTP Status Exception check to Grobid Citation Fetcher#9929

Merged
Siedlerchr merged 6 commits into
JabRef:mainfrom
mtheiley:main
May 30, 2023
Merged

9817 adding HTTP Status Exception check to Grobid Citation Fetcher#9929
Siedlerchr merged 6 commits into
JabRef:mainfrom
mtheiley:main

Conversation

@mtheiley

@mtheiley mtheiley commented May 20, 2023 •
edited by ThiloteE
Loading

Copy link
Copy Markdown

Fixes #9817

Hi all, I am part of the university group that was assigned this issue.

I tried to re-create this issue with failing to connect to http://grobid.jabref.org:8070/, however currently it is up and running fine. So instead I tried to re-create the issue by connecting to https://httpstat.us/503 which always gives a 503 error code when attempting to connect.

It looks like JSoup throws an HttpStatusException when the status code is not 200 OK https://jsoup.org/apidocs/org/jsoup/HttpStatusException.html

I have tried adding an additional catch statement in the try catch within GrobitCitationFetcher.java and this seems to be working. When we don't get 200 OK it will display the error after 0 entries are passed. This should hopefully catch all status related errors now including the 503 status. I have attached a copy of the error code being displayed in the GUI.

I originally posted this solution on the issue 9817 I have made some changes based on recommendations made there. I was recommended to change the RuntimeExceptions to FetcherExceptions for my changes, however I was just basing my changes on the existing SocketTimeoutException below. Should both of these be changed to FetcherExceptions?

Other members of my team are currently looking into wire mock tool as suggested.

Please let us know what you think we are eager to hear your feedback.

Connection Failure 
### Compulsory checks
- [ ] Change in `CHANGELOG.md` described in a way that is understandable for the average user (if applicable)
- [ ] Tests created for changes (if applicable)
- [x] Manually tested changed features in running JabRef (always required)
- [x] Screenshots added in PR description (for UI changes)
- [ ] [Checked developer's documentation](https://devdocs.jabref.org/): Is the information available and up to date? If not, I outlined it in this pull request.
- [ ] [Checked documentation](https://docs.jabref.org/): Is the information available and up to date? If not, I created an issue at <https://github.com/JabRef/user-documentation/issues> or, even better, I submitted a pull request to the documentation repository.

@Siedlerchr

Siedlerchr commented May 20, 2023

Copy link
Copy Markdown
Member

Yes please change all to Fetcher Exceptions. If all is correct they will be caught by the caller and displayed in a friendly way to the user

@mtheiley

mtheiley commented May 22, 2023 •
edited
Loading

Copy link
Copy Markdown
Author

Yes please change all to Fetcher Exceptions. If all is correct they will be caught by the caller and displayed in a friendly way to the user

Hi, I have tried to change these to Fetcher Exceptions, but I think I understand why these were originally left as runtime exceptions. The function parseUsingGrobid is passed into a Arrays.stream.map(this::parseUsingGrobid). When I change the runtime exceptions to Fetcher Exceptions I am getting an error on the line .map(this::parseUsingGrobid) saying that FetcherException is unhandled. I believe this is because we are giving .map() a reference to the function that it will call internally, since map can't catch / handle a FetcherException this is what is causing the problem.

I found a post on stack overflow discussing a similar problem. I believe using runtime exceptions is a workaround for this problem as it leaves the exception unchecked. https://stackoverflow.com/questions/19757300/java-8-lambda-streams-filter-by-method-with-exception

The Array.stream.map(this::parseUsingGrobid) is surrounded by a try catch that catches runtime exceptions and re-throws them as FetcherExceptions

@koppor

koppor commented May 22, 2023

Copy link
Copy Markdown
Member

The function parseUsingGrobid is passed into a Arrays.stream.map(this::parseUsingGrobid). When I change the runtime exceptions to Fetcher Exceptions I am getting an error on the line .map(this::parseUsingGrobid) saying that FetcherException is unhandled. I believe this is because we are giving .map() a reference to the function that it will call internally, since map can't catch / handle a FetcherException this is what is causing the problem.

I see.

Would you mind trying out faux-pas so that code at the fetcher is still clean?

I think, it's throwingFunction.

I found a post on stack overflow discussing a similar problem. I believe using runtime exceptions is a workaround for this problem as it leaves the exception unchecked. stackoverflow.com/questions/19757300/java-8-lambda-streams-filter-by-method-with-exception

I have been there years ago and upvoted some of the answers ^^. Though, no faux-pas there... Let's see.

@Siedlerchr

Siedlerchr commented May 22, 2023

Copy link
Copy Markdown
Member

I think for the moment we can keep the runtime exception.

Matthew Theiley added 2 commits May 24, 2023 23:01
9817 have used faux-pas as a work around to allow the use of FetcherE…
ab3be18 
…xceptions in .map() via a ThrowingFunction
9817 improved on faux-pas solution by using single line throwingFunct…
e78ed2b 
…ion() instead of intermediate variable ThrowingFunction
@mtheiley

mtheiley commented May 24, 2023

Copy link
Copy Markdown
Author

@koppor I have got faux-pas working and now this::parseUsingGrobid is wrapped inside a throwingFunction() which seems to fix the issue.

@Siedlerchr

Siedlerchr commented May 24, 2023 via email

Copy link
Copy Markdown
Member

Siedlerchr
Siedlerchr previously approved these changes May 24, 2023
View reviewed changes
@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label May 24, 2023
@Siedlerchr

Siedlerchr commented May 25, 2023

Copy link
Copy Markdown
Member

Jlink seems to work so from my point of view it's good!

koppor
koppor reviewed May 25, 2023
View reviewed changes
Comment thread src/main/java/org/jabref/logic/importer/fetcher/GrobidCitationFetcher.java Outdated
.map(FauxPas.throwingFunction(this::parseUsingGrobid))
.flatMap(Optional::stream)
.collect(Collectors.toList());
} catch (RuntimeException e) {

@koppor koppor May 25, 2023

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.

Is this still necessary? I think, this block can be removed!

@mtheiley mtheiley May 26, 2023

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@koppor I have removed the try/catch block checking for runtime exceptions since we are just using FetcherExceptions now

9817 have removed the try/catch checking for runtime exception, since…
218d5a4 
… we are only throwing FetcherExceptions within parseUsingGrobid now
@Siedlerchr Siedlerchr merged commit af9972b into JabRef:main May 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@koppor koppor koppor left review comments

@Siedlerchr Siedlerchr Siedlerchr approved these changes

Assignees

No one assigned

Labels

component: ui status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Improve error handling on GROBID server connection issues - 503 Service unaviable

4 participants

@mtheiley @Siedlerchr @koppor @ThiloteE