Wrap neo4j driver exceptions in ActiveGraph::Core::CypherError family #1640
Conversation
|
The idea was not to translate the driver exceptions. The driver has a very detailed exception hierarchy which I would prefer not to replicate. Some errors are only differentiable by message rather than exception class type. I would say the vast majority of those exceptions if not all are not recoverable in activegraph so they have only a debug value. The driver exceptions serve this purpose well. But you are right that the unused exception classes should be removed from the code. They orginate from times where there was no driver and the whole error system was driven by the http adapter. I think the clean up is the only thing we should do at the moment. |
|
Okay. This will probably be a significant hurdle for folks upgrading from
neo4jrb 9, since in neo4jrb 9, we catch exceptions by class instead of also
having to inspect the message. The documentation will need to be updated
with examples to show how to handle them in activegraph. The docs currently
show that you need to rename a few specific classes, and then beyond that,
most classes have just moved from the Neo4j namespace to the ActiveGraph
namespace (and implicitly, this covers exceptions), which is how we ended
up expecting these to be thrown. It seemed to be just a mistake that the
ActiveGraph exceptions were not used, but I see now it was intentional.
From a philosophical perspective, it seems to me to be a downgrade for the
API to change such that we now have to catch a general exception and
inspect the message, which is both a degraded developer experience (since
ruby has first class syntax for catching by exception class) and more
error prone (since the developer may get the message wrong). Again, this is
a place where it necessitates us writing our own wrappers to handle this
case because there are far too many occurrences in our code to duplicate
the message matching all over the place. Unfortunately, there isn’t a great
hook in ActiveGraph for us to provide our own exception wrappers so we will
probably stick with the fork of ActiveGraph I created.
Do we have a developer community of folks actually using ActiveGraph in a
large production application? It would be useful to get real user feedback
from others dealing with these problems. I’d be surprised if others felt it
was desirable to match on messages instead of exception classes.
…On Wed, Dec 23, 2020 at 1:28 PM Heinrich Klobuczek ***@***.***> wrote:
The idea was not to translate the driver exceptions. The driver has a very
detailed exception hierarchy which I would prefer not to replicate. Some
errors are only differentiable by message rather than exception class type.
I would say the vast majority of those exceptions if not all are not
recoverable in activegraph so they have only a debug value. The driver
exceptions serve this purpose well. But you are right that the unused
exception classes should be removed from the code. They orginate from times
where there was no driver and the whole error system was driven by the http
adapter. I think the clean up is the only thing we should do at the moment.
In general, I try to expose as many aspects of the driver as possible so
ActiveGraph::Core::Node is as well Neo4j::Driver::Node etc. That allows
using of the driver and active graph interchangeably.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1640 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAD7LSTRUS2JO5ZFVM4THA3SWIZDTANCNFSM4VGHDGEA>
.
|
|
@joshjordan could you give me some examples of how you react differently to different exception classes? I was under the impression that there is not much more you can do beyond simply logging those exceptions. |
|
@joshjordan just looked through the code. ClientException should have a |
This is a straw man solution for #1639
I do not expect this is ultimately how we want to solve it, but rather, opening this PR as an example that satisfies the tests in my application (which are looking for
ActiveGraph:Core::SchemaErrors::ConstraintValidationFailedError)If you can give me instructions on where to make the proper fix and what you'd like to see in terms of testing, I can fix up this PR.