SQL Syntax error when using acts_as_nested_set

[rob99]

I am moving my rails 4.2 from Sqlite3 to MS Sql Server 2008 and I ran into this problem:

CollectiveIdea::Acts::NestedSet::Model::Transactable::OpenTransactionsIsNotZero: ActiveRecord::JDBCError: com.microsoft.sqlserver.jdbc.SQLServerException: Incorrect syntax near ')'
.: SELECT t.id FROM ( SELECT ROW_NUMBER() OVER(ORDER BY [cml_counterparties].[lft]) AS _row_num, [cml_counterparties].[id] FROM [cml_counterparties] WHERE ([cml_counterparties].[lft] BETWEEN 2 AND 4 OR [cml_counterparties].[rgt] BETWEEN 2 AND 4)) ) AS t WHERE t._row_num BETWEEN 1 AND 9223372036854775807
C:/jruby-9.1.2.0/lib/ruby/gems/shared/gems/awesome_nested_set-3.1.1/lib/awesome_nested_set/model/transactable.rb:20:in `in_tenacious_transaction'
C:/jruby-9.1.2.0/lib/ruby/gems/shared/gems/awesome_nested_set-3.1.1/lib/awesome_nested_set/model/movable.rb:103:in `block in move_to'
C:/jruby-9.1.2.0/lib/ruby/gems/shared/gems/activesupport-4.2.6/lib/active_support/callbacks.rb:88:in `__run_callbacks__'
C:/jruby-9.1.2.0/lib/ruby/gems/shared/gems/activesupport-4.2.6/lib/active_support/callbacks.rb:778:in `_run_move_callbacks'
C:/jruby-9.1.2.0/lib/ruby/gems/shared/gems/activesupport-4.2.6/lib/active_support/callbacks.rb:81:in `run_callbacks'
C:/jruby-9.1.2.0/lib/ruby/gems/shared/gems/awesome_nested_set-3.1.1/lib/awesome_nested_set/model/movable.rb:102:in `move_to'

Notice the extra closing parenthesis:

SELECT t.id 
FROM ( 
  SELECT ROW_NUMBER() OVER(ORDER BY [cml_counterparties].[lft]) AS _row_num, [cml_counterparties].[id] 
  FROM [cml_counterparties] 
  WHERE ([cml_counterparties].[lft] BETWEEN 2 AND 4 OR [cml_counterparties].[rgt] BETWEEN 2 AND 4)) 
  ) AS t 
WHERE t._row_num BETWEEN 1 AND 9223372036854775807

It was working fine under sqlite so I am assuming this is something to do with MS SQL adapter.

--- Want to back this issue? **[Post a bounty on it!](https://www.bountysource.com/issues/36359519-sql-syntax-error-when-using-acts_as_nested_set?utm_campaign=plugin&utm_content=tracker%2F136963&utm_medium=issues&utm_source=github)** We accept bounties via [Bountysource](https://www.bountysource.com/?utm_campaign=plugin&utm_content=tracker%2F136963&utm_medium=issues&utm_source=github).

[rob99]

I have tracked the problem down to somewhere in Arjdbc::MSSQL::LimitHelpers::SqlServerReplaceLimitOffset#replace_limit_offset

https://github.com/jruby/activerecord-jdbc-adapter/blob/e9366a5a25dc919e7fe7d6319410aec2e9532edb/lib/arjdbc/mssql/limit_helpers.rb#L18

The SQL going in is

UPDATE [cml_counterparties] 
SET [lft] = CASE WHEN [lft] BETWEEN N'2' AND N'2' THEN [lft] + N'4' - N'2' WHEN [lft] BETWEEN N'3' AND N'4' THEN [lft] + N'2' - N'3' ELSE [lft] END,
 [rgt] = CASE WHEN [rgt] BETWEEN N'2' AND N'2' THEN [rgt] + N'4' - N'2' WHEN [rgt] BETWEEN N'3' AND N'4' THEN [rgt] + N'2' - N'3' ELSE [rgt] END, 
 [parent_id] = CASE WHEN [id] = N'8' THEN N'1' ELSE [parent_id] END, 
 updated_at = '2016-07-26 15:58:37.753' 
 WHERE [cml_counterparties].[id] IN 
  (SELECT [cml_counterparties].[id] 
  FROM [cml_counterparties] 
  WHERE ([cml_counterparties].[lft] BETWEEN 2 AND 4 OR [cml_counterparties].[rgt] BETWEEN 2 AND 4)
  )

... but somewhere along the way it picks up an extra close paren.

[rob99]

replace_limit_offset! is a nasty piece of work. It would appear it simply doesn't handle nested SELECT!?!?

[rob99]

I am working around the issue with this shameful hack at https://github.com/jruby/activerecord-jdbc-adapter/blob/e9366a5a25dc919e7fe7d6319410aec2e9532edb/lib/arel/visitors/sql_server/ng42.rb#L103

          # HACK
          # https://github.com/jruby/activerecord-jdbc-adapter/issues/736
          # Only mess with the limit if theres a "meaningful" limit
          if limit_for(o.limit) < 9223372036854775807
            replace_limit_offset!(sql, limit_for(o.limit), o.offset && o.offset.value.to_i, select_order_by)
          end
          # END HACK

[rob99]

I see the following in https://github.com/jruby/activerecord-jdbc-adapter/blob/master/lib/arel/visitors/sql_server/ng42.rb#L28

      def visit_Arel_Nodes_UpdateStatement(o, a)
        if o.orders.any? && o.limit.nil?
          o.limit = Nodes::Limit.new(9_223_372_036_854_775_807)
        end
        super
      end

In other words, if the update doesn't have a limit, add one. I can see similar code in the non-JDBC AR MSSQL adapter. I have no idea why this is done. Surely it makes the query less efficient.

[kares]

MS-SQL is very low on :purple_heart: from our users ... the 4.2 support was done 'quickly' with no real-world testing.

I am willing to look in detail into specific issues (if there's willingness to appreciate such effort on the other end). otherwise, if you guys are to fix up something, I strongly recommend you run the suite if you do want changes upstream. there's been issues with PR for the SQLServer adapter in the past that caused regressions (recall it was limit-offset related change). these parts might be ripe for a higher level dedicated refactoring :)

[rob99]

Hi Karol,

I certainly appreciate all the work you've done here - I use it every day! I'm in the process of setting up a SQL Server 2008 server so I can try to contribute something (Microsoft don't make it easy, do they!?).

One thing I'm confused about it the relationship between ActiveRecord versions, activerecord-jdbc-adapter versions, and SQL Server versions. If there's any wiki pages, old issues or anything I should read to clarify - please advise.

I am interested in Rails 4.2 and SQL Server 2008, but want to be sure I don't break things for other combinations.

BTW jTDS seem to be no longer maintained, and is not JDBC4 compliant (it's JDBC3). This adapter is also emitting incorrect SQL syntax when using jTDS and AR 4.2. So us poor SQL Server developers are really in a bind! Deprecating jTDS and supporting the MS sqljdbc4 driver seems the best way forward.

[kares]

AR-JDBC 1.3 (1-3-stable branch) supports multiple AR versions from a single base (see .travis.yml).

just run the suite before and after the change on 4.2 that will be most important - feel free to add new test cases (you could maybe confirm there's no regressions by running against 3.2 once you're PR ready). in terms of SQLServer - all the way down to 2000 used to be supported (not sure how much it is needed but maybe make sure there aren't changes breaking compatibility if you'd like changes upstream for 1.3.x).

historically adapter: mssql used the jTDS driver but later (as its no loner maintained and thus might not handle newer DB versions correctly) support for the official driver was added.