Permalink
Browse files

Make the tests break again

We need to fix this test
  • Loading branch information...
1 parent 152edcc commit 4a2650836680f51490e999c3c8441a2f9adff96e @rafaelfranca rafaelfranca committed Dec 2, 2013
Showing with 0 additions and 2 deletions.
  1. +0 โˆ’2 activerecord/test/cases/finder_test.rb
@@ -837,8 +837,6 @@ def test_find_with_nil_inside_set_passed_for_attribute
end
def test_with_limiting_with_custom_select
- skip 'broken test' if current_adapter?(:MysqlAdapter)
-
posts = Post.references(:authors).merge(
:includes => :author, :select => ' posts.*, authors.id as "author_id"',
:limit => 3, :order => 'posts.id'

7 comments on commit 4a26508

Member

vipulnsward replied Dec 8, 2013

@rafaelfranca
Yesterday I tried investigating why this test breaks. The query generated, here is

SELECT   posts.*, authors.id as \"author_id\", `posts`.`id` AS t0_r0, `posts`.`author_id` AS t0_r1, `posts`.`title` AS t0_r2, `posts`.`body` AS t0_r3, `posts`.`type` AS t0_r4, `posts`.`comments_count` AS t0_r5, `posts`.`taggings_count` AS t0_r6, `posts`.`taggings_with_delete_all_count` AS t0_r7, `posts`.`taggings_with_destroy_count` AS t0_r8, `posts`.`tags_count` AS t0_r9, `posts`.`tags_with_destroy_count` AS t0_r10, `posts`.`tags_with_nullify_count` AS t0_r11, `authors`.`id` AS t1_r0, `authors`.`name` AS t1_r1, `authors`.`author_address_id` AS t1_r2, `authors`.`author_address_extra_id` AS t1_r3, `authors`.`organization_id` AS t1_r4, `authors`.`owned_essay_id` AS t1_r5 FROM `posts` LEFT OUTER JOIN `authors` ON `authors`.`id` = `posts`.`author_id`  ORDER BY posts.id LIMIT 3

The Post object already has author_id attribute, for defining belongs_to. The select AS author_id, pollutes this, as we already have author_id as an attribute on our model. I don't see why someone, would do such a select AS which is conflicting with already existing attribute.

Not sure if such a behaviour should be supported. If yes, fixes need to be done to Result.column_types, which is a hash, and can contain only single attribute name at a time.

If this is not desired, I'l send a PR to change the SELECT AS

Owner

rafaelfranca replied Dec 8, 2013

I think is fine to change this, but one question that is not answered yet is why it only fails with mysql driver.

Member

vipulnsward replied Dec 8, 2013

Mysql results are handled with https://github.com/rails/rails/blob/master/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb#L413-L437

The result from conn.query is parsed and stored into a hash types, and Result is created by

result_set = ActiveRecord::Result.new(types.keys, result.to_a, types)

Since, the mapping in this hash is field name against value, the type stored for first author_id is overridden by second one, thus creating 1 less type-mapping, where as the arity of result values is maintained since we pass result.to_a(all values).

For retrieving back values, indices are used, on rows/cols form Result. The missing single key, is breaking the test, and assigning, type value Such a lovely day instead of Post

Sending PR, to change this.

Member

vipulnsward replied Dec 9, 2013

PostgreSQL is using

ret = ActiveRecord::Result.new(fields, result.values, types)

from https://github.com/rails/rails/blob/master/activerecord/lib/active_record/connection_adapters/postgresql/database_statements.rb#L151

to generate the result. Notice the fields here vs types.keys in mysql example.
The arity is properly maintained due to this, ie. nos. of result rows returned vs result cols.

Member

vipulnsward replied Dec 9, 2013

I hope, I am making sense, or we could chat on IM about it. ๐Ÿ˜„

Owner

rafaelfranca replied Dec 9, 2013

Yes, you are making sense, but the code is not ๐Ÿ˜„

Please sign in to comment.