Various fixes #1040
Various fixes #1040
Conversation
This helps debugging stuff that only fails when run in combination.
When a wrong URL is provided, it can happen that newConnection() returns null. If this happens during configure_connection, the whole process is called over and over again and results in a stack overflow. Instead, just throw an error. Noticed because a new test in Rails master tries to connect with "mysql2:///wait_timeout=60", an URL unknown to JDBC.
AR tests do things like:
ActiveRecord::Base.configurations["arunit"].merge(database: "inexistent_activerecord_unittest")
which is also valid in applications. Not dup'ing the config is problematic:
E.g. the mysql adapter has this (simplified):
unless config[:url]
config[:url] = "...#{config[:database]}"
end
So once a DB connection has been made with the original config, :url is
set, above example that tries a different DB has no effect at all and
gives a connection the the original database instead. Fun.
.deep_dup solves the problem as the original config is left untouched.
|
@dr-itz so these changes work all the way back to 5.0 but will minimize our diff for when 6.1 comes? Also do you think we should create 60-stable yet? |
…umn when insert_returning
There was a problem hiding this comment.
Looks ok to me but I will ask one question: Do we care that execute_insert has changed arity? In JRuby, proper I would say this is too risky but is the Java of arjdbc an API for external consumption?
We should play it safe and maintain original signature and pass context.nil for pk to the newer signature.
I'd say we shouldn't care too much since this is an API between the ruby and the java part...sure you can access it, but...
you lost me there. where do you mean? |
|
whoos, just notices this indented with tabs instead of spaces..will fix |
JDBC offers an API to specify what keys to return. Rails knows the keys. Use that API directly to avoid loading too much data. E.g. pgjdbc appends "RETURNING *" to an insert query, effectively return _all_ columns. Since the jdbc driver does that, nobody ever noticed that the PG adapter is missing the code that appends the `RETURNING <pk>`
@deprecated
@JRubyMethod(name = "execute_insert", required = 1)
public IRubyObject execute_insert(final ThreadContext context, final IRubyObject sql) {
return execute_insert(context, sql, context.nil);
}The only reason is if any existing external gem is using arjdbc as a transitive dep it will get an error. I suppose the question to ask is this change so needed that we should break an existing gem (if one actually exists)? |
|
Ah now I get it. Makes a lot of sense, but we need to rename the new signature because there's the version with binds too. So old we had required = 1/2 and new we have required = 2/3 having an overlap there. Pushed on top |
|
@enebo thanks |
|
Something‘s going on with mariadb in CI...i‘ll check tomorrow if it‘s related |
| @@ -96,7 +96,7 @@ def initialize; end | |||
| connection_handler = connection_handler_stub | |||
|
|
|||
| config = { :jndi => 'jdbc/TestDS' } | |||
| connection_handler.expects(:jndi_connection) | |||
| connection_handler.expects(:jndi_connection).with() { |c| config = c } | |||
There was a problem hiding this comment.
just a minor thingy ... (its morning here) but did you not mean config == c ? 🍰
There was a problem hiding this comment.
Nope. Since config is now cloned in the connection handler, we cannot check for changes on the original object. By catching the jndi_connection arg I can get the cloned and changed object out this way. It's a bit ugly but it was the easiest way I could come up with
There was a problem hiding this comment.
ooh so that is what this accomplishes, thanks!
There was a problem hiding this comment.
Rails's exec_insert takes a lot of params so I would not mind adding one on execute_insert to kind of be on "par". initially did not like the exec_insert_pk naming that much (if its something possibly to be used outside as an API) but checking Rails API I did not come up with anything better ... so fine by me.
ok, it's pre-existing. I'll look into it maybe over the weekend :) |
This is mainly some "fallout" from ongoing work for Rails 6.1 (i.e. master) support