Issuexxxx improve http error handling #1033

kouralex · 2020-07-16T22:45:33Z

This PR fixes a bunch or problems in current code.

Fixes the following listed issues:
Fixes #693
Fixes #721
Fixes #722
Fixes #723
Fixes #1011

Addresses #759 in the sense that it could be implemented in a similar way (add a retry button/handle ajax failure)

Adds PHP fatal error checks for various easyrdf http client induced errors. E.g., doesn't crash the UI in the usual cases for a non-responding SPARQL endpoint (some may still exist).
Global search: adds a warning for every selected, non-default endpoint vocabularies.

Searches may now return 500 Internal Server Error, which on turn is utilized by Ajax queries. Please note that not every failure results in an HTTP 500 Error.
Waypoint system in searches is fixed; no more extra requests.

TODO/open questions:

Should global search warnings lie above the number of results?
Translations for various new JS and Twig template variables
Tests? (and for which part)
Is the way errors are handled here the best possible?
Add a retry button to other parts of UI?
Is the usage of HTTP 500 good as it is?
Sidebar could be separated for good from the content area, this PR only has started the process.
Also, it is possible to merge the functionality of Skosmos into a single page app by developing this a bit further.

… throwable

…ializing ajax query on vocabulary concept error page

…ch queries; fixes waypoint system used in search; added checks for various easyrdf http client errors

sonarcloud · 2020-07-16T22:46:35Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
4 Code Smells

No Coverage information
0.0% Duplication

kouralex · 2020-07-16T22:47:02Z

model/Vocabulary.php

+             if ($this->model->getConfig()->getLogCaughtExceptions()) {
+                 error_log('Caught exception: ' . $e->getMessage());
+             }
+             return null;


Is this ok?

codecov · 2020-07-16T22:56:02Z

Codecov Report

Merging #1033 into master will decrease coverage by 0.65%.
The diff coverage is 15.38%.

@@             Coverage Diff              @@
##             master    #1033      +/-   ##
============================================
- Coverage     60.32%   59.67%   -0.66%     
- Complexity     1593     1612      +19     
============================================
  Files            32       32              
  Lines          4429     4484      +55     
============================================
+ Hits           2672     2676       +4     
- Misses         1757     1808      +51

