Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

hardcoded primary key #43

Closed
Excelan opened this issue Jun 30, 2010 · 8 comments
Closed

hardcoded primary key #43

Excelan opened this issue Jun 30, 2010 · 8 comments

Comments

@Excelan
Copy link

Excelan commented Jun 30, 2010

There is add_limit_offset_for_association_limiting!(sql,options) method wich contains hardcoded primary_key column name - "id":

  def add_limit_offset_for_association_limiting!(sql, options)
    sql.replace %|
      SET NOCOUNT ON
      DECLARE @row_number TABLE (row int identity(1,1), id int)
      INSERT INTO @row_number (id)
        #{sql}
      SET NOCOUNT OFF
      SELECT id FROM (
        SELECT TOP #{options[:limit]} * FROM (
          SELECT TOP #{options[:limit] + options[:offset]} * FROM @row_number ORDER BY row
        ) AS tmp1 ORDER BY row DESC
      ) AS tmp2 ORDER BY row
    |.gsub(/[ \t\r\n]+/,' ')
  end

while ActiveRecord in associations.rb processes parametr primary_key:

    def select_limited_ids_list(options, join_dependency)
      pk = columns_hash[primary_key]

      connection.select_all(
        construct_finder_sql_for_association_limiting(options, join_dependency),
        "#{name} Load IDs For Limited Eager Loading"
      ).collect { |row| connection.quote(row[primary_key], pk) }.join(", ")
    end

So it doesn't work with custom :primary_key option

@metaskills
Copy link
Contributor

Totally understood. The hacks we have had to do in the 2.x version of the adapter are not the best and the work in the 3.x version avoids all this. So this will be a 2.x patch.

Our options are limited to raw string scanning to find a table name and then reflect on the primary key of that table, again assuming we find a properly named AR class based on the singular table name. Or trusting the SQL format enough to find an ID in it. Or ultimately falling back to just id as coded now. There are helpers in the adapter for doing this.

Would you care to take a stab at it, write a regression tests and submit a patch? If so, this work should be done in the 2-3-stable remote tracking branch.

@Excelan
Copy link
Author

Excelan commented Jun 30, 2010

I'm really sorry Ken, for now I'm just a blind kitten in ruby and rails. :(
I'd love to, but googling tutorails isn't a quick way to mastering ruby :)
Probably I'll try.

@metaskills
Copy link
Contributor

That's OK, I can do this, it will just take me awhile, but I will get back to this after the rails3 work.

@Excelan
Copy link
Author

Excelan commented Jun 30, 2010

I was excited rail couple months ago, and then again after the railsconf and forthcoming rails 3 anouncement. So I was keen to apply rails3 power to my legacy database. Soon i found out that there is no sqlserver-adapter for rails3 yet and started with rails 2.3
So rails 3 adapter is definitely preferable :) Thank you, for all your work.

There are some other hardcoded restrictions. For example it is also imposible to organize abstract connection (:abstract=>true) with legacy column names. So I create views with proper names as a workaround.

@metaskills
Copy link
Contributor

Interesting, I've used abstract classes in 2.3 w/adapter for some time. Can you be specific? FYI, the adapter has great support for views too.

@Excelan
Copy link
Author

Excelan commented Jun 30, 2010

Shot me. :)

I meant :polymorphic

belongs_to :Owner, :polymorphic => true, :primary_key => :Id, :foreign_key => :OwnerId

won't work, because of hardcoded "_id" and "_type" everywhere

@metaskills
Copy link
Contributor

Tell you what, I'm gonna close this ticket because I doubt that I'll be doing anything related to limit/offset or eager loading in the 2.3 branch. It's just too hard and after so much time put to the 3 version, I'm just not able to help.

Here is what I suggest. The rails 3 version is passing all tests in ActiveRecord for rails 3, tracking the 3-0-stable branch of rails, currently unreleased. So if you freeze to edge 3-0-stable rails then use the current master branch of the adapter, you should be OK.

@Excelan
Copy link
Author

Excelan commented Aug 9, 2010

That's ok. Since there is a workaround - one can use views with required names, and this branch become obsolete, it is a clever decission to concentrate efforts on version 3 branch.

Please close the ticket.

This issue was closed.
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

No branches or pull requests

2 participants