This project is archived and is in readonly mode.

#6365 ✓committed
Ken Collins

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

Reported by Ken Collins | February 3rd, 2011 @ 02:59 AM | in 3.1

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

Comments and changes to this ticket

  • Santiago Pastorino

    Santiago Pastorino February 3rd, 2011 @ 05:30 PM

    • 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

  • Ken Collins

    Ken Collins February 3rd, 2011 @ 06:58 PM

    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?

  • Santiago Pastorino

    Santiago Pastorino February 3rd, 2011 @ 07:25 PM

    • 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.

  • Ken Collins

    Ken Collins February 3rd, 2011 @ 07:49 PM

    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.

  • Ken Collins

    Ken Collins February 3rd, 2011 @ 08:15 PM

    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?

  • Santiago Pastorino

    Santiago Pastorino February 3rd, 2011 @ 09:03 PM

    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.

  • Ken Collins

    Ken Collins February 3rd, 2011 @ 09:12 PM

    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?

  • Ken Collins

    Ken Collins February 3rd, 2011 @ 09:35 PM

    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?

  • Repository

    Repository February 3rd, 2011 @ 10:03 PM

    • State changed from “open” to “committed”

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

    [#6365 state:committed]

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

  • Repository

    Repository February 3rd, 2011 @ 10:16 PM

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

    [#6365 state:committed]

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

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

Attachments