-
Notifications
You must be signed in to change notification settings - Fork 22k
Redact SQL in errors #34468
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Redact SQL in errors #34468
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel good about parsing SQL to redact it, it can become too complex.
@tenderlove @matthewd @jeremy WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we don't need to warn. We can just skip the redact
method if prepared statements is enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I agree
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really think it is not a good idea to parse SQL. There are too many dialects, operation, operators. If people wants to redact SQL is better to not send the SQL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a fair point. I kind of agree, but I wanted to see how far I could get parsing it with regexes. I've tested this against MySQL, SQLite, and PG, but it can all goes out the window if we're expected to filter raw SQL strings crafted by developers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a big fan of setting ivars like this. If we decide to change the ivar name, this code will break. It's coupling the test to the internals of the connection object. Instead, can we set up a new connection that doesn't have prepared statements enabled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that this isn't great. We have that, but it doesn't have any tables in it 😑. I will see what I can do.
I kind of agree with this. We've had bad luck in the past with parsing SQL. OTOH, this should only happen when there are exceptions, so maybe it doesn't matter? I have no strong feelings either way, though I do think we should improve the test as my comment says. 😊 |
I'll update the PR to just not include the SQL in the error string if prepared statements is on. |
Yeah, I too don't like the idea of parsing the SQL -- DBMSes don't agree on how to quote strings, so even though it sounds simple, it'd get messy fast. I'm actually not a big fan of how we currently jam the query into the exception message, because it can make the message long and unwieldy (and very ugly if someone sticks it in a heading, say... which we do by default). What if we move it into a separate property on |
fad4071
to
b310150
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have coverage for forcing message encoding here, but since there's no more SQL interpolation, do we still need to encode this?
+1 on this. I like the idea of the statement being a property of the exception and not part of the message itself. |
b480aba
to
d16f037
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This and all other methods arguments changes is a breaking change without deprecation. All adapters will have to change their code and all users subclassing any StatementInvalid
exception will have to change their calls and definitions.
I think those changes are fine but I'd add all those breaking changes to the CHANGELOG.
2e00260
to
0dbed36
Compare
Move `ActiveRecord::StatementInvalid` SQL to error property. Also add bindings as an error property.
0dbed36
to
192b7bc
Compare
…_class` method As updated here : rails/rails#34468
…_class` method As updated here : rails/rails#34468
Summary
Fixes #34316.
Allows redaction of SQL in translated SQL errors with
config.active_record.redacted_sql_errors = true
. I'm not a regex pro, so I think there's room for improvement on both expressions. Also, now that we don't use mocha, what's the best way to mock initializer that raises an error?r? @rafaelfranca