EDS backend: support additional "missing record" cases #2324
Conversation
SetUp an override variable in config.ini: healthCheckId
…trieve the general BackendException had to be caught as well.
| @@ -342,6 +342,8 @@ public function retrieve($id, ParamBag $params = null) | |||
| default: | |||
| throw $e; | |||
| } | |||
| } catch(BackendException $e) { | |||
There was a problem hiding this comment.
What exception is actually getting thrown here? Is there some more specific metric we can use to determine when it's appropriate to return an empty response (because, for example, the record really doesn't exist) and when something is more seriously wrong with the system?
There was a problem hiding this comment.
Also note that you're missing a space after catch if you want to conform to style guidelines, which is at least in part why the build is failing here. You should be able to auto-fix it with vendor/bin/phing php-cs-fixer.
There was a problem hiding this comment.
The Exception is thrown in line 293, so I think we would have to define the metric by ourself if I'm not mistaken.
There was a problem hiding this comment.
I thought the exception thrown in line 293 was related to input validation -- that's saying that the provided ID is not in the expected format before even making an API call at all. How would invalid IDs end up on your lists to begin with? Or am I misunderstanding something? I thought the problem you were trying to solve here had to do with requesting IDs for records that were previously available but no longer exist.
There was a problem hiding this comment.
Ah, sorry then it's my fault. I tried to reproduce an error reported by a user but it seems I just didn't create a proper test case. :/
I will search for the id and check again what the exact exception will be.
There was a problem hiding this comment.
Numeric order corrected. :)
Should we add the 133 as well? Even if it is rare, I have the philosophy that the overall system should not be blocked by exceptions (especially not if it is coming from outside like an API) but rather handle them in a proper way, if possible.
I mean, if there is an error on any API I want the user to be able to continue working as normal as possible and maybe only to receive a message that something went wrong in a specific part of the process. But if we have information locally I want the system to show what we have and not let an API exception block VuFind in a broader range than it should.
For example in the user lists, I'm showing the user that the record is not available in the database any more and give all the information stored in the DB, with a link to a new search, so that the user could get alternative sources if available.
There was a problem hiding this comment.
Thanks, both of you! I don't think @cwolfebsco needs to bother anyone else -- the reference he shared is extremely helpful and I think answers the necessary questions. @he247, I agree that adding 133 is probably worthwhile, even if it's rare. It might also be worth using comments to annotate the codes in the switch statement with the descriptions shared above and/or including a link to the referenced documentation, just so it's more clear exactly what everything means if we review this code again in future.
There was a problem hiding this comment.
Great I'll add the comments and 133. Then I'll open the review again.
There was a problem hiding this comment.
Thanks, @he247 -- I'm leaving on vacation right now, but I'll merge this as soon as I can. :-)
There was a problem hiding this comment.
@demiankatz enjoy your holiday! :) Thank you for your help, as always!
demiankatz
changed the title
Only return an empty response in case of the specific Exception in line 293.
The actual concerning ID threw an API Exception 132
Comments and reference to error codes
demiankatz
changed the title
To prevent errors of the backend to interrupt the batch process of retrieve the general BackendException had to be caught as well.