NotSupportedException when retreiving doc with DateTime.MinValue

[ThomasHoest]

Hi

I get the above exception when trying to pull out data. Looking through the code i can see the it originates in the UnsignedLongDatumConverter. I'm puzzled by this implementation:

if (valueAsLong >= ulong.MaxValue || valueAsLong <= ulong.MinValue)
                    {
                        throw new NotSupportedException("Attempted to cast Datum to non-nullable unsigned long, but Datum outside range of valid unsigned long");
                    }

It will throw on 0 which happens in the case of DateTime.MinValue. Why <= and not just <? It seems only the long converters use this approach?

Thanks Thomas

[mfenniak]

Yup, that seems totally like a bug. The whole range check in long & ulong doesn't make sense... it's casting the value to a ulong, and then attempting to check if it's outside the value range of a ulong; which, after casting, it can never be. It would make sense if it were checking the double value from datum.r_num, and only doing a greater-than or less-than check (not equals).

I don't have time right now to fix this, but I completely agree with your assessment. If you were able to submit a pull request to adjust this code and correct any related unit tests, I'd love to accept it. If not, I'll fix this up when I have some free time.

[ThomasHoest]

Hi Mathieu

Thanks for your reply. I think I will have time to submit a pull request in the coming weekend. I'll keep you posted.

Thanks Thomas

On Mon, Nov 16, 2015, 04:44 Mathieu Fenniak notifications@github.com wrote:

Yup, that seems totally like a bug. The whole range check in long & ulong doesn't make sense... it's casting the value to a ulong, and then attempting to check if it's outside the value range of a ulong; which, after casting, it can never be. It would make sense if it were checking the double value from datum.r_num, and only doing a greater-than or less-than check (not equals).

I don't have time right now to fix this, but I completely agree with your assessment. If you were able to submit a pull request to adjust this code and correct any related unit tests, I'd love to accept it. If not, I'll fix this up when I have some free time.

— Reply to this email directly or view it on GitHub https://github.com/mfenniak/rethinkdb-net/issues/240#issuecomment-156907833 .

[ThomasHoest]

I have made the changes, could you add me so i can push the branch and create the pull request?

I had to modify the tests a bit since double looses precision above 16 digits and behaved oddly. Take a look when it is pushed.

[mfenniak]

Hey Thomas, great! Please create a fork of the repo on GitHub, push your branch there, and then create the pull request from that repo & branch into this repo & master. I'll review it right away.

[ThomasHoest]

Done

Just a comment on the tests

1.0 + long.MaxValue result in 9.2233720368547758E+18
long.MaxValue is 9223372036854775807

This failed the tests. I had to take this "rounding" into account and add a bigger number. Perhaps you have better way to get around this convert to double precision issue.

[mfenniak]

Fixed by #241.