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

Ensure #second (and others) acts like #first AR finder #13757

Merged
merged 1 commit into from Jan 20, 2014

Conversation

@terracatta
Copy link
Contributor

@terracatta terracatta commented Jan 18, 2014

This PR addresses issue #13743 where the ordinal ActiveSupport Array extentions were not assuming a default order like first was.

As suggested by @dhh this commit brings the famous ordinal Array instance methods defined
in ActiveSupport into ActiveRecord as fully-fledged finders.

These finders ensure a default ascending order of the table's primary key, and utilize the OFFSET SQL verb to locate the user's desired record. If an offset is defined in the query, calling #second adds
to the offset to get the actual desired record.

Example:

    User.all.second

    # Before
    # => 'SELECT  "users".* FROM "users"'

    # After
    # => SELECT  "users".* FROM "users"   ORDER BY "users"."id" ASC LIMIT 1 OFFSET 1'

    User.offset(3).second

    # Before
    # => 'SELECT "users".* FROM "users"  LIMIT -1 OFFSET 3' # sqlite3 gem
    # => 'SELECT "users".* FROM "users"  OFFSET 3' # pg gem
    # => 'SELECT `users`.* FROM `users`  LIMIT 18446744073709551615 OFFSET 3' # mysql2 gem"'

    # After
    # => SELECT  "users".* FROM "users"   ORDER BY "users"."id" ASC LIMIT 1 OFFSET 4'
@dhh
Copy link
Member

@dhh dhh commented Jan 19, 2014

Looking good to me! 👍

@josevalim
josevalim reviewed Jan 19, 2014
View changes
activerecord/lib/active_record/relation/finder_methods.rb Outdated
def find_first_with_limit(limit)
def find_nth_with_limit(ordinal, limit)
offset = case ordinal
when :first

This comment has been minimized.

@josevalim

josevalim Jan 19, 2014
Contributor

This function should not have knowledge of :first, :second and friends. We should probably just pass the offset_value as argument.

@terracatta
Copy link
Contributor Author

@terracatta terracatta commented Jan 19, 2014

@josevalim Ok, fixed that up for you! Any other concerns?

The famous ordinal Array instance methods defined in ActiveSupport
(`first`, `second`, `third`, `fourth`, and `fifth`) are now available as
full-fledged finders in ActiveRecord. The biggest benefit of this is ordering
of the records returned now defaults to the table's primary key in ascending order.

This comment has been minimized.

@senny

senny Jan 20, 2014
Member

Can you add:

Fixes #13743.

This comment has been minimized.

@terracatta

terracatta Jan 20, 2014
Author Contributor

Added it after the examples! Thanks for the heads up!

@senny
Copy link
Member

@senny senny commented Jan 20, 2014

@terracatta this looks good. Can you squash your commits?

@carlosantoniodasilva
carlosantoniodasilva reviewed Jan 20, 2014
View changes
activerecord/lib/active_record/relation/finder_methods.rb Outdated
if loaded?
@records.first
else
@first ||= find_first_with_limit(1).first
@nth ||= find_nth_with_limit(offset, 1).first

This comment has been minimized.

@carlosantoniodasilva

carlosantoniodasilva Jan 20, 2014
Member

I have a question: how problematic can this cached variable be now that we are using it for different "finds"?

This comment has been minimized.

@rafaelfranca

rafaelfranca Jan 20, 2014
Member

Very problematic. It will cache only the first call.

This comment has been minimized.

@terracatta

terracatta Jan 20, 2014
Author Contributor

Ok so I removed the caching here, but obviously that isn't the right way to go as that will cause multiple queries to fire unnecessarily when calling .first a bunch of times. I think the only option here is cache based on offset being passed into the private method.

Any other suggestions?

This comment has been minimized.

@terracatta

terracatta Jan 20, 2014
Author Contributor

ok I just went with that

@rafaelfranca
rafaelfranca reviewed Jan 20, 2014
View changes
activerecord/lib/active_record/relation/finder_methods.rb Outdated
find_nth(offset_value ? offset_value + 41 : 41)
end

# Same as +forty_second+ but raises <tt>ActiveRecord::RecordNotFound</tt> if no record

This comment has been minimized.

@rafaelfranca

rafaelfranca Jan 20, 2014
Member

it should be forty_two

This comment has been minimized.

@terracatta

terracatta Jan 20, 2014
Author Contributor

Fixed.

@terracatta
Copy link
Contributor Author

@terracatta terracatta commented Jan 20, 2014

@rafaelfranca @carlosantoniodasilva @senny, commits are squashed, updated the changelog, fixed the caching issue by dynamically setting and manually memoizing the instance variable based on the offset passed in. Tested it again on several personal rails projects and the test suite looks good!

Any other feedback?

@rafaelfranca
rafaelfranca reviewed Jan 20, 2014
View changes
activerecord/lib/active_record/relation/finder_methods.rb Outdated
if loaded?
@records.first
else
@first ||= find_first_with_limit(1).first
unless instance_variable_defined?("@offset_#{offset||0}")

This comment has been minimized.

@rafaelfranca

rafaelfranca Jan 20, 2014
Member

It is better to initialize a empty hash in the initialize of Relation and use the offset as the key to memoize it.

Since we are adding an internal variable to cache the value we have to update reset method

This comment has been minimized.

@terracatta

terracatta Jan 20, 2014
Author Contributor

Great suggestion, I'll implement it ASAP

@rafaelfranca
rafaelfranca reviewed Jan 20, 2014
View changes
activerecord/CHANGELOG.md Outdated
User.offset(3).second

# Before
# => 'SELECT "users".* FROM "users"'

