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.

Sign up for GitHub

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

Jump to bottom

Consistently use fetch + improve error handling #69

Merged
merged 27 commits into from
Dec 14, 2023
Merged

Consistently use fetch + improve error handling #69

merged 27 commits into from
Dec 14, 2023

Conversation

RobinTF
Copy link
Contributor

@RobinTF RobinTF commented Nov 25, 2023 •
edited by hannahbast
Loading

The JavaScript code of the QLever UI launches queries at many places (to get a query result, to get suggestions, to clear the cache, to get statistics, to get a share link, etc.). In the code so far, each request did this slightly differently. This is now unified as much as possible. In particular, all requests now use fetch and none of the requests use jQuery.ajax anymore.

Along the way, remove code that is no longer needed (like the code that realized SERVICE in the JavaScript) and improve the error messages when something goes wrong.

hannahbast
hannahbast requested changes Nov 29, 2023
View reviewed changes
Copy link
Member

@hannahbast hannahbast left a comment

Choose a reason for hiding this comment

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

Thank you very much for the cleanup

backend/static/js/qleverUI.js Show resolved Hide resolved
backend/static/js/qleverUI.js Outdated Show resolved Hide resolved
backend/static/js/qleverUI.js Show resolved Hide resolved
backend/static/js/qleverUI.js Show resolved Hide resolved
backend/static/js/qleverUI.js Outdated Show resolved Hide resolved
backend/static/js/qleverUI.js Outdated Show resolved Hide resolved
@RobinTF
Copy link
Contributor Author

RobinTF commented Nov 29, 2023

@hannahbast I believe we should discuss this 1:1. A lot of my coding decisions boil down to correcting execution order after using await and preventing so called "unhandled promise rejections" which occur when a promise is rejected somewhere without a catch or something. While browsers typically log such events it's better to explicitly handle this case, which is why you see all the "catch()" snippets throughout the codebase.

hannahbast
hannahbast requested changes Dec 2, 2023
View reviewed changes
Copy link
Member

@hannahbast hannahbast left a comment

Choose a reason for hiding this comment

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

1-1 with Robin, with minor change requests

hannahbast
hannahbast approved these changes Dec 2, 2023
View reviewed changes
Copy link
Member

@hannahbast hannahbast left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

@hannahbast
Copy link
Member

@RobinTF This query used to give a proper error message (the error message from QLever), now it just says "400 Bad Request" https://qlever.cs.uni-freiburg.de/wikidata/MCyZeF

@RobinTF
Copy link
Contributor Author

RobinTF commented Dec 2, 2023

@hannahbast Should be fixed now

@hannahbast
Copy link
Member

@RobinTF Awesome, thanks for the quick reaction, I updated the QLever UI instance and now we get a nice error message again: https://qlever.cs.uni-freiburg.de/wikidata/MCyZeF

@hannahbast
Copy link
Member

hannahbast commented Dec 2, 2023 •
edited
Loading

@RobinTF: We have to check the error handling again. I deliberately crashed https://qlever.cs.uni-freiburg.de/dblp . Now if you launch a query, you get

Unexpected token '<', "<!DOCTYPE "... is not valid JSON

Below is the full reply the Qlever UI gets (from the proxy in front of the backend) [1]. The error message used to be:

No reply from backend

That message was in the part, which you deleted: https://github.com/ad-freiburg/qlever-ui/blob/master/backend/static/js/qleverUI.js#L659 . And I now remember that it took some work to get it, but I forgot all the details.

[1] Full reply with headers:

HTTP/1.1 503 Service Unavailable
Date: Sat, 02 Dec 2023 23:59:22 GMT
Server: Apache/2.4.52 (Ubuntu)
Content-Length: 391
Connection: close
Content-Type: text/html; charset=iso-8859-1

<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML 2.0//EN">
<html><head>
<title>503 Service Unavailable</title>
</head><body>
<h1>Service Unavailable</h1>
<p>The server is temporarily unable to service your
request due to maintenance downtime or capacity
problems. Please try again later.</p>
<hr>
<address>Apache/2.4.52 (Ubuntu) Server at qlever.cs.uni-freiburg.de Port 443</address>
</body></html>

@RobinTF
Copy link
Contributor Author

RobinTF commented Dec 3, 2023

@hannahbast I added some changes that should make the error message nicer. Note that I deliberately changed the wording because technically at this point we got a response (if we have no internet connection we'll get a network error instead), we just realize that it's not valid JSON and inform the user about the fact that the response was not what we expected.

@hannahbast hannahbast mentioned this pull request Dec 9, 2023
Open
@hannahbast hannahbast changed the title Replace jQuery AJAX code with native fetch API Consistently use fetch instead of jQuery.ajax Dec 11, 2023
@hannahbast hannahbast changed the title Consistently use fetch instead of jQuery.ajax Consistently use fetch when speaking with the QLever backend Dec 14, 2023
@hannahbast hannahbast changed the title Consistently use fetch when speaking with the QLever backend Consistently use fetch for queries Dec 14, 2023
@hannahbast hannahbast changed the title Consistently use fetch for queries Consistently use fetch + improve error messages Dec 14, 2023
@hannahbast hannahbast changed the title Consistently use fetch + improve error messages Consistently use fetch + improve error handling Dec 14, 2023
@hannahbast hannahbast merged commit 79b72b6 into ad-freiburg:master Dec 14, 2023
@hannahbast
Copy link
Member

@RobinTF I have merged this now, thanks a lot for your work!

@RobinTF RobinTF deleted the migrate-to-fetch branch December 14, 2023 23:12
@hannahbast hannahbast mentioned this pull request Dec 15, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hannahbast hannahbast hannahbast approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@RobinTF @hannahbast