jruby / activerecord-jdbc-adapter

JRuby's ActiveRecord adapter using JDBC.
BSD 2-Clause "Simplified" License
461 stars 385 forks source link

Mismatched required arguments in execute_query_raw #1063

Open headius opened 4 years ago

headius commented 4 years ago

This affects ARJDBC 1.3 only. Versions 50+ were already patched as described below.

Transplanted from https://github.com/jruby/jruby/issues/6191#issuecomment-624282547

I believe the ArgumentError may be a bug in ar-jdbc 1.3.x exposed by indy.

Looking at the implementation of execute_query_raw in ar-jdbc:

https://github.com/jruby/activerecord-jdbc-adapter/blob/1-3-stable/src/java/arjdbc/jdbc/RubyJdbcConnection.java#L771-L788

We can see here that one definition sets up required = 1 and the other sets up required = 2. When we have bindings that conflict on the number of required arguments like this, we may end up with unpredictable behavior.

In your case, when invokedynamic is enabled, we call along the IRubyObject[] path (at least until the indy call optimizes) which requires two arguments. This is similar to other bugs we've seen where the varargs form of a "native" method does not handle the smaller arities.

The code below should patch this issue for ar-jdbc 1.3 but I have not worked on that branch in a long time. I'd need assistance from @kares or @enebo to test and/or release such a change.

diff --git a/src/java/arjdbc/jdbc/RubyJdbcConnection.java b/src/java/arjdbc/jdbc/RubyJdbcConnection.java
index ccd789f5..c572fe79 100644
--- a/src/java/arjdbc/jdbc/RubyJdbcConnection.java
+++ b/src/java/arjdbc/jdbc/RubyJdbcConnection.java
@@ -768,7 +768,7 @@ public class RubyJdbcConnection extends RubyObject {
      * @throws SQLException
      * @see #execute_query_raw(ThreadContext, IRubyObject[], Block)
      */
-    @JRubyMethod(name = "execute_query_raw", required = 1) // optional block
+    @JRubyMethod(name = "execute_query_raw") // optional block
     public IRubyObject execute_query_raw(final ThreadContext context,
         final IRubyObject sql, final Block block) throws SQLException {
         final String query = sql.convertToString().getUnicodeValue();
@@ -783,10 +783,12 @@ public class RubyJdbcConnection extends RubyObject {
      * @return raw query result as a name => value Hash (unless block given)
      * @throws SQLException
      */
-    @JRubyMethod(name = "execute_query_raw", required = 2, optional = 1)
-    // @JRubyMethod(name = "execute_query_raw", required = 1, optional = 2)
+    @JRubyMethod(name = "execute_query_raw", required = 1, optional = 2)
     public IRubyObject execute_query_raw(final ThreadContext context,
         final IRubyObject[] args, final Block block) throws SQLException {
+
+        if (args.length == 1) return execute_query_raw(context, args[0], block);
+
         // args: (sql), (sql, max_rows), (sql, binds), (sql, max_rows, binds)
         final String query = args[0].convertToString().getUnicodeValue(); // sql
         IRubyObject max_rows = args.length > 1 ? args[1] : null;
headius commented 4 years ago

@mahaswami If you could run your reproduction script while also passing -Xbacktrace.style=full I suspect we will confirm my theory. The trace should bottom out in an arity check triggered by an IRubyObject[] call from one of the invokedynamic InvokeSite classes in JRuby.