Trilogy errors all inherit from base Trilogy::Error class

[adrianna-chang-shopify]

This PR makes breaking changes to the error classes. All Trilogy errors are now descendants of the base error class, Trilogy::Error, and no longer inherit from syscall errors. This will need to go out as part of a major release.

More context on original introduction of these error classes: #15

I also attempted to add some docs for the various error classes. Feedback welcome! 🙇‍♀️

cc @byroot

[matthewd]

I was expecting us to aim in the other direction, of making more Errno subclasses for Trilogy-sourced errors.

Broadly, while I think it's very useful to be able to rescue "all Trilogy errors", I think it's also useful to be able to handle all ECONNRESET or similar, without forcing a general "maybe the network's broken" handler to know about the special Trilogy (etc) flavours of that exception.

[casperisfine]

@matthewd I thought we discussed that in #11

I think it's also useful to be able to handle all ECONNRESET or similar, without forcing a general "maybe the network's broken" handler to know about the special Trilogy (etc) flavours of that exception.

Well, for the vast majority of cases, Trilogy is expected to be used via the Active Record adapter right? So errors wouldn't even bubbule up as Syscall errors anyway.

And libraries that may raise many different base errors are really a pain to deal with. Like Net::HTTP for instance, is a nightmare to rescue potential errors properly, e.g. https://github.com/lostisland/faraday-net_http/blob/16ad984dfc0e2915addda17aa6879d8158ce32ad/lib/faraday/adapter/net_http.rb#L15-L31

[matthewd]

@casperisfine I had seen the module-based approach discussed there (and currently implemented) as the end state.

for the vast majority of cases, Trilogy is expected to be used via the Active Record adapter

I wouldn't want to design the library solely for that use case, but it's definitely a very important one. However, I'd argue that's exactly a situation where allowing AR to generically handle errno-style errors from all adapters, rather than rely solely on per-adapter lists of differently-spelled-but-semantically-identical exceptions, makes for a better result.

[casperisfine]

However, I'd argue that's exactly a situation where allowing AR to generically handle errno-style errors from all adapters,

What I've found from working on various network clients is that Errno codes aren't very meaningful unless you know exactly which syscall emitted them.

So it's up to you, but I'm very much unconvinced about this approach.

[adrianna-chang-shopify]

However, I'd argue that's exactly a situation where allowing AR to generically handle errno-style errors from all adapters

Are there other database clients that bubble up syscall errors? Mysql2 didn't, and I know PostgreSQL follows SQLSTATE code conventions rather than being tied to syscall errnos. Can you elaborate on what future use case you see for handling errno-style errors in AR across the various adapters?

Is there a middle ground where we could move away from inheriting from the syscall errors, but still preserve the underlying errno as an attribute? I'd really like to move us away from the module-based approach that currently exists; it's confusing, and I was under the impression it was a temporary solution until we were confident making breaking changes.

[matthewd]

I think I'm getting stuck on the fact that we do still raise Errno errors for other situations. I had been envisaging a change where we absorbed all of those and "upgraded" them to trilogy-specific subclasses (plus module inclusion), for https://github.com/github/trilogy/blob/142eb0e8370537fc7fe38081a03fdf42d1cb67aa/contrib/ruby/ext/trilogy-ruby/cext.c#L90-L91

These are the most common things to want to rescue, for sure, but it feels awkward to me for the same error-producing code to choose from two completely independent exception hierarchies. (Plus it's an incompatible change if at any point in the future we decide there's a third errno that's proper-exception-worthy.)

[adrianna-chang-shopify]

@matthewd and I talked about this at length today, and seems to make the most sense to stick with the module-based approach until we have a better sense for what syscall errors might appear, and how we’d like to classify them.

The main issue is that, as Matthew mentioned, we’re still surfacing syscall errors in some of the error handling code in Trilogy. It doesn’t make sense to move away from syscall errors if some code can potentially raise errno errors. I’d proposed having a single Trilogy::SyscallError base error that we’d use for all remaining sys call errors, but:

• This prevents rescuing against specific errors, which certain callers might care about
• It will make it hard for us to reclassify errors later on if we realize that particular syscall error warrants a proper subclass, should be treated as a connection error, etc.

I’d also proposed trying to map every syscall error to a StandardError class that would inherit from Trilogy::Error, moving away from the syscall superclasses, but Matthew reminded me that syscall errors vary across platforms so this would be tricky.

We decided that the best way forward was to:

• Keep the module-based approach, which offers users specificity in terms of rescuing particular errno errors, and flexibility with how we shape our error hierarchy moving forward.
• Ensure that the remaining syscall errors include Trilogy::Error so that they can be rescued generically by consumers.
• Keep an eye out to see which types of syscall errors still show up, and consider whether we should expand the error translation logic in the client and adapter to accommodate.

@byroot if you have more feedback feel free to chime in, but otherwise I'll 1) close this PR and 2) open a PR to ensure all syscall errs include the base Trilogy::Error module (we'll probably want to do some metaprogramming magic here 😅 )

[byroot]

the fact that we do still raise Errno errors for other situations

In my head, we'd do what is necessary to no longer raise any Errno, ever.

[adrianna-chang-shopify]

we'd do what is necessary to no longer raise any Errno, ever.

I don't have ideas for how to handle this other than attempting to turn all existing syscall errors into proper error classes, or using a blanket Trilogy::SyscallError that would absorb existing syscall errors. The first isn't really feasible, and the second forces us to make breaking changes any time we want to reclassify errors.

If you have suggestions for how to get away from Errno Jean, I'm all ears! :)

[casperisfine]

Let's go with Matthew's plan for now.