Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Issue #176 - fix ms sql 2000 - wrong primary key where nonstandard table name is used #177

Merged
merged 5 commits into from

3 participants

@gregors

Issue #176 - fix ms sql 2000 - wrong primary key where nonstandard table name is used.

I added a failing test then added passing code. I've only tested against rails3.2.1. For some reason I can't seem to install older versions of rails properly at the moment but I did add code which should work under ActiveRecord 2.3.0 if someone would test to be sure.

@jrubyci
Collaborator
@gregors

I have and get the following output - any suggestions using jruby 1.6.7 both with --1.8 and --1.9

$ rake appraisal:install

bundle install --gemfile=C:/Documents and Settings/gregory.ostermayr/My Documents/activerecord-jdbc-adapter/gemfiles/rails23.gemfile
C:/Documents not found

@nicksieger
Owner

Bummer -- the old "filename with spaces" strikes again. Might need to patch the appraisal gem...

@gregors

Duh - I totally didn't understand what I was seeing. I'll move it to a path without spaces and try it tonight. Thanks!

@gregors

I moved the repo to a place without spaces in the path and did rake appraisal:install which installed all the gems, however, when I tried to run the actual test it still pukes. I'm assuming it's some kind of windows weirdness I even uninstalled jruby 1.6.7 and tried 1.6.6 both 1.8 and 1.9 no dice. If I try it from on a mac it works fine - but I don't have access to the windows database I'm using. So.....

I ran each of the tests individually by manually changing the projects main Gemfile and specifying precisely which activerecord I was using and this worked as I could see which activerecord was being run in the tests.

I reverted the repo back before I touched anything to get a baseline these are the results:
2.3.14 = lots of errors
3.0.11 = 1 failed test (sys.views test - which I don't think can pass on sql server 2000)
3.1.3 = same as 2.3.14
3.2.1 = same as 3.0.11

Now this is somewhat strange the test with 'lots of errors' all had to do with "ROW_NUMBER is not a recognized function name" which makes sense since yes that didn't exist in SQL Server 2000 - but why does that work in 3.0.11? How does it know? I might look into that later.

Anyway, I apply my changes add my test:
2.3.14 = lots of errors
3.0.11 = ( 1 failed sys.view test)
3.1.3 = same as 2.3.14
3.2.1 = same as 3.0.11

So my fix for this issue hasn't caused any issues which were not already present and adds the ability to use legacy nonstandard primary key ids. The only thing I don't like about my solution is the possible issues if you have an obscene amount of models. A better solution would probably better to pass such information down to the helper from a higher level. I may revisit that in the future but presently for my needs it works.

Is there anything else I need to provide for these changes to be acceptable?

@gregors

Any feedback on this would be appreciated.

@nicksieger nicksieger merged commit 889f8ed into from
@nicksieger
Owner

I went ahead and merged it, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 28, 2012
  1. @gregors

    attempt to add failing test

    gregors authored
  2. @gregors

    add failing test

    gregors authored
    fix typos
  3. @gregors

    add get primary key helper

    gregors authored
    get non-standard primary keys
    get failing test to pass
Commits on Feb 29, 2012
  1. @gregors
  2. @gregors

    add rails 2 compatibility

    gregors authored
This page is out of date. Refresh to see the latest.
View
24 lib/arjdbc/mssql/limit_helpers.rb
@@ -12,6 +12,28 @@ def get_table_name(sql)
end
end
+ def get_primary_key(order, table_name)
+ if order =~ /(\w*id\w*)/i
+ $1
+ else
+ table_name[/\[+(.*)\]+/i]
+
+ # rails 2 vs rails 3
+ if ActiveRecord::VERSION::MAJOR >= 3
+ models = ActiveRecord::Base.descendants
+ else
+ models = ActiveRecord::Base.send(:subclasses)
+ end
+
+ model = models.select{|model| model.table_name == $1}.first
+ if model then
+ model.primary_key
+ else
+ 'id'
+ end
+ end
+ end
+
module SqlServer2000ReplaceLimitOffset
module_function
def replace_limit_offset!(sql, limit, offset, order)
@@ -31,7 +53,7 @@ def replace_limit_offset!(sql, limit, offset, order)
rest = rest_of_query[/FROM/i=~ rest_of_query.. -1]
#need the table name for avoiding amiguity
table_name = LimitHelpers.get_table_name(sql)
- primary_key = order[/(\w*id\w*)/i] || "id"
+ primary_key = LimitHelpers.get_primary_key(order, table_name)
#I am not sure this will cover all bases. but all the tests pass
if order[/ORDER/].nil?
new_order = "ORDER BY #{order}, #{table_name}.#{primary_key}" if order.index("#{table_name}.#{primary_key}").nil?
View
30 test/mssql_limit_offset_test.rb
@@ -3,6 +3,26 @@
ActiveRecord::Schema.verbose = false
+class CreateLegacyShips < ActiveRecord::Migration
+
+ def self.up
+ create_table "legacy_ships",{:primary_key => :ShipKey} do |t|
+ t.string "name", :limit => 50, :null => false
+ t.integer "width", :default => 123
+ t.integer "length", :default => 456
+ end
+ end
+
+ def self.down
+ drop_table "legacy_ships"
+ end
+
+end
+
+class LegacyShip < ActiveRecord::Base
+ self.primary_key = "ShipKey"
+end
+
class CreateLongShips < ActiveRecord::Migration
def self.up
@@ -64,6 +84,7 @@ class NoIdViking < ActiveRecord::Base
class MsSQLLimitOffsetTest < Test::Unit::TestCase
def setup
+ CreateLegacyShips.up
CreateLongShips.up
CreateVikings.up
CreateNoIdVikings.up
@@ -71,6 +92,7 @@ def setup
end
def teardown
+ CreateLegacyShips.down
CreateVikings.down
CreateLongShips.down
CreateNoIdVikings.down
@@ -84,6 +106,14 @@ def test_limit_with_no_id_column_available
end
end
+ def test_limit_with_alternate_named_primary_key
+ %w(one two three four five six seven eight).each do |name|
+ LegacyShip.create!(:name => name)
+ end
+ ships = LegacyShip.limit(3)
+ assert_equal(3, ships.size)
+ end
+
def test_limit_and_offset
%w(one two three four five six seven eight).each do |name|
LongShip.create!(:name => name)
Something went wrong with that request. Please try again.