Revert "Raise ConnectionNotEstablished rather than StatementInvalid in Mysql2Adapter#quote_string"

[kamipo]

Reverts #40152

cc @casperisfine

[tgxworld]

I'm not sure if the errors surfaced by ActiveRecord are considered internal but it looks like 0313f5f may be backwards incompatible as well if people are rescuing from the native errors.

[kamipo]

I see your point, but I support at least #40110 and #40148 as an improvement.

[tgxworld]

Yup I agree that the changes are improvements but I was wondering if we need to state the changes in the release notes or provide some sort of upgrade path since the changes may be backwards incompatible.

[kamipo]

It is almost unintentional that Active Record might return a native error, and I don't know if anyone understood that, so I don't know if it is even worth mentioning in the release notes.

Can I hear your thoughts?

[tgxworld]

So I've written code like the following because ActiveRecord has been raising the native error.

https://github.com/discourse/rails_failover/blob/c2ab4d28af3ab623c7a0a901c054e0deed38fbc7/lib/rails_failover/active_record/railtie.rb#L28-L35

Even if it may not have been intentional, ActiveRecord has been exposing the native errors for a while now so gems or applications may be surprised by the change in the next release.

[kamipo]

Okey so it is worth to add a CHANGELOG entry.

cc @casperisfine

[casperisfine]

Okey so it is worth to add a CHANGELOG entry.

Do you want me to PR the change back with that changelog entry?

I also understand that this kinda break compatibility, I did PR these change because I realized our code base was riddled with native errors rescue. But I don't see any way to change this in a backward compatible way.

[kamipo]

Honestly I don't have strong opinion whether adding a CHANGELOG entry.
If anyone think it is worth to add a CHANGELOG entry, I'm not against to that.

[casperisfine]

Up to you, I'm just asking what's the best way to move forward this this. I think we can all agree that leaking native exceptions is a bug, and that wrapping them is a good thing.

I'm not even sure it make sense to talk about compatibility when we're fixing a bug.

This changed allowed a very nice cleanup in our code base, so let me know what I can do to make it mergeable.

[kamipo]

Are you talking about #40152?

I think #40110 and #40148 are almost a bug fix.

But for #40152, it is hard to tell by itself whether the change is worth breaking compatibility. If you break compatibility to achieve something more valuable, I don't strongly against to that.

[tgxworld]

I'm not even sure it make sense to talk about compatibility when we're fixing a bug.

@casperisfine Sorry I actually commented on the wrong PR in my original comment. I meant to make the comment in #40148 which lead me to the discovery of 0313f5f. I was suggesting that we at least make the change to wrap native errors more visible since code which is relying on rescuing native errors will break without any hint in the release note of the change. I believe Rails have been raising the native errors for many versions now so I'm not sure if that is considered a bug.

[casperisfine]

Are you talking about #40152?

I'm talking about all three.

But for #40152, it is hard to tell by itself whether the change is worth breaking compatibility

Keep in mind that that #40152 should have been part of #40110. in 6.0 the native error is raised, so StatementInvalid is already a change. So might as well wrap with the proper one.

[casperisfine]

I was suggesting that we at least make the change to wrap native errors more visible

Sure, that make sense.

I believe Rails have been raising the native errors for many versions now so I'm not sure if that is considered a bug.

I know, but I think we can agree that it's quite a cruft. If you really think it's a big deal I guess we could introduce a config flag to switch that behavior. It seems excessive to me, but if that's what it takes to clean that up I can do it.

[kamipo]

Keep in mind that that #40152 should have been part of #40110. in 6.0 the native error is raised, so StatementInvalid is already a change. So might as well wrap with the proper one.

#40152 is breaking not only @connection.quote in mysql2 adapter but also @connection.execute in all adapters, I've concerned about the latter part.

[casperisfine]

r but also @connection.execute in all adapters

Oh I see.

So what would be acceptable? A CHANGELOG entry? a config flag to keep backward compatibility by default? something else?

Or is it just not an acceptable change at all?

[tgxworld]

I know, but I think we can agree that it's quite a cruft. If you really think it's a big deal I guess we could introduce a config flag to switch that behavior. It seems excessive to me, but if that's what it takes to clean that up I can do it.

O no I actually prefer the change but I raised the question for two reasons. The first is to make the change more visible in the release note which I think a CHANGELOG entry will be sufficient. The second was to find out if the Rails team considers it to be a breaking change or a bug fix since I personally think it is a breaking change considering that the native errors have been surfaced unofficially for awhile now. I also didn't want to suggest adding a config flag to the 6.1 framework defaults first because the work would be redundant if the Rails team thinks otherwise.

[kamipo]

I would like to see first how valuable the breaking change #40152 is.

As I said at #40158 (comment), it is hard to tell by itself whether the change is worth breaking compatibility.

[casperisfine]

I would like to see first how valuable the breaking change #40152 is.

The value is to distinguish between programing errors (e.g. invalid syntax, missing table, etc) vs transient errors (e.g. network was down for a second, the db was overloaded, etc).

These two main classes of errors have to be dealt with differently. One is a bug and should be reported to the developer, the other is more of a reality to deal with, should probably be retried a couple times and then be translated to a health metric.

Most network clients make a good distinction between these two families of errors, for instance in redis-rb you have BaseConnectionError vs CommandError. Same in say dalli where you have a very distinct NetworkError.

So yes, I think it's important to raise the proper exception class. Historically it's been very hard to do proper error handling with Active Record. It got better the the last few releases, but there are still quite a few problems that makes it hard.

[kamipo]

In the past, I had added StatementTimeout in #31129 for proper error handling.

Actually all XxxTimeout errors are raised from non invalid statements, but the error classes inherited StatementInvalid for compatibility with the previous.

I think this situation is similar with #31129.

We can do the same way as #31129, but if we prefer the way of breaking compatibility, I think that we need at least a CHANGELOG entry.

[casperisfine]

We can do the same way as #31129,

Not really though. StatementTimeout was a new exception, so it could be made a subclass for compatibility, but ConnectionNotEstablished already exists.

Unless you suggest to introduce another sublass of InvalidStatement, but then that would mean a second connection error, which would be a pain.

I think I see a few ways out of this:

• Accept the compatibility breakage and simple document it.
• Use modules and include them in the exceptions, this allow to have multiple parallel classifications.
• Have a deprecation cycle with a config variable that switch between the current error classification and a new cleaned up one.

[kamipo]

Is it acceptable to change ConnectionNotEstablished to a subclass of StatementInvalid?
It is a bit strange, but I think it has almost no compatibility impact.

[casperisfine]

Is it acceptable to change ConnectionNotEstablished to a subclass of StatementInvalid?

I don't think so no. Because when you rescue StatementInvalid today you wouldn't get these kind of connection errors.

[kamipo]

Okey so I can support the first way, probably we (at least I) can follow the compatibility breakage if it is noted.

[casperisfine]

Ok, so do you want me to PR all these changes again with a proper CHANGELOG entry?

Also on a sidenote since you mentioned it, StatementTimeout being a subclass of InvalidStatement is really not ideal :/. But one battle at a time I guess.

[kamipo]

Ok, so do you want me to PR all these changes again with a proper CHANGELOG entry?

Yeah, please go ahead.