-
Notifications
You must be signed in to change notification settings - Fork 820
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a decoding flag to get all numbers as strings #10
Comments
Bump. I've gotten a bug report in my program (jshon) from this one. For example
Neither of these outcomes are good. |
So you'd like to decode the value as a string? Or is the imprecise decimal representation the problem? |
I would like to skip decoding altogether and access the original string. The imprecise decimal notation is only a symptom of why it is needed. Jshon does not do any math - it is only supposed to extract and spit out chunks of json. (Arguably one could attach a precision field to the json_t struct and dig this value out when it comes time to printf and generate a printf format string on the fly. But that seems silly.) |
Ok. But this decoding flag would give you a string, i.e. "200.123456789", not a special value that's a number but whose contained value is a string. So you would lose the information that the value in JSON input was a number. |
That seems not good. And breaks a chunk of the json spec, since the type information would be screwed up. Two ideas of the top of my head... Two more int fields in the structs. These int fields would contain the index and length of substring from the original input that the decoded value was derived from. Probably accessed by a json_get_source() function? Hacky, but less hacky than denying that numbers exist at all. A new non-numerical number type. It would be neither int nor float, just javascript-style "number". Internally represented as a string, up to the user to decode however they want. This also makes the people who want a single JS-style numerical type happy. Probably enabled by a parse flag? |
Adding a precision field to the |
Darn. I was hoping for a cleaner fix, something that did not involve throwing more patches onto a string -> number -> string conversion, by way of using sprintf to dynamically generate format string for printf. Yuck. To that end, I've got half of the code put together for a new SNUMBER type at https://github.com/keenerd/jansson Only the code for basic loading and dumping has been written. It is missing the almost all the helper functions (setting, deleting, etc) and is a little kludgy because the parse flags are not available in the lexing stage (can't have a TOKEN_SNUMBER). It does build, but I have not been able to test it properly because cmake refuses to build shared .so libraries. Figuring out how to make cmake build these was more complicated that writing the prototype SNUMBER code. |
Introducing a new type for this use case doesn't sound so good. The ultimately correct fix would be to replace |
For an example of how another library does this: yajl-tree simply stores the original string and lets you access it directly: https://github.com/lloyd/yajl/blob/master/src/api/yajl_tree.h#L81 |
Yeah, this is also an option. But allocating the extra memory to store input strings of every number doesn't sound good for people using this on embedded devices. It could of course be enabled by a decoding flag to make it optional. Would if be beneficial to do this also for strings? They have many possible input forms because any Unicode code point can be escaped with |
Because of the decimal precision issue we made a change to our copy of Jansson to allow all numbers to be treated as strings. This might not be efficient for many use cases but for ours it's actually more efficient. In brief:
If there's interest in adding this to Jansson let me know and I'll create a pull request. |
@dbelliveau If it's still possible to submit your patches as PR, then it might help with #425 (my guess is that it's taking similar approach but it comes from a different source). |
Yes I can do that, but I probably won't get to it until the weekend. |
@akheron any reason you are against creating a new |
For us new type would work as well |
I'm not against adding a new type if it's not only for working around quirks of floating point formatting. What are the use cases that we're trying to support here? I think I've lost track :) Supporting numbers that don't have a native representation ("bignums") come to my mind, but are there other use cases? |
In addition to dealing with numbers that cannot be reliably round-trip parsed then dumped I could see JSON_RAW being useful for certain resource constrained situations / performance critical code paths. A few ways I could see it used:
{
"jsonrpc": "2.0",
"id": "123",
"method": "request-from-client",
"params": {"dest": "server2", "payload": {"key": "value"}}
} I can't directly post a good example JSON so assume that the I think more thought is required to decide how to control json_load recursion. Maybe we could have a few flags like JSON_DECODE_RECURSIVE_0 or JSON_DECODE_RECURSIVE_1 that would serve most needs in a simple way. Another way might be an interface like json_pack / json_unpack where a format string would control how contents are parsed, or maybe even a callback based interface. In any case I think this would be a follow-up, I'm just making the case that JSON_RAW would have additional uses beyond handling numbers that cannot be represented natively. |
Alright, sounds good. Having a JSON_RAW value, how does the user know what it contains (number/string/etc)? |
I was thinking that JSON_RAW would use json_string_t in the background and thus would not store further information about the contents. In theory we could provide a function
We would have json_raw support functions matching the json_string functions, the create/set functions would function slightly differently than json_string functions. Create/set would strip white-space at the start / end but not within.
The following code would print json_t *raw = json_raw(" \n\t[ 1, 2\n\t ] \n\t");
printf("strcmp: %d\n", strcmp(json_raw_value(raw), "[ 1, 2\n\t ]")); |
Would it be useful to have an option to not store anything at all in JSON_RAW? That would make the JSON structure non-encodable, but on the other hand it would save memory if only a part of the input is interesting for the user. |
I hadn't thought of that, I'm not sure. My idea was that JSON_RAW would be used where the value is not locally interesting, but still needs to be saved to disk or forwarded to clients / other servers for processing. |
Well, I guess it can be added later if needed. |
Related to issue akheron#10
@akheron I've done some work on an actual implementation of the JSON_RAW type, I think that the correct way to determine what a I've posted #446 with the first attempt implementation of JSON_RAW. |
Related to issue akheron#10
Related to issue akheron#10
Related to issue akheron#10
Is there any progress on this issue? @coreyfarrell Are you still working on the new json raw type? |
#446 is the last I've worked on this. Unfortunately I've been busy with other commitments lately. Until I get some critical feedback on my PR it's unlikely to see any further progress. |
All numbers or only overflowing numbers, or maybe two separate flags?
The text was updated successfully, but these errors were encountered: