'none' query method to return empty ActiveRecord::Relation #4548

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
6 participants
@xuanxu
Contributor

xuanxu commented Jan 19, 2012

Here is a method to return an empty relation from a AR model (i.e Post.none).
It is chainable and makes any subsequent relation empty, so Post.none.where(:author_id => 1) is also empty.

@ugisozols

This comment has been minimized.

Show comment
Hide comment
@ugisozols

ugisozols Jan 20, 2012

Contributor

Out of curiosity what would be the use case for this?

Contributor

ugisozols commented Jan 20, 2012

Out of curiosity what would be the use case for this?

@xuanxu

This comment has been minimized.

Show comment
Hide comment
@xuanxu

xuanxu Jan 20, 2012

Contributor

Useful for cases where you call a method or a scope that could return no results and need a chainable relation as response. For instance:

User is searching through Posts we want him to search only the posts he have rights to access (stupid example, but just to make the point):

@posts = current_user.visible_posts.where(:name => params[:name])

you want the visible_post method to return a Relation, here is useful the none method:

def visible_posts
  case role
  if 'Country Manager'
    Post.where(:country => country)
  if 'Reviewer'
    Post.published 
  if 'Bad User'
    Post.none #returning [] instead breaks the previous code because it's not chainable 
  end
end
Contributor

xuanxu commented Jan 20, 2012

Useful for cases where you call a method or a scope that could return no results and need a chainable relation as response. For instance:

User is searching through Posts we want him to search only the posts he have rights to access (stupid example, but just to make the point):

@posts = current_user.visible_posts.where(:name => params[:name])

you want the visible_post method to return a Relation, here is useful the none method:

def visible_posts
  case role
  if 'Country Manager'
    Post.where(:country => country)
  if 'Reviewer'
    Post.published 
  if 'Bad User'
    Post.none #returning [] instead breaks the previous code because it's not chainable 
  end
end
@ugisozols

This comment has been minimized.

Show comment
Hide comment
@ugisozols

ugisozols Jan 20, 2012

Contributor

I'm not convinced but thanks for explanation :)

Contributor

ugisozols commented Jan 20, 2012

I'm not convinced but thanks for explanation :)

@carlosantoniodasilva

This comment has been minimized.

Show comment
Hide comment
@carlosantoniodasilva

carlosantoniodasilva Jan 20, 2012

Member

How about using Post.scoped only? It's chainable as well :)

How about using Post.scoped only? It's chainable as well :)

@benmanns

This comment has been minimized.

Show comment
Hide comment
@benmanns

benmanns Jan 20, 2012

Contributor

