fix tuple conversion to JsonNode from % in pure/json.nim
#24082
Conversation
`%` - Conversion of an object or tuple to JsonNode doesn't work correctly for tuples atm, since the generic type is missing. Added the tuple generic type to correctly allow tuples as input as well. Credits to Elegantbeef from the Nim Discord.
In the issue you mentioned there is a comment in the code saying:
I just ran this code snippet from issue #10019 (slightly changed echo messages): proc fun1[T](elements: seq[T]) =
echo "ok1a"
proc fun1(o: object|tuple) =
echo "ok2"
proc fun2[T](elements: openArray[T]) =
echo "ok1b"
proc fun2(o: object) =
echo "ok2"
proc fun_bug[T](elements: openArray[T]) =
echo "ok1c"
proc fun_bug(o: object|tuple) =
echo "ok2"
type
Obj1[T] = object
v: T
converter toObj1[T](t: T): Obj1[T] = return Obj1[T](v: t)
proc main()=
block:
var x = @["hello", "world"]
#puts out ok1a
fun1(x)
block:
var x = ["hello", "world"]
#puts out ok1b
fun2(x)
block:
var x = ["hello", "world"]
#puts out ok1c - instead of the previous ambiguity error
fun_bug(x)
main()And it seems to be working without a problem now. Whatever the issue was before, it seems to have been fixed over the years lol. So should I leave the commit as is or do you want me to change it to its own proc with the tuple type only instead? |
|
It should be fine as is then. I realized now that the |
Deleted old test: not compiles(%{"error": "No messages"})
Added new test to check whether tuple to JsonNode conversion via `%` works correctly now. (Both for named tuple and anonymous tuple.)
Interesting background info I stumbled upon:
At some point in the past, anonymous tuples and named tuples were converted to different JSON types. Anonymous tuples were converted to JSON arrays (because their fields didn't have names) and named tuples to JSON objects (because their fields had names). At some point this was (correctly) changed, and both now convert to JSON objects. For anonymous tuples the respective JSON object field-names are constructed by naming them as such: "Field0" for field 0, "Field1" for field 1, "Field2" for field 2, etc. ...
|
Had to change the old test a bit, since anon tuples now convert to objects instead of arrays. Honestly I don't get the difference between %* and % since both seem to do the exact same thing. Just to be sure I included a test for both of them. Edit: Sorry about the tagging earlier, I just deleted the new message. I realized the question was dumb lol. xD Echo must implicitly convert the JsonNode to a string, so assert wouldnt do that on its own. |
forgot to remove a bracket "]" and replaced it with "}" whoops
Update tjson.nim - applied camelCase to the test vars to fit the conventions
|
Sorry, now that I think about it, if As mentioned in #16851 though |
|
Thanks for the thorough explanation, still took me a while to understand what the problem is.
No need for apologies, wasn't by intention anyways. I'd still like to try and fix this though. So something like Now, I was wondering why that would happen, since the function in question doesn't even accept arrays. However, this function does: proc `%`*[T](elements: openArray[T]): JsonNode =
## Generic constructor for JSON data. Creates a new `JArray JsonNode`
result = newJArray()
for elem in elements: result.add(%elem)Usually, it wouldn't process tuples inside the array, since Isn't there an exclude operator for generics? Something like |
Nim converts {"key":"val"} to [("key", "val")] at compile time. So {} is a tuple inside an array actually.
Using the % operator on this will pass it as an argument to the respective function that takes an openarray as argument and convert the tuple inside this array:
%{"key":"val"} becomes [{"Field0":"key","Field1":"val"}].
As to why this is a problem, I do not know, but two tests failed because of that.
Before, this wasn't a problem, because one of the `%` procedures that was supposed to accept tuples and convert them to JsonNodes only accepted objects. Now that we also want to convert tuples to a JSON object as well via `%`, we have to make sure that {} is not handled as a tuple inside an array also.
Yes, Also really sorry again, just noticed this PR is against branch Another thing: the implementation added in #10823 including making an array for non-named tuples is also the one used in jsonutils, so for symmetry you might want to copy the proc in #10823. Sorry for dragging you along so much lol |
|
No worries! I'm actually glad you're holding my hand through this and explaining me stuff, as I haven't had much experiences with commits at all, lol. So thank you 😁.
Oh yeah, good to know, I noticed that too, but didn't really know what it meant earlier. In any case, not that many lines to change and most of them will be copy-and-paste anyways.
Alright! Will do that. I will make sure to keep the |
|
Arguably a tuple should be mapped to a JSON array, not an object, esp. since I personally consider tuples with named fields an obsolete design (just use an object). |
Understandable, but it's very counterintuitive when it comes to named tuples. Edits: Now that I think about it, as far as I know, I can't create an object on the fly but have to create a custom object type for it first. I do not have to do that for a named tuple. Please correct me if I'm wrong, though. I haven't worked with objects that much yet. |
|
Opened new PR here: #24089. Hopefully I didn't miss anything. |
%- Conversion of an object or tuple to JsonNode doesn't work correctly for tuples atm, since the generic type is missing. Added the tuple generic type to correctly allow tuples as input as well. Credits to Elegantbeef from the Nim Discord.