rails / arel

A Relational Algebra
2.06k stars 390 forks source link

Oracle visitor gets undefined method `to_i' for #<Arel::Nodes::BindParam:0x00000002c92910> #438

Closed yahonda closed 8 years ago

yahonda commented 8 years ago

The original issue has been reported https://github.com/rsim/oracle-enhanced/issues/848

I need help from Arel developers then opened another issue here.

This Rails application code should generates the sql statement below, but actually it gets undefined method `to_i' for #Arel::Nodes::BindParam:0x00000002c92910

@employee.order(:sort_order).offset(0).first.first_name
SELECT * FROM (
  SELECT raw_sql_.*, rownum raw_rnum_
  FROM (SELECT  "TEST_EMPLOYEES".* FROM "TEST_EMPLOYEES"  ORDER BY "TEST_EMPLOYEES"."SORT_ORDER" ASC ) raw_sql_
  WHERE rownum <= 1
)
WHERE raw_rnum_ > 0

Taking a look at Arel code and existing test case which works fine, but somehow offset.expr.to_i does not work anymore.

          collector << ") raw_sql_
                WHERE rownum <= #{offset.expr.to_i + limit}
              )
              WHERE "
          return visit(offset, collector)
        end

          it 'creates a different subquery when there is an offset' do
            stmt = Nodes::SelectStatement.new
            stmt.limit = Nodes::Limit.new(10)
            stmt.offset = Nodes::Offset.new(10)
            sql = compile stmt
            sql.must_be_like %{
              SELECT * FROM (
                SELECT raw_sql_.*, rownum raw_rnum_
                FROM (SELECT ) raw_sql_
                 WHERE rownum <= 20
              )
              WHERE raw_rnum_ > 10
            }
          end
* Rails 5.0.0.rc1
* Arel 7.0.0
* Oracle enhanced adapter `rails5` branch
* Oracle Database 11g

This issue does not reproduce with Oracle 12c database since it uses another visitor named Oracle12.

yahonda commented 8 years ago

Here is a test case to reproduce. https://gist.github.com/yahonda/f4bcb0fd1701c4620972eb5b38bd4498

It needs Oracle database , here is a step to create rails-dev-box running Oracle database.

git clone git@github.com:yahonda/rails-dev-box.git
cd rails-dev-box
git checkout runs_oracle
cp /path/to/oracle-xe-11.2.0-1.0.x86_64.rpm.zip puppet/modules/oracle/files/.
vagrant up
vagrant ssh
$ ruby rep.rb
elgreco commented 8 years ago

+1 please fix

rootatdarkstar commented 8 years ago

It's seems it's happen because of this commit: https://github.com/rails/rails/commit/574f255629a45cd67babcfb9bb8e163e091a53b8#diff-bf6dd6226db3aab589916f09236881c7R959

Since commit above Arel have no access to limit and offset values, now it's just BindParam without any value, so oracle visitor can't calculate rawnum value here - https://github.com/rails/arel/blob/master/lib/arel/visitors/oracle.rb#L30

When i tried to solve this problem i doesn't found a way to calculate rawnum in SQL with liimit binding. I can generate this statement with this hackish code:

          SELECT * FROM (
            SELECT raw_sql_.*, rownum raw_rnum_
            FROM (
                  .....
            ) raw_sql_ WHERE rownum <= (:a1 + :a2)
          )
          WHERE raw_rnum_ > :a2

But i can't bind one variable to two same placeholder, because oracle_enhanced uses OCI bind_params, that binds variables by number, not by name and last :a2 placeholder is empty in SQL query.

roooodcastro commented 8 years ago

@rootatdarkstar did you get your fix to work? I'm running into the exact same problem here, with this setup:

Edit:

I tried your "solution", and got this query generated:

...start_of_query_using_:a1_and_:a2) raw_sql_
                     WHERE rownum <= (:a3 + :a4)
                 )
                 WHERE raw_rnum_ > :a4

And the values bound to these parameters seem to add up, considering this is SQL for the first page of a query limited to 10 records each.

[["ano", "2016"], ["idcurso", 48], ["OFFSET", 0], ["LIMIT", 10]].

However, that didn't return any results, and it's because your placeholder is in the wrong parameter. It should be using the offset instead of limit for the raw_rnum_, so the row num is between offset and offset + limit. I switched this and my query worked as intended.

This is my hacked oracle.rb that seems to be working, (at least for me).

roooodcastro commented 8 years ago

I feel like my solution fixes this issue, but I still need to run the test suite and actually test it. I could create a PR for this, but I feel this code isn't very good and that there's a better solution somewhere. What do you guys think?

yahonda commented 8 years ago

Thanks for finding a fix. It would be appreciated if you to open a pull request even if it is not "best" solution. Then arel committers can review it and it can be merged.