#377 ✓resolved
Jon Hancock

finder conditions bug

Reported by Jon Hancock | June 13th, 2008 @ 04:29 AM

The following works (dm-core 0.9.1):

SupportRequest.all(:closed_at => nil, :public => true, :order => [:created_at.desc], :limit => per_page, :offset => offset)

generates:

SELECT "id", "owner_id", "title", "budget", "public", "updated_at", "created_at", "suspended_at", "closed_at" FROM "support_requests" WHERE "public" = TRUE AND "closed_at" IS NULL ORDER BY "created_at" DESC LIMIT 5

The following does not work:

SupportRequest.all(:conditions => [:closed_at => nil, :public => true], :order => [:created_at.desc], :limit => per_page, :offset => offset)

end

generates:

SELECT "id", "owner_id", "title", "budget", "public", "updated_at", "created_at", "suspended_at", "closed_at" FROM "support_requests" WHERE publictrueclosed_at ORDER BY "created_at" DESC LIMIT 5

There is a bigger issues besides the bug, which is that I don't think the first example, that works, should be an acceptable form. Its too sloppy/permissive. I could have attributes in my table called limit or offset, which of course leads to trouble. I think its nice to have short-hand conditions where you do not have to specify the :conditions as a hash with array value but this short hand should only be allowed for calls where there are no other params.

