Skip to content

Conversation

@kjvarga
Copy link

@kjvarga kjvarga commented Apr 19, 2011

Hi there,

Could you merge in this patch? The SQLServer adapter indiscriminately messes up the ORDER BY for other adapters too e.g MySQL.

Here's my own spec that I'm running to test my changes FYI:

require 'spec_helper'

describe 'SQLServer Adapter' do
it "should not mess up the ORDER BY" do
Artist.order('FIELD(id,1,2,3)').to_sql.should == 'SELECT artists.* FROM artists WHERE artists.is_valid = 1 ORDER BY name ASC, FIELD(id,1,2,3)'
end

it "should mess up the ORDER BY" do
Arel::Visitors::SQLServer.expects(:===).returns(true)
Artist.order('FIELD(id,1,2,3)').to_sql.should == 'SELECT artists.* FROM artists WHERE artists.is_valid = 1 ORDER BY name ASC, FIELD(id ASC, 1 ASC, 2 ASC, 3) ASC'
end
end

Thanks,
Karl

@kjvarga
Copy link
Author

kjvarga commented Apr 19, 2011

It seems like you don't want to be doing this kind of processing on the ORDER clause anyways because basically any clause with a comma in it will be broken up and processed when that's probably not what the user had in mind.

Like what if you're using COALESCE in the ORDER clause?

@metaskills
Copy link
Member

Looks good, I prefer this style too. I'll look at testing this tonight and get back to you.

@metaskills
Copy link
Member

FYI, I made a commit on my own that added a simple guard vs merging this one in. I have some big chances in the Rails31 branch and I wanted to keep the diff simple as to avoid conflicts and confusion when that branch is rebased back to master. Can you please confirm this is OK for you?

@kjvarga
Copy link
Author

kjvarga commented Apr 29, 2011

Yes it's working for me.

Thanks.

modsognir pushed a commit to australian-medical-council/activerecord-sqlserver-adapter that referenced this pull request May 8, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants