AR find conditions proposal

rubyonrails-core
1

I am currently implementing this functionality in a monkey patch, and I'm wondering if there is sufficient interest in it to turn it into a real Rails patch with appropriate tests.

Right now, Foo.find(:all, :conditions => { :bar => baz }) will generate and execute SQL with conditions for the `bar` field equal to the baz variable. If baz is a Range object it will use BETWEEN instead of =. If baz is nil it will use IS NULL. If baz is an Array (or AssociationCollection) it will use IN.

The problem is that there is no way to express NOT IN, IS NOT NULL, NOT BETWEEN, <>, <, >, <=, or >=. I've implemented a way of expressing it. Here are some examples:

  baz = { :operator => '<>', :value => 'quux' }   SQL: "<> 'quux'"

  baz = { :not => true, :value => nil }   SQL: "IS NOT NULL"

  baz = { :not => true, :value => 10..20 }   SQL: "NOT BETWEEN 10 AND 20"

  baz = { :not => true, :operator => '>', :value => 20 }   SQL: "<= 20"

  baz = { :not => true, :value => %w(foo bar baz quux) }   SQL: "NOT IN ('foo', 'bar', 'baz', 'quux')"

As a monkey patch, my implementation is roughly 75 lines long. I suspect it would be smaller as a patch to ActiveRecord::Base (plus tests). Is there sufficient interest that I should put it in Trac as a feature request with a patch for implementation and tests?

--Greg

2

Have you ever heard of ar extensions? At least some of that, if not all, is there. Plus more (such as RegExp).

http://www.continuousthinking.com/tags/arext

Bernardo

3

How is this better than writing:

:conditions => [“baz <= ?”, 20]

In my opinion, core AR shouldn’t be out trying to implement all of SQL operators and functions in Ruby. That’s why we have projects like Ambition.

4

This approach does not embody the Rails value of beautiful code. Take a look at ez-where, ar-extensions, and Ambition for better approaches to doing more SQL in ActiveRecord via Ruby syntax. Also, DataMapper has a pretty lightweight syntax that adds methods to Symbol for SQL operations (e.g. ":foo.not => nil".

This issue has been discussed literally for years. If you want to play in an open source project like Rails, it's best to do your homework and check out the other approaches people are using before jumping in with both feet. That way your work is more likely to be received as a useful contribution. Not wanting to squelch your energy and enthusiasm, just trying to offer some constructive criticism.

5

Have you ever heard of ar extensions? At least some of that, if not all, is there. Plus more (such as RegExp).

http://www.continuousthinking.com/tags/arext

Ah, that looks very helpful. I'm a little uncomfortable with carrying the comparison operator semantics in a suffix to the field name, and I don't see support for IS NOT NULL, but it may be all I need for what I'm doing right now. More about that in a response to one of the *ahem* less helpful responses.

Bernardo

--Greg

6

[...]

> The problem is that there is no way to express NOT IN, IS NOT NULL, > NOT BETWEEN, <>, <, >, <=, or >=. I've implemented a way of expressing > it.

[...]

> Is there sufficient interest that I should put it in Trac as a feature > request with a patch for implementation and tests? > > --Greg

This approach does not embody the Rails value of beautiful code.

That's an interesting response. I thought it fit in well with the existing AR conditions hash syntax. Perhaps you have suggestions on how to beautify it?

Take a look at ez-where, ar-extensions, and Ambition for better approaches to doing more SQL in ActiveRecord via Ruby syntax. Also, DataMapper has a pretty lightweight syntax that adds methods to Symbol for SQL operations (e.g. ":foo.not => nil".

I have a good reason for trying to do it within AR. The ar-extensions stuff looks promising for that, though its use of field name suffixes to denote operators puts me off. Ambition, ez-where, and DataMapper all fail for my purposes. I am implementation a reporting engine (the querying backend, including some support for user-built queries) that uses and understands AR associations. To manage that I need to be pretty tightly integrated with AR, which is why I'm monkey patching. I'm going to look into ar-extensions, but odds are good that I'll be sticking with my monkey patch.

This issue has been discussed literally for years. If you want to
play in an open source project like Rails, it's best to do your
homework and check out the other approaches people are using before
jumping in with both feet. That way your work is more likely to be
received as a useful contribution. Not wanting to squelch your energy
and enthusiasm, just trying to offer some constructive criticism.

Yeah, this is just unnecessary. If you read my post, it wasn't a demand for functionality but an offer of code I had written for my own purposes. Saying that a policy decision has been made that providing broader support for SQL comparators in AR conditions is out of scope is one thing. Saying that I should do my homework because I offered some code you don't happen to be interested in is another.

Josh Susser

--Greg

7

This approach does not embody the Rails value of beautiful code.

That's an interesting response. I thought it fit in well with the existing AR conditions hash syntax. Perhaps you have suggestions on how to beautify it?

Look down at the next few lines, which you responded to even. Those were in fact my suggestions. I admit I have a bias against really complicated hash structures as a substitute for what I consider a good OO API, but it's not just me. I asked a few other experienced Rails folks just now and no one thought your syntax was acceptable.

Take a look at ez-where, ar-extensions, and Ambition for better approaches to doing more SQL in ActiveRecord via Ruby syntax. Also, DataMapper has a pretty lightweight syntax that adds methods to Symbol for SQL operations (e.g. ":foo.not => nil".

I have a good reason for trying to do it within AR. The ar-extensions stuff looks promising for that, though its use of field name suffixes to denote operators puts me off. Ambition, ez-where, and DataMapper all fail for my purposes. I am implementation a reporting engine (the querying backend, including some support for user-built queries) that uses and understands AR associations. To manage that I need to be pretty tightly integrated with AR, which is why I'm monkey patching. I'm going to look into ar-extensions, but odds are good that I'll be sticking with my monkey patch.

I wasn't suggesting you use those pieces of code, but look to them as examples of better ways to express more complicated SQL queries in Ruby syntax.

This issue has been discussed literally for years. If you want to play in an open source project like Rails, it's best to do your homework and check out the other approaches people are using before jumping in with both feet. That way your work is more likely to be received as a useful contribution. Not wanting to squelch your energy and enthusiasm, just trying to offer some constructive criticism.

Yeah, this is just unnecessary. If you read my post, it wasn't a demand for functionality but an offer of code I had written for my own purposes. Saying that a policy decision has been made that providing broader support for SQL comparators in AR conditions is out of scope is one thing. Saying that I should do my homework because I offered some code you don't happen to be interested in is another.

Offering some code doesn't mean that you get to bypass the usual process of people responding to your proposed contribution and evaluating it on its value and fit with the rest of Rails. I do actually think the functionality you are proposing could be useful, but I dislike the syntax and think it's a poor fit with Rails style and values. And I didn't say you needed to do your homework because I wasn't interested in your code; I said that because this topic has been addressed many times before, and there are existing solutions that kick your solution's ass. But if you can't cope with people disagreeing with your opinion of the merits of your own code, you should get out of the open source game right away.

I hope you'll take my comments in the spirit they are intended. All of us in this community make proposals that get shot down for various reasons, and not everyone disagrees with everyone else on everything. But when people take the time to respond to you and offer well-intentioned constructive criticism, it's usually good to at least be civil in response, no matter what you think of their comments.

8

Er...really? I mean really? I mean, I'm not going to make excuses here, DataMapper wasn't a good fit for you, fine. But you're building a reporting solution and for some reason the library with the thinnest of SQL abstractions (AR, not an insult, it just is what it is) is the only one that will work for you?

I mean, if I had need for a reporting solution _today_, I would use Sequel. It's got a relatively clean set of abstractions over SQL and maps fairly cleanly to reporting efforts from what I've seen.

Enough of that though, do what you're motivated about. :slight_smile:

Just to comment on your suggestion though, I personally think it's a terrible abuse of Hash. Please don't take it personally. :wink: If you're not happy with the DM syntax, I'd suggest a set of constant-style- methods might be a good alternative. ie:

# class Operator # attr_reader :type, :target # def new(type, target) # @type, @target = type, target # end