Translate new error classes #24
Conversation
| @@ -75,7 +75,7 @@ class << self | |||
| def new_client(config) | |||
| config[:ssl_mode] = parse_ssl_mode(config[:ssl_mode]) if config[:ssl_mode] | |||
| ::Trilogy.new(config) | |||
| rescue Trilogy::DatabaseError => error | |||
| rescue Trilogy::ProtocolError => error | |||
There was a problem hiding this comment.
It looks like ER_ACCESS_DENIED_ERROR = 1045 will be a BaseConnectionError instead of a ProtocolError, which I think means it's no longer getting translated the same way. We are probably missing a test for this case...
There was a problem hiding this comment.
Actually I don't think I've quite wrapped my head around what happens to the BaseConnectionErrors here in general. They used to get translated into ActiveRecord::ConnectionNotEstablished, but now are they getting raised as is?
There was a problem hiding this comment.
Ah yes thank you! I'll add a test case and ensure that we're also rescuing connection errors when initializing the client here 👍
There was a problem hiding this comment.
Oops sorry, wrote my comment before seeing your second one!
Actually I don't think I've quite wrapped my head around what happens to the BaseConnectionErrors here in general. They used to get translated into ActiveRecord::ConnectionNotEstablished, but now are they getting raised as is?
I definitely think we'd want to translate them here. Maybe we should only be rescuing BaseConnectionError and not ProtocolError at all though? Is it worth running this branch with only BaseConnectionErrors rescued against GH's CI to see if there are any edge cases we're missing where we'd expect a non-BaseConnectionError to be translated to ConnectionNotEstablished?
There's also some interesting behaviour right now if a bad host is supplied -- would love your thoughts on that, see comment below :D
There was a problem hiding this comment.
It looks like this method has explicit translation for a few more errors:
ER_BAD_DB_ERROR = 1049
ER_CONN_HOST_ERROR = 2003
ER_UNKNOWN_HOST_ERROR = 2005
and the trilogy PR has those all as ProtocolError. So I think we still need to rescue ProtocolError unless we're planning to turn those specific ones into BaseConnectionError. I don't know all these errors too well, so I don't have any strong opinions to offer. Rescuing both seems safest for now to match the current behavior, but that doesn't necessarily make it the most correct. 🤷🏻♂️
There was a problem hiding this comment.
Sounds good 👍 I think we may want ER_CONN_HOST_ERROR and ER_UNKNOWN_HOST_ERROR to show up as connection errors, but I don't think we even see these right now in Trilogy because host-related errors are being surfaced as QueryErrors ..? Either way, ER_BAD_DB_ERROR is indeed a ProtocolError, so we def need to rescue that!
|
|
||
| test ".new_client on host error" do | ||
| configuration = @configuration.merge(host: "unknown") | ||
| assert_raises ActiveRecord::DatabaseConnectionError do |
There was a problem hiding this comment.
@composerinteralia this fails right now, because we raise this as a query error so it's not being rescued and translated.
ActiveRecord::ConnectionAdapters::TrilogyAdapterTest#test_.new_client_on_host_error [/Users/adriannachang/src/github.com/activerecord-trilogy-adapter/test/lib/active_record/connection_adapters/trilogy_adapter_test.rb:51]:
[ActiveRecord::DatabaseConnectionError] exception expected, not
Class: <Trilogy::QueryError>
Message: <"trilogy_connect - unable to connect to unknown:3306: TRILOGY_DNS_ERR">
But on main currently, this type of error shows up as a Trilogy::Error and not a Trilogy::DatabaseError, so I don't believe it's being translated as intended right now either 🤔 :
ActiveRecord::ConnectionAdapters::TrilogyAdapterTest#test_.new_client_on_host_error [/Users/adriannachang/src/github.com/activerecord-trilogy-adapter/test/lib/active_record/connection_adapters/trilogy_adapter_test.rb:32]:
[ActiveRecord::NoDatabaseError] exception expected, not
Class: <Trilogy::Error>
Message: <"trilogy_connect - unable to connect to unknown:3306: TRILOGY_DNS_ERR">
This ought to be raised as a BaseConnectionError right (on the Trilogy side)?
There was a problem hiding this comment.
Yeah, I agree QueryError doesn't quite make sense for this one, since that is meant "for invalid queries or parameters". We could add a case for that to handle_trilogy_error. BaseConnectionError is for "potentially transient network errors", correct? Does that fit this DNS error?
This does make me wonder if there are other trilogy errors that are going to turn up as QueryErrors but maybe make more sense as something else.
There was a problem hiding this comment.
BaseConnectionError is for "potentially transient network errors", correct? Does that fit this DNS error?
I think so 👍 It's the network refusing the connection, and Mysql2 treats these as ConnectionErrors so it seems reasonable for us to do the same.
There was a problem hiding this comment.
Okay, so I started looking at this in the Trilogy client, but I'm a little stuck on which layer to change the error 😅 We could change the #initialize logic to raise BaseConnectionError directly instead of calling #handle_trilogy_error, but there's no relevant error code to set... The host errors (CR_CONN_HOST_ERROR => 2003 and CR_UNKNOWN_HOST => 2005) on Mysql2's end (I think) come from the libmysqlclient library, so we wouldn't expect to see these in Trilogy either way, only server errors.
So I guess we have a couple options:
- Continue to raise this type of error as a query error, update the adapter to rescue appropriately based on the error message instead of looking for an error code
- Convert this to a
BaseConnectionErrorin the Ruby bindings for Trilogy, update the adapter to rescue based on the error message - Something else..?
Either way, perhaps it makes sense to skip the host test for now so that we can get this initial work shipped, and then I'll investigate what we should be doing in a follow up. WDYT?
cc @casperisfine do you have any opinions here?
There was a problem hiding this comment.
Jean and I chatted IRL -- seems like the most appropriate solution is just to add all of the relevant error code ints to #handle_trilogy_error and have those raised as connection errors. TRILOGY_DNS_ERR for one, but also TRILOGY_EOF and TRILOGY_PROTOCOL_VIOLATION (as evidenced in the connection tests that fails with MySQL8 right now)
adrianna-chang-shopify
force-pushed
the
ac-translate-new-error-classes
branch
from
a799a8e
to
8704a92
Compare
adrianna-chang-shopify
force-pushed
the
ac-translate-new-error-classes
branch
from
8704a92
to
4143db9
Compare
adrianna-chang-shopify
force-pushed
the
ac-translate-new-error-classes
branch
from
4143db9
to
03c1610
Compare
We're already using some of the new error constants from that release (see #24) so we need to bump.
We're already using some of the new error constants from that release (see trilogy-libraries#24) so we need to bump.
Related to: trilogy-libraries/trilogy#15
Updates the adapter code to work with the changes made to Trilogy's error classes (see above PR).
The diff is pretty minimal -- we change
DatabaseErrortoProtocolErrorin a couple of spots, and the baseTrilogy::Erroris nowTrilogy::BaseError.Curious about the release process -- do we make the changes to the Trilogy Ruby bindings, release a new version X, and then merge these changes in a new version of
activerecord-trilogy-adapterthat specifies X as the minimum required version fortrilogy?cc @composerinteralia @casperisfine