Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feeding eq_any an empty list (eventually) breaks. #368

Closed
cqfd opened this issue Jun 11, 2015 · 9 comments
Closed

Feeding eq_any an empty list (eventually) breaks. #368

cqfd opened this issue Jun 11, 2015 · 9 comments

Comments

@cqfd
Copy link

cqfd commented Jun 11, 2015

Feeding eq_any an empty list doesn't behave very well: you get back an Arel::Nodes::Grouping whose @expr is nil. Calling to_sql will then throw a RuntimeError: unsupported: NilClass exception.

It would be great if Arel could return an empty/impossible relation here. If that's not possible, I would much rather have eq_any throw right away rather than waiting for to_sql to blow up.

@danielpclark
Copy link

BUMP

eq_any works in Arel 5 and accepts empty collections and likewise works in returning empty collections. As of Arel 6 this no longer works. Both:

eq_any(nil)
eq_any([])

Raise:

RuntimeError: unsupported: NilClass

@sgrif
Copy link

sgrif commented Oct 4, 2015

Thanks but I don't think we need to support this use case. As you mentioned, it changed in a major version bump.

@sgrif sgrif closed this as completed Oct 4, 2015
@danielpclark
Copy link

This is an important use case. I use it in geo_location lookup with addresses.

events = Event.where(
  Event.arel_table[:eventable_type].eq("User").
    and(Event.arel_table[:eventable_id].eq(current_user.id).or(
      Event.open.where_values.first.and(
        Event.arel_table[:id].eq_any(
          Address.where(addressable_type: "Event").
          where.not(addressable_id: current_user.events.ids).
          near(
            current_user.current_addresses.first.try(&:geo_locale),
            300
          ).select(&:addressable_id)
        )
      )
    )
  )
)

This query pulls up events that the user has created and any near the user. By not having eq_any support empty results, and having it raise an error, you break standard behavior that ActiveRecord and other Arel methods still conform to. I don't see why it's important to have a breaking change like this when expecting empty results is a given in any situation.

@sgrif
Copy link

sgrif commented Oct 4, 2015

The use of Arel from Active Record is not supported. It is an internal, private API.

@danielpclark
Copy link

So the wonderfully made conference talks on using Arel for ActiveRecord queries were misinformed?

@sgrif
Copy link

sgrif commented Oct 4, 2015

Or maybe just missing the caveat that like any private API, it may break
between versions.

On Sun, Oct 4, 2015, 8:04 AM Daniel P. Clark notifications@github.com
wrote:

So the wonderfully made conference talks on using Arel for ActiveRecord
queries were misinformed?


Reply to this email directly or view it on GitHub
#368 (comment).

@jfeldstein
Copy link

Love the fact that in open source you can just fix the issue. Boom.

Thanks for making the sane choice: eq_any an empty set == no results, not a crash.

@danielpclark
Copy link

@jfeldstein The patch was only given for the test suite in Arel 5 for the Rails 4.1 series. After Rails 4.1 it will crash on an empty set. If you want to avoid this issue with later versions of Rails you can write your own method:

module ::Arel
  module Predications
    def eql_any others
      if others.length.zero?
        Nodes::SqlLiteral.new "(NULL)"
      else
        grouping_any :eq, others
      end
    end
  end
end

@jfeldstein
Copy link

Much obliged.

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

No branches or pull requests

4 participants