afair / postgresql_cursor

ActiveRecord PostgreSQL Adapter extension for using a cursor to return a large result set
MIT License
612 stars 47 forks source link

Add naive support for includes/preload for each_instance #34

Closed zapo closed 6 years ago

zapo commented 7 years ago

This needs some tests but it seems like it would fix #25. It's not as efficient as each_instance was as it needs to loop through a slice twice. This probably would be better hooked earlier at a each_block level or something like that.

afair commented 6 years ago

This didn't check out, so I had to revert it. Perhaps the ActiveRecord implementation changed. Do you want to look into it?

TestPostgresqlCursor#test_activerecord: NoMethodError: undefined method preload_values' for Product(id: integer, data: string):Class /Users/allen/.rvm/gems/ruby-2.5.1/gems/activerecord-5.2.0/lib/active_record/dynamic_matchers.rb:22:inmethod_missing' /Users/allen/src/Ruby/postgresql_cursor/lib/postgresql_cursor/cursor.rb:123:in each_instance' /Users/allen/src/Ruby/postgresql_cursor/lib/postgresql_cursor/active_record/sql_cursor.rb:73:ineach_instance_by_sql' test/test_postgresql_cursor.rb:97:in `test_activerecord'

zapo commented 6 years ago

@afair I think we need to call it on an ActiveRecord::Relation instead (sounds like it no longer available on class directly). I can reopen this PR with a fix.

afair commented 6 years ago

This is the problem with shadowing a private API; this library isn't based on the sanctioned API. I'm also afraid of when the next major Rails upgrades hits, we'll -- That is I -- will have to support these changes. If you don't need it, or can write your app code based on existing methods, then the whole library won't be as brittle for everyone else. I have been chasing the basic feature set of Rails (I started this 10 years ago, and have seen lots of churn), but don't want to take on additional risk that isn't always needed. Thoughts?

zapo commented 6 years ago

That makes much sense, you are right, trying to support all ActiveRecord::Relation features, for those rails versions in postgresql_cursor forces you to use rails private APIs and is a guarantee of endless maintenance headache. Support for preload can definitely be done at app level instead :+1: