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

Index page check collection.limit(1).exists? causes exception when ordering by virtual column #994

Closed
RKushnir opened this issue Feb 9, 2012 · 5 comments
Assignees
Milestone

Comments

@RKushnir
Copy link
Contributor

RKushnir commented Feb 9, 2012

I'm using scope to restrict the records shown to the user:

scope_to :current_user, :association_method => :managed_reservations

The method managed_reservations calls a query

Reservation.joins(:user).select("reservations.*, (users.location_id = #{location_id}) AS local").order('local DESC')

The key problem here is that there's an ordering by a virtual column local. For its internal queries ActiveAdmin replaces select statement with '1' (perhaps to check if any records exist), but doesn't replace the order statement which is then referencing a non-existent column and thus fails. The resulting SQL looks like:

SELECT  1 FROM "reservations" INNER JOIN "users" ... ORDER BY local DESC, "reservations".id desc LIMIT 1 OFFSET 0

So I suggest to replace the order statement in such queries too. For now I'm reverting to 0.3.4 which seems to not having this issue.

@latortuga
Copy link
Contributor

As far as I'm aware, it's not possible to remove chained methods from an AR query without removing everything (e.g. with unscoped). This leaves two options - change your existing :association_method to not order or make a new one without the ordering.

Because you didn't give much detail as to when this is happening, I'll assume that it was on the "show" or "edit" page for a record - it would not make sense in this situation to have an order clause in such a SQL query. If you want the order in place for the index page for your model, I'd advise to set one of your columns as the default sort and to remove it from your association method.

@RKushnir
Copy link
Contributor Author

RKushnir commented Feb 9, 2012

@latortuga This is happening on the index page. I need to have ordering in place because the rows have different priority.

@RKushnir
Copy link
Contributor Author

RKushnir commented Feb 9, 2012

As far as I'm aware, it's not possible to remove chained methods from an AR query without removing everything

@latortuga But somehow it manages to replace the select part of the query. I don't think the order is much different :)

@latortuga
Copy link
Contributor

Alright here's what I found after some digging. You're right that internally they are using a limit(1) to check for existence before rendering the view - you can see it https://github.com/gregbell/active_admin/blob/master/lib/active_admin/views/pages/index.rb#L20

As far as a fix, I am thinking something like this in place of the linked code above:

collection.reorder("#{active_admin_config.resource_class.primary_key} asc").limit(1).exists?

This will overwrite any ordering for the existence check (bypassing the issue where an order clause is using a named column from an overwritten select clause) but won't affect the final output to the screen. Alternatively there's no reason to actually do the wrong query - the correct query could simply be done and check to see if the size of the collection is greater than zero. In fact it's one query instead of two to just evaluate the relation instead of checking for existence followed by evaluating it.

When I said that it's not possible to remove chained AR relation chunks, I merely meant that there isn't a standard way to remove them as far as I know. There are ways to overwrite (using reorder and, more non-obviously, using exists?) but not remove.

@ghost ghost assigned gregbell Feb 14, 2012
@gregbell
Copy link
Contributor

Thanks @RKushnir for reporting this and thanks @latortuga for looking into it!

I've commit a patch that implements @latortuga's suggested fix with one change. Instead of sorting by the primary key, I've passed in a blank order clause, which seems to leave no sort order. This has the same effect without the dependency on the model.

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

No branches or pull requests

3 participants