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

Relation#last has issues with existing order_clauses #1272

Closed
ericallam opened this Issue May 24, 2011 · 4 comments

Comments

Projects
None yet
4 participants
@ericallam
Copy link
Contributor

ericallam commented May 24, 2011

On rc1, if a model has a default_scope with an order clause, #last does not generate the correct sql query, resulting in the same object as #first being returned.

For example, lets say I have a User model with a default_scope with created_at ASC:

class User < ActiveRecord::Base
  def self.default_scope
     order('created_at ASC')
  end
end

Then User.last generates this sql:

SELECT "users".* FROM "users" ORDER BY created_at ASC, created_at DESC LIMIT 1

So:

>> User.first == User.last
  User Load (3.1ms)  SELECT "users".* FROM "users" ORDER BY created_at ASC LIMIT 1
  User Load (1.1ms)  SELECT "users".* FROM "users" ORDER BY created_at ASC, created_at DESC LIMIT 1
=> true

It looks like it only happens with order clauses in the default_scope. (also effects #last in a relationship call e.g. project.users.last)

@pixeltrix

This comment has been minimized.

Copy link
Member

pixeltrix commented May 24, 2011

This is to do with how the default scope is merged in last now in 3.1 - AR::Relation#last calls reverse_order internally which it does on the order in the default scope but arel merges in the default scope again so you end up with two order clauses.

@jonleighton would we be better off merging in the default scope in the finder methods like find_first, find_last, etc. rather than merging in the arel method? Using with_default_scope.reverse_order in AR::Relation#last seems to fix this.

@jonleighton

This comment has been minimized.

Copy link
Member

jonleighton commented May 24, 2011

@pixeltrix We need to change reverse_order to just set a reverse_order_value flag, and then deal with it properly in build_arel... I had to do the same thing for reorder when I originally made the default scope change, but I missed this one.

I will fix when I get a chance - or feel free to take it in the mean time :)

@ghost ghost assigned jonleighton May 24, 2011

jonleighton added a commit that referenced this issue Jun 1, 2011

Fix issue #1272
Set reverse_order_value when asked to reverse_order().
Do the actual reversal in build_arel.

jonleighton added a commit that referenced this issue Jun 1, 2011

Fix issue #1272
Set reverse_order_value when asked to reverse_order().
Do the actual reversal in build_arel.

@jonleighton jonleighton closed this Jun 1, 2011

@oriolgual

This comment has been minimized.

Copy link
Contributor

oriolgual commented Jun 8, 2011

This has introduced a new problem:

>> Fuu.order(Fuu.arel_table[:name].desc).reverse_order.to_sql
=> "SELECT \"fuus\".* FROM \"fuus\"  ORDER BY #<Arel::Nodes::Ordering:0x0000010319dc88> DESC"

Versus:

>> Fuu.order(Fuu.arel_table[:name].desc.to_sql).reverse_order.to_sql
=> "SELECT \"fuus\".* FROM \"fuus\"  ORDER BY \"fuus\".\"name\" ASC"

Notice the to_sql call in Fuu.arel_table[:name].desc

@jonleighton

This comment has been minimized.

Copy link
Member

jonleighton commented Jun 8, 2011

I have filed a new issue for this: #1571

jake3030 pushed a commit to jake3030/rails that referenced this issue Jun 28, 2011

Ruby 1.9 compat: fix for SSL in Active Resource
[rails#1272 state:committed]

Signed-off-by: Jeremy Kemper <jeremy@bitsweat.net>

boazsegev added a commit to boazsegev/rails that referenced this issue Jul 5, 2018

Add support for the new Rack specification
This adds native WebSocket support to ActiveCable, based on the [Rack PR rails#1272](rack/rack#1272).

Tested using iodine version 0.6.4.

IMHO, this is the best **compatible** approach, that still accommodates `hijack` support.

This approach should improve server-client performance. However, channel and broadcasting performance is likely to experience the same bottlenecks that aren't addressed by this change.

Please note:

This approach moves the responsibility for the WebSocket and network logic to the Server, freeing the application and framework from protocol and network concerns.

IMHO, it would make sense if the road-map were updated so `hijack` support would be dropped after the default server (currently Puma) implements the new Rack specification.

This would allow Rails to reduce it's dependencies and remove the `websocket-driver` and the `nio4r` gems from the dependency list.

This would also improve the separation of concerns, where the framework could be freed from IO or protocol related concerns.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment