AtnNn / librethinkdbxx

RethinkDB driver for C++
Other
98 stars 31 forks source link

Invalid JSON number '1434014861.388999939' #5

Closed neonrust closed 9 years ago

neonrust commented 9 years ago

I've been trying to test librethinkdbxx on a bit larger scale (on a customer's data set).

I started simpe with a nonsense query, but got the above exception thrown (R::Error):

table.run(*conn);

I have narrowed it down to a single field in one document (the first one, so there might be more). Dumped by rethinkdb-export it looks like this: "_modified": {"timezone": "+00:00", "$reql_type$": "TIME", "epoch_time": 1434014861.389}}

So I guess that's where the number in the exception comes from, but why is it invalid?

This also lead me to the question: is librethinkdbxx using the JSON protocol towards the server? I saw in the code there are constants for both the binary, V0_4 and JSON, but have not discovered any way to select which protocol to use, so I'm guessing it's negotiated or some such?

neonrust commented 9 years ago

Hm... what about the check in json.cc:

    double number = strtod(buf, &end);
    if (*end != 0) {
        throw Error("Invalid JSON number '%s'", buf);
    }

Perhaps it should actually be:

    double number = strtod(buf, &end);
    if (end == buf && number == 0.0) {
        throw Error("Invalid JSON number '%s'", buf);
    }

According to man strtod:

       If no conversion is performed, zero is returned and the value of nptr is stored in the location ref‐
       erenced by endptr.

However I'm unsure how the case checked for in the current code should be handled... But isn't at least the number valid in that case?

AtnNn commented 9 years ago

I'll try to reproduce this.

AtnNn commented 9 years ago

Yes, this driver uses the JSON protocol to communicate with the server.

I believe the *end != 0 test is correct. strtod sets end to the first character it cannot parse. *end is null iff the whole number was parsed.

I wasn't able to reproduce the error you are seeing. What version of what libc are you using?

neonrust commented 9 years ago

I just realised after posting that strtod is locale-dependent.

And so, I did a small test:

    const char *testnum = "1434014861.388999939";
    char *endp = 0;
    auto val = strtod(testnum, &endp);
    printf("%.4f %d\n", val, endp == testnum + strlen(testnum));

Produces: 1434014861,0000 0

But, with comma as decimal point:

    const char *testnum = "1434014861,388999939";
    char *endp = 0;
    auto val = strtod(testnum, &endp);
    printf("%.4f %d\n", val, endp == testnum + strlen(testnum));

It produces: 1434014861,3890 1 Hm... I'm not sure what function to use for converting a number locale-neutrally...

Regarding JSON, was it a convenience choice or is there no (significant) performance to gain from using the binary protocol?

AtnNn commented 9 years ago

Ah, strtod should not be used here if it is locale dependant.

The other protocol has been deprecated: https://github.com/rethinkdb/rethinkdb/issues/3664

AtnNn commented 9 years ago

@tatsujin1: I pushed a potential fix: b58d2f8

neonrust commented 9 years ago

Yes it works!

Oh, deprecated, I see.