@carlosantoniodasilva I believe the intention here is to be chainable and return zero results (empty scoped doesn't filter results). However, I agree that this should not be solved with named scopes. And, if necessary, a simple where(0) suffices.

Contributor

benmanns commented Jan 20, 2012

@carlosantoniodasilva I believe the intention here is to be chainable and return zero results (empty scoped doesn't filter results). However, I agree that this should not be solved with named scopes. And, if necessary, a simple where(0) suffices.

@carlosantoniodasilva

This comment has been minimized.

Show comment
Hide comment
@carlosantoniodasilva

carlosantoniodasilva Jan 20, 2012

Member

@benmanns agreed with the intention of the code, but I kinda disagree with the idea though. Giving the example, I'd rather stop building that complex query at the point I know it should return no records - no visible posts, instead of firing the query to the db.

So, I'm +1 on your idea, where(0).

@benmanns agreed with the intention of the code, but I kinda disagree with the idea though. Giving the example, I'd rather stop building that complex query at the point I know it should return no records - no visible posts, instead of firing the query to the db.

So, I'm +1 on your idea, where(0).

@xuanxu

This comment has been minimized.

Show comment
Hide comment
@xuanxu

xuanxu Jan 20, 2012

Contributor

The idea is to have that condition as syntactic sugar in a none method, the opposite of the all method.
BTW where(0) is not valid in all DBs (i.e: ActiveRecord::StatementInvalid: PGError: ERROR in postgresql)

Contributor

xuanxu commented Jan 20, 2012

The idea is to have that condition as syntactic sugar in a none method, the opposite of the all method.
BTW where(0) is not valid in all DBs (i.e: ActiveRecord::StatementInvalid: PGError: ERROR in postgresql)

@andyvb

This comment has been minimized.

Show comment
Hide comment
@andyvb

andyvb Jan 22, 2012

Agreed that where(0) is not portable. where('1 = 0') is better, not sure if there's a shorter where clause. Also, ideally a .none method would not even hit the DB, but instead return an empty array when the relation is executed.

andyvb commented Jan 22, 2012

Agreed that where(0) is not portable. where('1 = 0') is better, not sure if there's a shorter where clause. Also, ideally a .none method would not even hit the DB, but instead return an empty array when the relation is executed.

@iHiD

This comment has been minimized.

Show comment
Hide comment
@iHiD

iHiD Jan 22, 2012

Contributor

If you are in the middle of a complex bit of query building and you hit a situation where you know that no results will be returned, then it makes sense to have a 1=0 or equivalent. Ideally I guess you'd check for all such scenarios before building the query, but that can make the code so much more complex.

I really like the idea of a scoped .none for this situation, that just returns an empty record set. It's cheaper and cleaner than firing the query to the database.

Contributor

iHiD commented Jan 22, 2012

If you are in the middle of a complex bit of query building and you hit a situation where you know that no results will be returned, then it makes sense to have a 1=0 or equivalent. Ideally I guess you'd check for all such scenarios before building the query, but that can make the code so much more complex.

I really like the idea of a scoped .none for this situation, that just returns an empty record set. It's cheaper and cleaner than firing the query to the database.

@xuanxu

This comment has been minimized.

Show comment
Hide comment
@xuanxu

xuanxu Jan 22, 2012

Contributor

@andyvb @iHiD Agreed that it would be ideal for the noneto avoid firing a query to the DB, my first approach was something like this:

    def none
      relation = clone
      @first = @last = @to_sql = @order_clause = @scope_for_create = @arel = nil
      @should_eager_load = @join_dependency = nil
      @records = []
      @loaded = true
      self
    end

That way it just returns a Relation with empty array of records, BUT that way if you chain another call it will return records:

Post.none.where(:author => user) #would return the same as Post.where(:author => user)

and I think that is counterintuitive: if none has been called any subsequent query with more conditions should also retun no records. And to get that you have to make noneadd a false condition to hit the database at the minimum cost and just make a query to return [] as fast as posible, that's what the nonein the pull request does.

Contributor

xuanxu commented Jan 22, 2012

@andyvb @iHiD Agreed that it would be ideal for the noneto avoid firing a query to the DB, my first approach was something like this:

    def none
      relation = clone
      @first = @last = @to_sql = @order_clause = @scope_for_create = @arel = nil
      @should_eager_load = @join_dependency = nil
      @records = []
      @loaded = true
      self
    end

That way it just returns a Relation with empty array of records, BUT that way if you chain another call it will return records:

Post.none.where(:author => user) #would return the same as Post.where(:author => user)

and I think that is counterintuitive: if none has been called any subsequent query with more conditions should also retun no records. And to get that you have to make noneadd a false condition to hit the database at the minimum cost and just make a query to return [] as fast as posible, that's what the nonein the pull request does.

@iHiD

This comment has been minimized.

Show comment
Hide comment
@iHiD

iHiD Jan 22, 2012

Contributor

@xuanxu @andyvb I've added an implementation that skips the database if there is a where(false).

My implementation's at #4607

Thoughts?

Contributor

iHiD commented Jan 22, 2012

@xuanxu @andyvb I've added an implementation that skips the database if there is a where(false).

My implementation's at #4607

Thoughts?

@xuanxu

This comment has been minimized.

Show comment
Hide comment
@xuanxu

xuanxu Jan 23, 2012

Contributor

I'm closing this pull in favor of #4609, where I've implemented the none method skiping the database based on @iHiD idea but not using the where method but instead following the convention of the 'method_value' named attributes.

Contributor

xuanxu commented Jan 23, 2012

I'm closing this pull in favor of #4609, where I've implemented the none method skiping the database based on @iHiD idea but not using the where method but instead following the convention of the 'method_value' named attributes.

@xuanxu xuanxu closed this Jan 23, 2012

@carlosantoniodasilva

This comment has been minimized.

Show comment
Hide comment
@carlosantoniodasilva

carlosantoniodasilva Jan 23, 2012

Member

@xuanxu cool, this new implementation seems a lot cleaner to me, specially avoiding to hit the database in such situations. Nice work.

@xuanxu cool, this new implementation seems a lot cleaner to me, specially avoiding to hit the database in such situations. Nice work.

@andyvb

This comment has been minimized.

Show comment
Hide comment
@andyvb

andyvb Jan 23, 2012

@xuanxu, thumbs up!

andyvb commented Jan 23, 2012

@xuanxu, thumbs up!

@xuanxu xuanxu referenced this pull request Mar 4, 2014

Closed

none query method #4609

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment