rails-sqlserver / activerecord-sqlserver-adapter

SQL Server Adapter For Rails
MIT License
972 stars 558 forks source link

Problems with ActiveRecord models that are paginated #167

Closed metalbot closed 12 years ago

metalbot commented 12 years ago

I've been using the sql server adapter to connect a rails front end to a legacy database. It's been working great for everything that I need it to do.

However, I started playing with the activeadmin gem yesterday and started to see an odd error:

TinyTds::Error: No column name was specified for column 2 of 'rnt'.: EXEC sp_executesql N'SELECT TOP (1) [__rnt].* FROM ( SELECT ROW_NUMBER() OVER (ORDER BY [Payments].[UPaymentID] ASC) AS [rn], 1 FROM [Payments] ) AS [rnt] WHERE [rnt].[rn] > (0) ORDER BY [rnt].[__rn] ASC'

I think I've narrowed down what the problem is. ActiveAdmin uses the kaminari gem for pagination. For the most part this works fine, until we call the exists? method of an ActiveRecord::Relation that has the kaminari pagination applied.

If you want to reproduce this, add kaminari to a rails project. This will automatically support pagination. From the rails console, grab an ActiveRecord:: Relation from your model , for example:

p = Payment.limit(10).order("payment_date") # gives us an ActiveRecord::Relation p.exists? #works fine here p = Payment.page(1).per(5) # gives us another ActiveRecord::Relation p.exists? # blows up here with the TinyTDS error above

metaskills commented 12 years ago

I believe your issue duplicates #161 which was fixed in 3.2.1 of the adapter?

metalbot commented 12 years ago

Disclaimer: It's possible I'm stupid. :)

It turns out that I was on an older version of the adapter. However, updating to 3.2.1 (and updating rails to 3.2) does not resolve this problem. I can confirm that I'm on the 3.2.1 gem.

I'm fairly sure that I can write a failing unit test, however to do so I would need to add the kamanari gem to the :test group. Would that be helpful?

I looked through the commit for #161, but I don't see how that would resolve this particular problem. It's the visit_Arel_nodes_SelectStatementWithOutOffset that seems problematic. The Arel object that is passed has a projections of ["1"] which becomes an unnamed column that we try to select from within the FROM statement.

metaskills commented 12 years ago

No your good. Let me give you some back history. A lot of pagination gems on top of SQL Server tend to blow up because other databases allow you to return an non-deterministic result when it comes to limit offset. So code like this Payment.page(1).per(5) will never work because SQL Server always wants an order. I think that is the root of your issue.

Now, I have tried to filling in the blanks going all the way back to the 2.1 adapters. Any time there is no order, I find the table name, go back in and ask for its primary key. That's what the rowtable_orders does in the visitor now and the #161 issue was to further cast out non-deterministic results within the inner set.

So that out of the way, this could be related or not to the issue at hand. Upon closer look, I can see what your saying, but would need more handholding. So by all means play with the tests and add kaminari, but if you can, please submit a patch that does not have that as a development dep. I would first like to understand what the patch might look like and understand who's error it is.

metalbot commented 12 years ago

I can reproduce this without kaminari.

context 'When selecting with limit and offset' do

should 'work when determining if a collection exists' do assert Book.offset(3).exists? end

metaskills commented 12 years ago

Sweet, that makes things easy :) Well, maybe. Now for the patch. Will be busy for awhile from this front.

metalbot commented 12 years ago

I have a patch that "fixes" this, but clearly doesn't do so in an elegant way. All I'm doing is aliasing any column named "1" to "1 [__wrp]" in the visit_Arel_Nodes_SelectStatementWithOutOffset method:

(projections.map{ |x| visit(x) == "1" ? "1 [__wrp]" : visit(x) }.join(', ')),

It passes the full suite of unit tests, but seems like a pretty clunky way to handle a single obscure case.

ivanoats commented 12 years ago

I was able to reproduce this too, but I patched line 173 like you did and it did not resolve my issue. I get active record trying to look for the table in my main mysql database, not my sqlserver database.

metalbot commented 12 years ago

You need to patch line 159, not 173. I've got a pull request pending with my changes if you want to take a look.

ivanoats commented 12 years ago

thanks! saw your patch, tried it, unfortunately i guess it didn't resolve my issue. I'll try to figure out what's going on but I don't know much about sqlserver adapter internals

metalbot commented 12 years ago

Actually from your description of the problem 2 posts up, this doesn't sound like an adapter problem. Are you following standard practices for having different models coming from different databases? I'd suggest that you either open an issue on this or post to the google group. Ken is pretty responsive, and someone else might be able to point you in the right direction.