It looks like I am missing something obvious. ActiveRecord is
generating _really_ bad SQL for this configuration, and I can''t quite
figure it out.
I''ve instrumented ActiveRecord enough to localize the problem somewhat,
and generally by this time I would have a good idea of what I was
missing because it''s all in the source.
It appears that something in ActiveRecord.construct_finder_sql is
choking on something I am doing. Any ideas would be appreciated.
Thanks in advance,
David Johnson
Here''s my code that is failing:
--------------------------------
def create
# changed because we are instantiating the presentations here
# may not be needed if we instantiate presentations in the initialize,
# but that will require thinking because we already have a use for
# the initialize method.
@question = Question.new
STDOUT.print("\r\n------------------------------\r\n")
STDOUT.print("Looking for Parent=
",params[:parent_quiz],"\r\n")
aParent = Quiz.find(:conditions => ["id=?",params
[:parent_quiz]], :limit => 1)
STDOUT.print("Found Parent= ",aParent,"\r\n") # aParent is
returning
nil, but it shouldbe a valid object
Here''s the STDOUT log (I have instrumented ActiveRecord to print the
SQL
before it tries to run it):
--------------------------------
Looking for Parent= ff2d7022-be0a-11da-9f01-00400506faf5
Initial request = nil
SQL Conditions = ''nil''
Have one ID to check: ff2d7022-be0a-11da-9f01-00400506faf5
find all - finder_sql= SELECT FIRST 1 * FROM quizzes WHERE (quizzes.id
= ''--- !map:HashWithIndifferentAccess
ff2d7022-be0a-11da-9f01-00400506faf5: ""
'')
I instrumented ActiveRecord.find
More data: The faulty option appears to be generated in this code snippet from ActiveRecord.first> case ids.size > when 0 > raise RecordNotFound, "Couldn''t find #{name} without an ID#{conditions}" > when 1 > > STDOUT.print("\r\nHave one ID to check: ",ids.first,"\r\n") > # this merge is generating the faulty condition in the SQL > if result = find(:first, options.merge({ :conditions => "#{table_name}.#{primary_key} = #{sanitize(ids.first)}#{conditions}" })) > return expects_array ? [ result ] : result > else > raise RecordNotFound, "Couldn''t find #{name} with ID=#{ids.first}#{conditions}" > end > else > # Find multiple ids > ids_list = ids.map { |id| sanitize(id) }.join('','') > result = find(:all, options.merge({ :conditions => "#{table_name}.#{primary_key} IN (#{ids_list})#{conditions}"})) > if result.size == ids.size > return result > else > raise RecordNotFound, "Couldn''t find all #{name.pluralize} with IDs (#{ids_list})#{conditions}" > end > endOn Sat, 2006-04-01 at 13:57 -0600, David Johnson wrote:> It looks like I am missing something obvious. ActiveRecord is > generating _really_ bad SQL for this configuration, and I can''t quite > figure it out. > > I''ve instrumented ActiveRecord enough to localize the problem somewhat, > and generally by this time I would have a good idea of what I was > missing because it''s all in the source. > > It appears that something in ActiveRecord.construct_finder_sql is > choking on something I am doing. Any ideas would be appreciated. > > Thanks in advance, > David Johnson > > > > > Here''s my code that is failing: > -------------------------------- > > def create > # changed because we are instantiating the presentations here > # may not be needed if we instantiate presentations in the initialize, > # but that will require thinking because we already have a use for > # the initialize method. > @question = Question.new > STDOUT.print("\r\n------------------------------\r\n") > STDOUT.print("Looking for Parent= ",params[:parent_quiz],"\r\n") > aParent = Quiz.find(:conditions => ["id=?",params > [:parent_quiz]], :limit => 1) > STDOUT.print("Found Parent= ",aParent,"\r\n") # aParent is returning > nil, but it shouldbe a valid object > > > > > Here''s the STDOUT log (I have instrumented ActiveRecord to print the SQL > before it tries to run it): > -------------------------------- > Looking for Parent= ff2d7022-be0a-11da-9f01-00400506faf5 > > Initial request = nil > > SQL Conditions = ''nil'' > > > Have one ID to check: ff2d7022-be0a-11da-9f01-00400506faf5 > > find all - finder_sql= SELECT FIRST 1 * FROM quizzes WHERE (quizzes.id > = ''--- !map:HashWithIndifferentAccess > ff2d7022-be0a-11da-9f01-00400506faf5: "" > '') > > I instrumented ActiveRecord.find > > > > > > _______________________________________________ > Rails mailing list > Rails@lists.rubyonrails.org > http://lists.rubyonrails.org/mailman/listinfo/rails
I got past the initial problem (but I had to dig bloody deep to find
it). Apparently, params[:parent_quiz] was returning a
HashWithIndifferentAccess. Casting it as a String using params
[:parent_quiz].to_s resolved that problem.
However, I am still getting bad SQL in the find invoked by the save.
The parent_id should be quoted.
As I am still a ruby-newbie, I would greatly appreciate input from more
experienced Ruby folks as to the ideal resolution. I believe that it is
to replace configuration[:scope].to_s with quote(configuration[:scope]).
Ideas anyone?
Example:
-----------------
SELECT FIRST 1 * FROM questions WHERE (parent_id = ff2d7022-
be0a-11da-9f01-00400506faf5) ORDER BY seq DESC
-----------------
The code snippet from ActiveRecord.Acts.List is generating the SQL with
unquoted text:
-----------------
if configuration[:scope].is_a?(Symbol)
scope_condition_method = %(
def scope_condition
if #{configuration[:scope].to_s}.nil?
"#{configuration[:scope].to_s} IS NULL"
else
#Unquoted SQL is generated by this line.
"#{configuration[:scope].to_s} = \#{#{configuration
[:scope].to_s}}"
end
end
)
else
scope_condition_method = "def scope_condition() \"#
{configuration[:scope]}\" end"
end
-----------------
I have submitted a patch for the failure to quote the ID. I would appreciate feedback for any corrections that may be necessary. I still don''t have the hang of Ruby''s style of inline string substitutions. Ticket URL: <http://dev.rubyonrails.org/ticket/4538> Thanks On Sat, 2006-04-01 at 18:33 -0600, David Johnson wrote:> I got past the initial problem (but I had to dig bloody deep to find > it). Apparently, params[:parent_quiz] was returning a > HashWithIndifferentAccess. Casting it as a String using params > [:parent_quiz].to_s resolved that problem. > > However, I am still getting bad SQL in the find invoked by the save. > The parent_id should be quoted. > > As I am still a ruby-newbie, I would greatly appreciate input from more > experienced Ruby folks as to the ideal resolution. I believe that it is > to replace configuration[:scope].to_s with quote(configuration[:scope]). > > Ideas anyone? > > > > Example: > ----------------- > SELECT FIRST 1 * FROM questions WHERE (parent_id = ff2d7022- > be0a-11da-9f01-00400506faf5) ORDER BY seq DESC > ----------------- > > > > The code snippet from ActiveRecord.Acts.List is generating the SQL with > unquoted text: > ----------------- > > if configuration[:scope].is_a?(Symbol) > scope_condition_method = %( > def scope_condition > if #{configuration[:scope].to_s}.nil? > "#{configuration[:scope].to_s} IS NULL" > else > #Unquoted SQL is generated by this line. > "#{configuration[:scope].to_s} = \#{#{configuration > [:scope].to_s}}" > end > end > ) > else > scope_condition_method = "def scope_condition() \"# > {configuration[:scope]}\" end" > end > > ----------------- > > > _______________________________________________ > Rails mailing list > Rails@lists.rubyonrails.org > http://lists.rubyonrails.org/mailman/listinfo/rails
On Apr 1, 2006, at 11:57 AM, David Johnson wrote:> It looks like I am missing something obvious. ActiveRecord is > generating _really_ bad SQL for this configuration, and I can''t quite > figure it out.snip...> Here''s my code that is failing: > -------------------------------- > > def create > # changed because we are instantiating the presentations here > # may not be needed if we instantiate presentations in the initialize, > # but that will require thinking because we already have a use for > # the initialize method. > @question = Question.new > STDOUT.print("\r\n------------------------------\r\n") > STDOUT.print("Looking for Parent= ",params[:parent_quiz],"\r\n") > aParent = Quiz.find(:conditions => ["id=?",params > [:parent_quiz]], :limit => 1)This line should read: aParent = Quiz.find(params[:parent_quiz]) Not clear why you''d name the id of the parent record :parent_quiz, seems :parent_id would be much more readable. -- -- Tom Mornini
On Sat, 2006-04-01 at 22:00 -0800, Tom Mornini wrote:> On Apr 1, 2006, at 11:57 AM, David Johnson wrote: > > > It looks like I am missing something obvious. ActiveRecord is > > generating _really_ bad SQL for this configuration, and I can''t quite > > figure it out. > > snip... > > > Here''s my code that is failing: > > -------------------------------- > > > > def create > > # changed because we are instantiating the presentations here > > # may not be needed if we instantiate presentations in the initialize, > > # but that will require thinking because we already have a use for > > # the initialize method. > > @question = Question.new > > STDOUT.print("\r\n------------------------------\r\n") > > STDOUT.print("Looking for Parent= ",params[:parent_quiz],"\r\n") > > aParent = Quiz.find(:conditions => ["id=?",params > > [:parent_quiz]], :limit => 1) > > This line should read: > > aParent = Quiz.find(params[:parent_quiz]) >Still fails ... needs to be Quiz.find(params[:parent_quiz].to_s). For some reason params[:parent_quiz] is a HashWithIndifferentAccess (not a string) that needs to be dereferenced before it is passed down into the lower layers. Also, see my patch to ActiveRecord.Acts.List. Primary keys that are not integers are not properly quoted on the reselect that follows the insert.> Not clear why you''d name the id of the parent record :parent_quiz, seems > :parent_id would be much more readable. >
On Sat, 2006-04-01 at 22:00 -0800, Tom Mornini wrote:> Not clear why you''d name the id of the parent record :parent_quiz, > seems > :parent_id would be much more readable.Because (1) there are places in the overall data model where the "parent" relationship is a bit ambiguous, (2) I am trying to focus on the learning Ruby and Rails, (3) this will be refactored into session properties once I get the relational model idioms sorted out (I hate the idea of sending session information back to the browser). This particular application is an exercise I do whenever I start learning a new framework. It happens to be immediately useful to me, but there is no requirement that I succeed in completing the task. I don''t expect to have to maintain this code base after it is complete. It is just complex enough and breaks enough framework builders'' expectations to expose weaknesses in the handling of relational models. But it is not so large or complex as to become unmanageable whatever the problems are that I run into. Thanks for the feedback. I would really appreciate your review of my proposed patch at <http://dev.rubyonrails.org/ticket/4538>. Thanks.
On Apr 2, 2006, at 10:41 AM, David Johnson wrote:> On Sat, 2006-04-01 at 22:00 -0800, Tom Mornini wrote: >> On Apr 1, 2006, at 11:57 AM, David Johnson wrote: >> >>> It looks like I am missing something obvious. ActiveRecord is >>> generating _really_ bad SQL for this configuration, and I can''t >>> quite >>> figure it out. >> >> snip... >> >>> Here''s my code that is failing: >>> -------------------------------- >>> >>> def create >>> # changed because we are instantiating the presentations here >>> # may not be needed if we instantiate presentations in the >>> initialize, >>> # but that will require thinking because we already have a use for >>> # the initialize method. >>> @question = Question.new >>> STDOUT.print("\r\n------------------------------\r\n") >>> STDOUT.print("Looking for Parent= ",params[:parent_quiz],"\r\n") >>> aParent = Quiz.find(:conditions => ["id=?",params >>> [:parent_quiz]], :limit => 1) >> >> This line should read: >> >> aParent = Quiz.find(params[:parent_quiz]) > > Still fails ... needs to be Quiz.find(params[:parent_quiz].to_s). For > some reason params[:parent_quiz] is a HashWithIndifferentAccess (not a > string) that needs to be dereferenced before it is passed down into > the > lower layers.Two things: 1) If you''re getting a HashWithIndifferentAccess, then something is wrong. Perhaps (just guessing) you need something like: params[:parent_quiz][:id] 2) Id''s should be integers, not strings.> Also, see my patch to ActiveRecord.Acts.List. Primary keys that > are not > integers are not properly quoted on the reselect that follows the > insert.Rails doesn''t support primary keys that aren''t integers. It''s likely more than acts_as_list will need to be patched to "fix" this. -- -- Tom Mornini
On Sun, 2006-04-02 at 11:47 -0700, Tom Mornini wrote:> On Apr 2, 2006, at 10:41 AM, David Johnson wrote: > > > On Sat, 2006-04-01 at 22:00 -0800, Tom Mornini wrote: > >> On Apr 1, 2006, at 11:57 AM, David Johnson wrote: > >> > >>> It looks like I am missing something obvious. ActiveRecord is > >>> generating _really_ bad SQL for this configuration, and I can''t > >>> quite > >>> figure it out. > >> > >> snip... > >> > >>> Here''s my code that is failing: > >>> -------------------------------- > >>> > >>> def create > >>> # changed because we are instantiating the presentations here > >>> # may not be needed if we instantiate presentations in the > >>> initialize, > >>> # but that will require thinking because we already have a use for > >>> # the initialize method. > >>> @question = Question.new > >>> STDOUT.print("\r\n------------------------------\r\n") > >>> STDOUT.print("Looking for Parent= ",params[:parent_quiz],"\r\n") > >>> aParent = Quiz.find(:conditions => ["id=?",params > >>> [:parent_quiz]], :limit => 1) > >> > >> This line should read: > >> > >> aParent = Quiz.find(params[:parent_quiz]) > > > > Still fails ... needs to be Quiz.find(params[:parent_quiz].to_s). For > > some reason params[:parent_quiz] is a HashWithIndifferentAccess (not a > > string) that needs to be dereferenced before it is passed down into > > the > > lower layers. > > Two things: > > 1) If you''re getting a HashWithIndifferentAccess, then something is > wrong.I agree. But what? params[:parent_quiz] is (allegedly) coming directly from the browser parameters at this point. The only thing that processes it before I get to it is the Rails framework. If there is a bug in that part of the framework then I need to understand a bit more before I expend effort trying to patch it.> Perhaps (just guessing) you need something like: > > params[:parent_quiz][:id] >Tried it. Got the Rails equivalent of "Watchu talking ''bout Willis?" (see stdout log) Couldn''t find Quiz without an ID RAILS_ROOT: script/../config/.. Application Trace | Framework Trace | Full Trace /usr/lib/ruby/gems/1.8/gems/activerecord-1.13.2/lib/active_record/base.rb:409:in `find'' ./script/../config/../app/controllers/question_controller.rb:39:in `create''> 2) Id''s should be integers, not strings. >The assumption behind this is that all databases are centralized. There are many applications where databases must be decentralized and merged on demand. Integers do not guarantee uniqueness once you have split the database across multiple pieces of hardware that cannot communicate with each other. Assigning integer ranges to hardware systems is a sure recipe for long-term problems (been there done that). An example of this is the first database application I wrote for a construction company. One of the infrastructure constraints was that most sites went in before there was communications. The cell tower was usually erected about 2 weeks after crews demobilized. But we had to be able to merge the field and office data on demand at least once per week during construction. The use of UUID''s as primary keys is accepted practice for this sort of application. Since most DBMS'' do not support UUID''s natively yet, representing them as strings is a normal practice. UUID''s are guaranteed unique in the world until the year 3040 or so because they combine physical characteristics of the generating machine, time stamp, and a random factor to generate the resulting value.> > Also, see my patch to ActiveRecord.Acts.List. Primary keys that > > are not > > integers are not properly quoted on the reselect that follows the > > insert. > > Rails doesn''t support primary keys that aren''t integers.According to "Agile" book, Rails does support this. According to reality, it appears to be just a very thin sliver shy of full support.> > It''s likely more than acts_as_list will need to be patched to "fix" > this. >I suspect that the other acts_as... methods would also require the same patch. I had to adapt a bit, but review of the Rails code showed me that if the class provides an ID on creation, Rails will not override it. I have a UUID helper mixin that I lifted from a Rails Blog that is working just fine on row creation. A large part of my purpose in using this particular model is to see how far I can take Rails'' constructs. If it can''t handle my needs then I will simply move on. So far, the only issues I have run into are in the class of minor implementation tweaks (maturity) rather than baseline design issues.
On Apr 2, 2006, at 2:38 PM, David Johnson wrote:>> 2) Id''s should be integers, not strings. >> > The assumption behind this is that all databases are centralized. > There > are many applications where databases must be decentralized and merged > on demand. Integers do not guarantee uniqueness once you have > split the > database across multiple pieces of hardware that cannot communicate > with > each other. Assigning integer ranges to hardware systems is a sure > recipe for long-term problems (been there done that).Sorry, I didn''t make this clear enough. I thought (see below) that I was stating fact, not giving advice. Rails assumes that IDs are integers.>>> Also, see my patch to ActiveRecord.Acts.List. Primary keys that >>> are not integers are not properly quoted on the reselect that >>> follows the insert. >> >> Rails doesn''t support primary keys that aren''t integers. > According to "Agile" book, Rails does support this. According to > reality, it appears to be just a very thin sliver shy of full support.You''re right! It''s been a while since I read it, and I guess I''ve heard so many people recommend against it, I forgot it was true. That said, it seems it doesn''t work properly, so perhaps I was "correct" in a rather backward way... -- -- Tom Mornini
David Johnson wrote:>> Perhaps (just guessing) you need something like: >> >> params[:parent_quiz][:id] >> > Tried it. Got the Rails equivalent of "Watchu talking ''bout > Willis?" (see stdout log) > > Couldn''t find Quiz without an IDHow does your params hash actually look? If you check your logfile, the it should be visible at the beginning of each request. Does it actually contain the values you expect?>> 2) Id''s should be integers, not strings. >> > I had to adapt a bit, but review of the Rails code showed me that if the > class provides an ID on creation, Rails will not override it. I have a > UUID helper mixin that I lifted from a Rails Blog that is working just > fine on row creation.Is anything stopping you from having both an id and a uuid column? Leave Rails to worry about the id column, which is an integer like it expects, and you use a mixin to handle the uuid? You can then do stuff like Quiz.find_by_uuid(params[:parent_quiz]) (assuming params[:parent_quiz] actually holds your uuid). That said, I imagine it would be nice if non-integer id''s worked just dandy out of the box. Not something I''ve had a need for myself, though. -- Jakob Skjerning - http://mentalized.net
On Mon, 2006-04-03 at 13:45 +0200, Jakob Skjerning wrote:> David Johnson wrote: > >> Perhaps (just guessing) you need something like: > >> > >> params[:parent_quiz][:id] > >> > > Tried it. Got the Rails equivalent of "Watchu talking ''bout > > Willis?" (see stdout log) > > > > Couldn''t find Quiz without an ID > > How does your params hash actually look? If you check your logfile, the > it should be visible at the beginning of each request. Does it actually > contain the values you expect?Yes, it contains the correct values. I''ll revisit this later this week. I have to prepare for an interview, prepare for a major installation on a live system this weekend, and learn to use svn diff to submit my proposed patch.> > >> 2) Id''s should be integers, not strings. > >> > > I had to adapt a bit, but review of the Rails code showed me that if the > > class provides an ID on creation, Rails will not override it. I have a > > UUID helper mixin that I lifted from a Rails Blog that is working just > > fine on row creation. > > Is anything stopping you from having both an id and a uuid column? Leave > Rails to worry about the id column, which is an integer like it expects, > and you use a mixin to handle the uuid?It complicates the schema and makes merging the distributed databases just a tad less straightforward. With the UUID as the PK, I can merge databases without writing any code. The integer introduces the need to actually write a program (or generator, or trigger).> > You can then do stuff like Quiz.find_by_uuid(params[:parent_quiz]) > (assuming params[:parent_quiz] actually holds your uuid). > > That said, I imagine it would be nice if non-integer id''s worked just > dandy out of the box. Not something I''ve had a need for myself, though. >It''s not the most common paradigm. But, it''s a useful one for real distributed applications and for testing the limits of new frameworks.
On Sun, 2006-04-02 at 22:45 -0700, Tom Mornini wrote:> >> Rails doesn''t support primary keys that aren''t integers. > > According to "Agile" book, Rails does support this. According to > > reality, it appears to be just a very thin sliver shy of full support. > > You''re right! It''s been a while since I read it, and I guess I''ve heard > so many people recommend against it, I forgot it was true. That said, > it seems it doesn''t work properly, so perhaps I was "correct" in a > rather > backward way... >First tenet of debate - a fact rebuts all logic. You win (until we get a couple of patches out) :o) Please accept my apologies for the tone of my response. The recommendation against strings is because many poorly designed data models use values that are actually meaningful to the business as their primary keys. The argument is not against strings, per se, but against uncontrolled free form data that is meaningful to the business and may be subject to change. Integer sequences (tokens) are the easiest mechanism to implement that separates the primary key from business meaning. Since the majority of RDBMS applications are centralized, or at least localized in nature, it is the most common mechanism used. There are other mechanisms that guarantee uniqueness, and some of those must be considered when you start using DRDBMS and you have to merge data across platforms that have only spurious communications. A string is a "least common denominator" that can represent the results of any suitable keying algorithm (including integers). Have you had a chance to review my proposed patch? I am still not entirely comfortable with the inline string substitution for self modifying code, so I am not convinced that my solution won''t break someone else'' project.