search

neo4jrb / activegraph

An active model wrapper for the Neo4j Graph Database for Ruby.
http://neo4jrb.io
MIT License
1.39k stars 276 forks source link

Wrap neo4j driver exceptions in ActiveGraph::Core::CypherError family #1640

Closed joshjordan closed 6 months ago

joshjordan commented 3 years ago

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.

klobuczek commented 3 years ago

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.

joshjordan commented 3 years ago

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 notifications@github.com 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 https://github.com/neo4jrb/activegraph/pull/1640#issuecomment-750420695, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAD7LSTRUS2JO5ZFVM4THA3SWIZDTANCNFSM4VGHDGEA .

klobuczek commented 3 years ago

@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.

klobuczek commented 3 years ago

@joshjordan just looked through the code. ClientException should have a code attribute e.g. Neo.ClientError.Schema.ConstraintValidationFailed. See https://neo4j.com/docs/status-codes/current/. Let me know if that's not working for you. I know it's an additional check within the rescue clause. However, there are over 120 such codes. Creating an exception class for each of them would be overkill.