Comments and changes to this ticket

  • Dan Kubb (dkubb)

    Dan Kubb (dkubb) June 13th, 2008 @ 08:31 AM

    • State changed from “new” to “open”
    • Assigned user changed from “Sam Smoot” to “Dan Kubb (dkubb)”

    I was going to update the :conditions option so that it could also accept a Hash rather than just an array. Any contents of the Hash would be treated as if they were conditions, regardless of their name. This would allow you to query for fields that are reserved names.

    The bug (which took me a second to find) is that you can (currently) pass in a Hash inside an Array for :conditions, and it treats it like if it were a raw String condition. An assertion should be added to ensure the first argument with the :conditions Array is a String.

  • Jon Hancock

    Jon Hancock June 16th, 2008 @ 12:34 AM

    As I'm not used to "speaking ruby" with others as I code in Shanghai away from the western ruby-ist community. Let me first state my terminology (correct me if I'm wrong please):

    1 - Association - a key/value pair such as :email => "me@you.com". I've looked through ruby docs and don't see a "first-class" name for a key-value pair as created by the "=>" operator/message. I'm calling it an Association because calling it a "key/value pair expression" seems verbose and obviates us needing a first class term for it.

    2 - Hash - a collection of associations, indexed by key, such as {:email => "me@you.com", created_at => nil}

    3 - Array - a collection such as [a, b, c]

    4 - Array of Associations - an Array such as [:email => "me@you.com", created_at => nil]

    5 - Array with one Hash - [{:email => "me@you.com", created_at => nil}]

    The reason I start off by clarifying my terms is I see some calling :conditions => true a Hash when I'm calling it an Association. I think being careful with these terms may be necessary to coming up with a clear API for finders (amongst others).

    Current docs http://datamapper.org/docs/find.... show:

    exhibitions = Exibition.all(:run_time.gt => 2, :run_time.lt => 5)

    which is an array of params which are used to create "AND" conditions for a "WHERE" clause.

    This is a nice short-hand method of saying something like:

    exhibitions = Exibition.all(:conditions => [:run_time.gt => 2, :run_time.lt => 5]) or

    exhibitions = Exibition.all(:conditions => {:run_time.gt => 2, :run_time.lt => 5}) if you want use a Hash instead of an Array. Unfortunately, there is a bug in the current datamapper (0.9.1) which causes this more formal use of the API to not work.

    There seems to be some confusion here (at least in the docs and maybe the current API is inconsistent as well.

    Here's what I think:

    Pick one method.

    Having too permissible an API will lead the code base into ActiveRecord Hell. ActiveRecord is horrible about not throwing errors if the options are not sane or valid. To be fair, I don't think this says that ActiveRecord is poorly written but rather should be a lesson in how futile it is to provide full coverage for highly permissible APIs.

    IMO, An API with built-in collision between attribute conditions (where clause :conditions) and query options (:limit or :order_by) is broken by design. Its broke because people will eventually create queries they didn't expect and it makes the API so permissible that having safe and quick runtime checking quickly makes the dm code too complex.

    So pick one...

    without being expert enough to give a definitive answer, here's a quick sketch:

    "finder" calls with only attribute value conditions (an Array of Associations, "array" being in how ruby handles this form of param passing) can be done without specifying that they are :conditions.

    Such as: exhibitions = Exibition.all(:run_time.gt => 2, :run_time.lt => 5)

    "finder" calls with anything extra must declare which associations are the attribute conditions:

    exhibitions = Exibition.all(:conditions => {:run_time.gt => 2, :run_time.lt => 5}, :order => [:created_at.desc])

    The choice as to if you use an Array of Associations for the conditions or a Hash is not important to me but just use one method so the docs are clear.

    Either use:

    1 - exhibitions = Exibition.all(:conditions => {:run_time.gt => 2, :run_time.lt => 5}, , :order => [:created_at.desc]) - maybe more time efficient for constructing the query to have a hash?

    2 - exhibitions = Exibition.all(:conditions => [:run_time.gt => 2, :run_time.lt => 5], , :order => [:created_at.desc]) - fits nicer with the Array used for :order_by, also maybe more space efficient ?

    3 - exhibitions = Exibition.all(:conditions => [{:run_time.gt => 2, :run_time.lt => 5}], , :order => [:created_at.desc]) - An array with a hash maybe enables to have easier OR ops. That is one hash witht he ANDs and a second Hash in the Array for the ORs. Probably not what you want...but just shows how this could get out of hand.

    I'm guessing you want option 1 or 2. But please pick one and only one. It makes documentation much easier and will certainly enable datamapper to be trusted as something that can do more strict param checking.

  • Dan Kubb (dkubb)

    Dan Kubb (dkubb) June 16th, 2008 @ 10:50 PM

    • State changed from “open” to “resolved”

    If you look at the code for the Query object there is a ton of user input validation. It's pretty restrictive in what it will allow for the reserved options, and treats any unknown key/value pairs as conditions. There is at least 2-3 dozen lines of code asserting what is allowed for each option when the Query object is initialized.

    With the exception of a possible ":group" or ":group_by" option this interface is pretty much set in stone and will not be changed at all in the future.

    Originally :conditions was going to allow a Hash or an Array, so that there was a way to pass in conditions for attributes that are reserved words. Your ticket just reminded me that there was documentation referring to it, but that it wasn't implemented.

    I can appreciate your point of view of wanting a restrictive API. You'll noticed that most of DM is built that way. The only places where we are more permissive is when we interface with user-space code, which is only in a few places. And when this happens we normalize the incoming data to our native objects/types, which themselves have assertions to maintain integrity in their own internal state. We use a whitelisting approach to ensure that data matches our expectations, and exceptions will be thrown when this isn't the case. All the methods that form part of DM's public UI are build this way.

    The following commit was made that added the ability for :conditions to be a Hash:

    http://github.com/sam/dm-core/co...

  • Jon Hancock

    Jon Hancock June 17th, 2008 @ 01:24 AM

    Thanks for the thorough replies on this.

    I do notice your git commit comment says:

    This is necessary to pass in conditions where the attribute is a

    reserved word in the Query options hash. You're out of luck if your

    attribute is "conditions" though :)

    This resolves ticket [337]

    I'll press on with my issue a little more in that I feel the formal API should be:

    find(:conditions => {:name => name, :date => date}, ... other options)

    ...and the find without the :conditions option explicitly specified is the "shorthand" version which should be internally inferred into the formal structure. Does your comment regarding collision of a field named "conditions" apply only to the shorthand version? Or would I still have a name collision on an attribute called :conditions even if I put this into the explicit :conditions hash?

  • Dan Kubb (dkubb)

    Dan Kubb (dkubb) June 17th, 2008 @ 06:55 PM

    • Milestone cleared.

    Now that I think about it, the comment in the commit is probably wrong -- you're not out of luck if the attribute is :conditions, it should work just fine.

    You're correct that the more permissive nature of the query syntax will occasionally cause collisions when people try to use attribute names that are reserved, although this will be relatively rare. In the last year or so that DM's been around with this syntax I can't remember one reported issue about this.

    As far as the cleanliness of the API, we're basically talking about where to shift the burden to: the library user or library developer. We could've made things easier on the dev team and just put all :conditions into a Hash and called it a day. However we decided to optimize for the common case and deal with the rare collision.

    I'm reviewing the code now and doing it your way it might save maybe 2 or 3 lines of library code, and maybe a dozen altogether if you factor in spec code; not enough of a savings to warrant making the common case more difficult for library users. Even if this required a hundred lines of code to deal with, I think it'd be a fair trade off.

    I think at this point we're going to have to agree to disagree though. You're welcome to write all the queries using the explicit :conditions parameter though... while I, and most other users will continue to use the current abbreviated syntax.

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.

New-ticket Create new ticket

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

People watching this ticket

Tags

Pages