Impacted Files	Coverage Δ	Complexity Δ
controller/WebController.php	`14.16% <0.00%> (-0.82%)`	`88.00 <0.00> (+5.00)`	⬇️
model/Model.php	`78.37% <0.00%> (-3.15%)`	`109.00 <0.00> (+4.00)`	⬇️
model/DataObject.php	`63.26% <21.42%> (-12.35%)`	`27.00 <0.00> (+4.00)`	⬇️
model/Vocabulary.php	`82.77% <40.90%> (-4.03%)`	`118.00 <0.00> (+6.00)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 37601f9...9f762ff. Read the comment docs.

osma

Bundling so many fixes into a single PR makes it very difficult to review. Could you please split this into smaller, more focused PRs?

kouralex · 2021-02-26T22:59:42Z

Thank you for your valuable feedback @osma .

I just gave a quick look at this draft PR after it has been a while since I made this and it seemed like that except for the waypoint fixes (that just happened to annoy me at the time of building this PR up) and #1011-related part (which has been by the way fixed in another commit already (now) residing in the master branch) this PR truly fixes (or at least tries to fix, as it says on the PR comment, there are still things to be done) one big problem that has just been reported in multiple places - well OK, to be honest, one could separate most UI fixes into their own PR but then again - they depend on this PR as this changes the way they (that is to say, UI) expect issues are passed down (catching them). One can, of course, drop them from this PR (which may, by the way, cause other hassle as the UI is not prepared for aforementioned changes) but I personally think this is a matter of personal preference for how to test that things truly are working in the very UI without writing the unit tests first. Anyways, anything needed for a successful review is there in the PR comment - after all, this is a draft PR and some comments are appreciated and very much needed (see the list of TODOs/open questions) in order to finish this PR.

osma

I gave some quick comments.
The biggest problem ATM is the conflicts that should be resolved before this PR can even be tested with the code in current master.

osma · 2021-09-07T08:10:36Z

view/search-result.twig

+<div class="alert alert-warning">
+{% for voc in skipped_vocabs %}
+{% set vocTitle = voc.title %}
+<h4>{% trans %}Warning: Skipped searching from '{{vocTitle}}' vocabulary!{% endtrans %}</h4>


We need translations for this new UI string

osma · 2021-09-07T08:11:02Z

view/vocab-search-listing.twig

    <h4>{% trans %}Error: Requested vocabulary not found!{% endtrans %}</h4>
+    {% else %}
+    <h4>{% trans %}Error: Search failed!{% endtrans %}</h4>


We need translations for this new UI string

osma · 2021-09-07T08:11:16Z

view/vocab-shared.twig

+    {% set vocabInfo = vocab.info(request.contentLang) %}
+    {% if not vocabInfo %}
+    <div class="alert alert-danger" role="alert">
+        {% trans "Error: Failed to retrieve vocabulary information!" %}


We need translations for this new UI string

osma · 2021-09-07T08:12:29Z

resource/js/docready.js

+        },
+        error: function(jqXHR, textStatus, errorThrown) {
+          $loading.detach();
+          var $failedSearch = $('<div class="alert alert-danger"><h4>Error: Loading for more items failed!</h4></div>');


This error message should be translated

osma · 2021-09-07T08:12:40Z

resource/js/docready.js

+        error: function(jqXHR, textStatus, errorThrown) {
+          $loading.detach();
+          var $failedSearch = $('<div class="alert alert-danger"><h4>Error: Loading for more items failed!</h4></div>');
+          var $retryButton = $('<button class="btn btn-default" type="button">Retry</button>');


"Retry" should be translated as well

osma · 2021-09-07T08:15:24Z

controller/WebController.php

-                $vocabObjects[] = $this->model->getVocabulary($vocid);
+                try {
+                    $vocabObjects[] = $this->model->getVocabulary($vocid);
+                } catch (Exception $e) {


An empty catch block is bad practice. What happens with the $vocabObjects variable that is used below?
At a minimum, it could be set to null here, or array() if that makes more sense.

osma · 2021-09-07T11:24:24Z

It's not possible to do proper review on this PR as it mixes up so many different fixes - and it's even more difficult now that the branch has diverged from master and ended up in conflicted state. I will split this into separate branches (mainly by commits) and open PRs for each of them.

osma · 2021-09-07T12:59:55Z

I've now disentangled most of the changes included in this PR into separate PRs:

I think these can be handled separately in those PRs.

What remains is the last commit (so far) in this PR 9f762ff which is itself a bundle of several fixes in a single commit, and it seems that it may rely on changes done in the previous commits, so cannot easily be separated. I think the above PRs should be merged first and then the remaining commit can be dealt with.

osma · 2021-09-09T14:42:47Z

The remaining parts of this PR (well, except the message about skipped vocabularies, which was deemed too obtrusive) have been merged via PR #1197. Closing this one.

kouralex added 6 commits July 14, 2020 12:05

error-resistant Vocabulary->getInfo()

1fcd308

fixes issues with failing search; improve exception handling to catch…

a1bfbed

… throwable

sidebar navigation now works after erroneous page; fixes #721

4ab7ba3

fixes #722, #723 by passing vocab to twig template and adding an init…

583b950

…ializing ajax query on vocabulary concept error page

fix a bug in working vocabulary search

af94269

fixes fore failing vocabulary/global search; fixes erroring ajax sear…

9f762ff

…ch queries; fixes waypoint system used in search; added checks for various easyrdf http client errors

kouralex added this to the Next Tasks milestone Jul 16, 2020

kouralex requested review from joelit and osma July 16, 2020 22:45

kouralex commented Jul 16, 2020

View reviewed changes

osma requested changes Sep 1, 2020

View reviewed changes

kouralex added size-medium 2 hours to 2 days needs discussion needs unittest size-large more than 2 days enhancement and removed size-medium 2 hours to 2 days labels Feb 26, 2021

kouralex requested a review from osma September 2, 2021 08:17

kouralex assigned osma and kouralex Sep 2, 2021

osma requested changes Sep 7, 2021

View reviewed changes

osma mentioned this pull request Sep 7, 2021

error-resistant Vocabulary->getInfo() #1194

Merged

This was referenced Sep 7, 2021

Improve search error handling #1195

Merged

Fix sidebar navigation after erroneous page #1196

Merged

osma mentioned this pull request Sep 8, 2021

Fixes for searches, ajax, waypoints, easyrdf HTTP client errors #1197

Merged

osma closed this Sep 9, 2021

joelit modified the milestones: Next Tasks, No further action Jun 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issuexxxx improve http error handling #1033

Issuexxxx improve http error handling #1033

kouralex commented Jul 16, 2020 •

edited

Loading

sonarcloud bot commented Jul 16, 2020

kouralex Jul 16, 2020

codecov bot commented Jul 16, 2020 •

edited

Loading

osma left a comment

kouralex commented Feb 26, 2021

osma left a comment

osma Sep 7, 2021

osma Sep 7, 2021

osma Sep 7, 2021

osma Sep 7, 2021

osma Sep 7, 2021

osma Sep 7, 2021

osma commented Sep 7, 2021

osma commented Sep 7, 2021 •

edited

Loading

osma commented Sep 9, 2021

Issuexxxx improve http error handling #1033

Issuexxxx improve http error handling #1033

Conversation

kouralex commented Jul 16, 2020 • edited Loading

sonarcloud bot commented Jul 16, 2020

kouralex Jul 16, 2020

Choose a reason for hiding this comment

codecov bot commented Jul 16, 2020 • edited Loading

Codecov Report

osma left a comment

Choose a reason for hiding this comment

kouralex commented Feb 26, 2021

osma left a comment

Choose a reason for hiding this comment

osma Sep 7, 2021

Choose a reason for hiding this comment

osma Sep 7, 2021

Choose a reason for hiding this comment

osma Sep 7, 2021

Choose a reason for hiding this comment

osma Sep 7, 2021

Choose a reason for hiding this comment

osma Sep 7, 2021

Choose a reason for hiding this comment

osma Sep 7, 2021

Choose a reason for hiding this comment

osma commented Sep 7, 2021

osma commented Sep 7, 2021 • edited Loading

osma commented Sep 9, 2021

kouralex commented Jul 16, 2020 •

edited

Loading

codecov bot commented Jul 16, 2020 •

edited

Loading

osma commented Sep 7, 2021 •

edited

Loading