This comment has been minimized.

@rafaelfranca

rafaelfranca Jan 20, 2014
Member

This example is wrong.

>> Book.offset(3).second
  Book Load (1.2ms)  SELECT "books".* FROM "books"  LIMIT -1 OFFSET 3

This comment has been minimized.

@terracatta

terracatta Jan 20, 2014
Author Contributor

I can't reproduce this locally on several Rails projects with my code, can you elaborate on how you were able to create this output?

This comment has been minimized.

@rafaelfranca

rafaelfranca Jan 20, 2014
Member

I created a new Rails application using sqlite3. I have not tested with others adapters but this is what I got with sqlite3.

This comment has been minimized.

@terracatta

terracatta Jan 20, 2014
Author Contributor

Hmmm still not able to reproduce it. Generated the rails project from my local rails code, updated the gemfile to point rails to my path, kept sqlite3, did a quick bundle exec rails g model Book title:string, popped open the console, added 6 books with titles, and did Book.offset(3).second and got the following:

  Book Load (0.1ms)  SELECT  "books".* FROM "books"   ORDER BY "books"."id" ASC LIMIT 1 OFFSET 4
 => #<Book id: 5, title: "The Fifth", created_at: "2014-01-20 21:19:06", updated_at: "2014-01-20 21:19:06"> 

Can you create a quick AR executable test case? I'm trying to do the same but I can't get it to fail...

This comment has been minimized.

This comment has been minimized.

@rafaelfranca

rafaelfranca Jan 20, 2014
Member

Just to be clear, I'm talking about the Before example. The after example I had not checked.

@rafaelfranca
rafaelfranca reviewed Jan 20, 2014
View changes
activerecord/lib/active_record/relation/finder_methods.rb Outdated
@@ -126,10 +126,11 @@ def take!
# Person.first # SELECT "people".* FROM "people" ORDER BY "people"."id" ASC LIMIT 1
#
def first(limit = nil)
offset = offset_value ? offset_value : nil

This comment has been minimized.

@rafaelfranca

rafaelfranca Jan 20, 2014
Member

Do we need this?

If offset_value is not set so it will be nil. That said your ternary is doing exactly what the offset_value already represent.

This comment has been minimized.

@terracatta

terracatta Jan 20, 2014
Author Contributor

Nope we don't, let me fix it.

@terracatta
Copy link
Contributor Author

@terracatta terracatta commented Jan 20, 2014

@rafaelfranca, I made the requested changes, but couldn't reproduce your issue or create a failing test case (still trying), can you try again with the latest code and see if you still get a negative limit?

This commit bring the famous ordinal Array instance methods defined
in ActiveSupport into ActiveRecord as fully-fledged finders.

These finders ensure a default ascending order of the table's primary
key, and utilize the OFFSET SQL verb to locate the user's desired
record. If an offset is defined in the query, calling #second adds
to the offset to get the actual desired record.

Fixes #13743.
@terracatta
Copy link
Contributor Author

@terracatta terracatta commented Jan 20, 2014

@rafaelfranca Ok great, added better before examples for pg, sqlite3, and mysql2 gems. I also added some extra test cases for the offset functionality just in case!

Anything else? Sorry for all the back and forth!

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Jan 20, 2014

Looks good. Thank you so much for working on this.

rafaelfranca added a commit that referenced this pull request Jan 20, 2014
Ensure #second (and others) acts like #first AR finder
@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Jan 20, 2014

Merged ❤️

@rafaelfranca rafaelfranca merged commit cafe31a into rails:master Jan 20, 2014
1 check passed
1 check passed
default The Travis CI build passed
Details
@@ -364,19 +444,19 @@ def find_take
end
end

def find_first
def find_nth(offset)
if loaded?
@records.first

This comment has been minimized.

@carlosantoniodasilva

carlosantoniodasilva Jan 21, 2014
Member

This is apparently not going to work with already loaded associations. Just testing around I got two different issues:

# consider the models `Author` has many `Post`s

## 1st scenario - raises exception
>> carlos.posts.reload.first
  Post Load (0.1ms)  SELECT "posts".* FROM "posts"  WHERE "posts"."author_id" = ?  [["author_id", 1]]
=> #<Post id: 1, author_id: 1, title: "ZOMG", body: nil, created_at: "2014-01-21 11:44:22", updated_at: "2014-01-21 11:44:22">
>> carlos.posts.reload.second
  Post Load (0.1ms)  SELECT "posts".* FROM "posts"  WHERE "posts"."author_id" = ?  [["author_id", 1]]
NoMethodError: undefined method 'first' for nil:NilClass
  from....rails/rails/activerecord/lib/active_record/relation/finder_methods.rb:449:in 'find_nth'


## 2nd scenario - returns the same record
>> carlos.posts.first
=> #<Post id: 1, author_id: 1, title: "ZOMG", body: nil, created_at: "2014-01-21 11:44:22", updated_at: "2014-01-21 11:44:22">
>> carlos.posts.second
=> #<Post id: 1, author_id: 1, title: "ZOMG", body: nil, created_at: "2014-01-21 11:44:22", updated_at: "2014-01-21 11:44:22">
>> carlos.posts.third
=> #<Post id: 1, author_id: 1, title: "ZOMG", body: nil, created_at: "2014-01-21 11:44:22", updated_at: "2014-01-21 11:44:22">

@terracatta can you take a look please? It'd be good to provide some tests for already loaded associations with some of these methods. Thanks.

This comment has been minimized.

@terracatta

terracatta Jan 21, 2014
Author Contributor

Absolutely! Thanks for testing!

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

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.