Lighthouse has a new layout. Prefer the old one? Return to the old layout, and switch back any time from the link at the top of each page.

This project is archived and is in readonly mode.

The type_cast_calculated_value method will trust DB types before casting Float/String to a BigDecimal

#6365

Some databases return integer values for AVG. Allow that value to pass through vs raising an Fixnum#to_d no method error.

Reported by Ken Collins · February 3rd, 2011 @ 02:59 AM

State: committed
Milestone: 3.1
Assigned to: Santiago Pastorino Santiago Pastorino
Importance: Low

Activity

  1. Santiago Pastorino
    Santiago Pastorino
    • Importance changed from to Low

    hey Ken, can you provide more info about which databases return integer values for AVG?.
    Also your patch have no test and you could do ...

    value.respond_to? :to_d ? value.to_d : value

    February 3rd, 2011 @ 05:30 PM

  2. Ken Collins
    Ken Collins

    Specifically SQL Server. Details on return type here.
    http://msdn.microsoft.com/en-us/library/ms177677.aspx

    Re the code style, I do like your line better and tend to do that more often, but working so much in ARel lately, I started using the Constant===. Maybe for speed? I dunno. I'm fine either way and actually like the concise respond_to? better.

    Re the tests, I have done short patches like these before since a test would have been deep implementation mock. I can easily do one for regression purposes and would prefer on if that is what you prefer too?

    February 3rd, 2011 @ 06:58 PM

  3. Santiago Pastorino
    Santiago Pastorino
    • State changed from new to open
    • Milestone set to 3.1
    • Assigned user set to Santiago Pastorino

    Ok please provide a test case and use the respond_to? and I will push the patch ;).
    Thanks.

    February 3rd, 2011 @ 07:25 PM

  4. Ken Collins
    Ken Collins

    Trying this right now. Just got to figure out the right stub point. For instance.

    def test_should_return_integer_average_if_db_returns_such
      Account.connection.stubs :execute => [{'avg_id' => 3}]
      value = Account.average(:id)
      assert_equal 3, value
    end
    

    This works with sqlite3 but not mysql. Will come up with something shortly.

    February 3rd, 2011 @ 07:49 PM

  5. Ken Collins
    Ken Collins

    OK, uploaded a new patch. Test added, used the respond_to? method. All tests passed in mysql/postgresql/sqlite.

    Also, will this make it into the v3.0.4 release?

    February 3rd, 2011 @ 08:15 PM

  6. Santiago Pastorino
    Santiago Pastorino

    Ken please avoid stubs you can find lot of examples in ActiveRecord tests about how things are tested.
    Also you have removed the try, are you sure that this is right? what happens if value is nil?.
    Thanks.

    February 3rd, 2011 @ 09:03 PM

  7. Ken Collins
    Ken Collins

    I had a test that directly tested the method but that it would have been discouraged. It looked something like this.

    expected_value = 3
    column = Account.columns_hash['id']
    returned_value = Account.send :type_cast_calculated_value, expected_value, column, 'average'
    assert_equal expected_value, returned_value
    

    Re the nil. To my knowledge there is no NilClass#to_d method. The same thing would happen as before. And lastly, nil would be an expected result from a simple function where there is 0 rows.

    How should I proceed?

    February 3rd, 2011 @ 09:12 PM

  8. Ken Collins
    Ken Collins

    FYI, this test is below mine and is even visible in the patch. It outlines the nil question of yours.

    def test_should_return_nil_as_average
      assert_nil NumericData.average(:bank_balance)
    end
    

    I am still working on coming up with a test style that is acceptable. Typically I feel dirty doing these types of test due to their coupling to the low level internals. They are tricky to get right and any feedback would be appreciated. So no database in the blessed 3 are going to return an integer, the stub seemed appropriate and allowed the test to stay high level. Testing the private method directly seems wrong too. Thoughts?

    February 3rd, 2011 @ 09:35 PM

  9. Repository
    Repository
    • State changed from open to committed

    (from [95d5d9b6c48c08f1fba0c77ecbc97b62b2603824]) The type_cast_calculated_value method will trust DB types before casting to a BigDecimal.

    [#6365 The type_cast_calculated_value method will trust DB types before casting Floa... state:committed]

    Signed-off-by: Santiago Pastorino santiago@wyeworks.com
    https://github.com/rails/rails/commit/95d5d9b6c48c08f1fba0c77ecbc97...

    February 3rd, 2011 @ 10:03 PM

  10. Repository