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) 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.
-
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:
-
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.
Create new ticket
Create your profile
Help contribute to this project by taking a few moments to create your personal profile. Create your profile »
Dan Kubb (dkubb)