rails-sqlserver / activerecord-sqlserver-adapter

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

Duplicate Columns in Order By #203

Closed Lunat1k closed 12 years ago

Lunat1k commented 12 years ago

Please see this ticket http://www.redmine.org/issues/10930

I wanted to open a ticket here too to see if it is an adapter issue or a redmine issue.

metaskills commented 12 years ago

I have no idea without a stack trace and then I would have to look at the redmine code. But I doubt I have time to do that. I doubt as well that this is an adapter bug. After looking at the some of the code examples in that ticket I would easily deduce that the issue is most likely with the way their ActiveRecord code is written. Like why in the hell would they do this line?

Enumeration.update_all("is_default = #{connection.quoted_false}", {:type => type})
# vs
Enumeration.update_all({:is_default => false, {:type => type})

Standard idioms would allow ActiveRecord and each adapter to do the work they are supposed to do. Seems like a lot of their code works harder vs smarter. So please QA their code first.

Lunat1k commented 12 years ago

This is why I have an open ticket with them. I just wanted to get your feedback on the issue so I have something to tell them.

metaskills commented 12 years ago

OK, give em hell :smile:

Lunat1k commented 12 years ago

So let me get your feedback on this. I am getting a duplicate order by with this code.

In subversion.rb they have

  def latest_changesets(path, rev, limit=10)
    revisions = scm.revisions(path, rev, nil, :limit => limit)
    revisions ? changesets.find_all_by_revision(revisions.collect(&:identifier), :order => "committed_on DESC", :include => :user) : []
  end

Which then is causing this error.

ActiveRecord::StatementInvalid (TinyTds::Error: A column has been specified more than once in the order by list. Columns in the order by list must be unique.: EXEC sp_executesql N'SELECT [changesets].* FROM [changesets] WHERE [changesets].[repository_id] = 1 AND [changesets].[revision] IN (N''72967'', N''70725'', N''69665'', N''67166'', N''63501'', N''59564'', N''59111'', N''59110'', N''59108'', N''58614'') ORDER BY changesets.committed_on DESC, changesets.id DESC, committed_on DESC'):
  app/models/repository/subversion.rb:43:in `latest_changesets'
  app/controllers/repositories_controller.rb:122:in `show'

I looked here and I really didn't see anything off hand that would cause it. http://www.redmine.org/projects/redmine/repository/entry/trunk/app/models/changeset.rb

The only thing I can think of is maybe something with this file is causing the issue. http://www.redmine.org/projects/redmine/repository/entry/trunk/app/helpers/sort_helper.rb

Would you mind take a quick peek at those files and see if you can find anything that jumps out at you that I can go back to them about?

Lunat1k commented 12 years ago

Hmm I did a grep on the files for commited_on I did come across this on line 24. I wonder if between the subversion.rb and repository.rb code it is causing it to add it twice. Opionions since I'm not that familiar with ruby yet.

http://www.redmine.org/projects/redmine/repository/entry/trunk/app/models/repository.rb

metaskills commented 12 years ago

Duplicate orders is something that SQL Server will not allow, our visitor goes to great length to convert order strings into real order objects that we can uniq and remove. To allow the adapter and other DBs that do not allow multiple order, they should match up. In this case if the second order was fully qualifed to changesets.committed_on DESC it would work.

Lunat1k commented 12 years ago

Bingo. I added #{Changeset.table_name}.commited_on to their subversion.rb file now it works ;) I will go back to them and get them to fix it. Thanks again for your help