Skip to content
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

fix #18007: std/json now serializes nan,inf,-inf as strings instead of invalid json #18026

Merged
merged 7 commits into from
May 16, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,8 @@
- `jsonutils` now serializes/deserializes holey enums as regular enums (via `ord`) instead of as strings.
Use `-d:nimLegacyJsonutilsHoleyEnum` for a transition period.

- `json` and `jsonutils` now serialize NaN, Inf, -Inf as strings, so that
`%[NaN, -Inf]` is the string `["nan","-inf"]` instead of `[nan,-inf]` which was invalid json.

## Standard library additions and changes
- Added support for parenthesized expressions in `strformat`
Expand Down
121 changes: 71 additions & 50 deletions lib/pure/json.nim
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,16 @@ proc `%`*(n: BiggestInt): JsonNode =

proc `%`*(n: float): JsonNode =
## Generic constructor for JSON data. Creates a new `JFloat JsonNode`.
result = JsonNode(kind: JFloat, fnum: n)
runnableExamples:
assert $(%[NaN, Inf, -Inf, 0.0, -0.0, 1.0, 1e-2]) == """["nan","inf","-inf",0.0,-0.0,1.0,0.01]"""
assert (%NaN).kind == JString
assert (%0.0).kind == JFloat
# for those special cases, we could also have used `newJRawNumber` but then
# it would've been inconsisten with the case of `parseJson` vs `%` for representing them.
if n != n: newJString("nan")
timotheecour marked this conversation as resolved.
Show resolved Hide resolved
elif n == Inf: newJString("inf")
elif n == -Inf: newJString("-inf")
else: JsonNode(kind: JFloat, fnum: n)

proc `%`*(b: bool): JsonNode =
## Generic constructor for JSON data. Creates a new `JBool JsonNode`.
Expand Down Expand Up @@ -662,6 +671,47 @@ proc escapeJson*(s: string): string =
result = newStringOfCap(s.len + s.len shr 3)
escapeJson(s, result)

proc toUgly*(result: var string, node: JsonNode) =
## Converts `node` to its JSON Representation, without
## regard for human readability. Meant to improve `$` string
## conversion performance.
##
## JSON representation is stored in the passed `result`
##
## This provides higher efficiency than the `pretty` procedure as it
## does **not** attempt to format the resulting JSON to make it human readable.
var comma = false
case node.kind:
of JArray:
result.add "["
for child in node.elems:
if comma: result.add ","
else: comma = true
result.toUgly child
result.add "]"
of JObject:
result.add "{"
for key, value in pairs(node.fields):
if comma: result.add ","
else: comma = true
key.escapeJson(result)
result.add ":"
result.toUgly value
result.add "}"
of JString:
if node.isUnquoted:
result.add node.str
else:
escapeJson(node.str, result)
of JInt:
result.addInt(node.num)
of JFloat:
result.addFloat(node.fnum)
of JBool:
result.add(if node.bval: "true" else: "false")
of JNull:
result.add "null"

proc toPretty(result: var string, node: JsonNode, indent = 2, ml = true,
lstArr = false, currIndent = 0) =
case node.kind
Expand Down Expand Up @@ -689,10 +739,7 @@ proc toPretty(result: var string, node: JsonNode, indent = 2, ml = true,
result.add("{}")
of JString:
if lstArr: result.indent(currIndent)
if node.isUnquoted:
result.add node.str
else:
escapeJson(node.str, result)
toUgly(result, node)
of JInt:
if lstArr: result.indent(currIndent)
result.addInt(node.num)
Expand Down Expand Up @@ -743,47 +790,6 @@ proc pretty*(node: JsonNode, indent = 2): string =
result = ""
toPretty(result, node, indent)

proc toUgly*(result: var string, node: JsonNode) =
## Converts `node` to its JSON Representation, without
## regard for human readability. Meant to improve `$` string
## conversion performance.
##
## JSON representation is stored in the passed `result`
##
## This provides higher efficiency than the `pretty` procedure as it
## does **not** attempt to format the resulting JSON to make it human readable.
var comma = false
case node.kind:
of JArray:
result.add "["
for child in node.elems:
if comma: result.add ","
else: comma = true
result.toUgly child
result.add "]"
of JObject:
result.add "{"
for key, value in pairs(node.fields):
if comma: result.add ","
else: comma = true
key.escapeJson(result)
result.add ":"
result.toUgly value
result.add "}"
of JString:
if node.isUnquoted:
result.add node.str
else:
node.str.escapeJson(result)
of JInt:
result.addInt(node.num)
of JFloat:
result.addFloat(node.fnum)
of JBool:
result.add(if node.bval: "true" else: "false")
of JNull:
result.add "null"

proc `$`*(node: JsonNode): string =
## Converts `node` to its JSON Representation on one line.
result = newStringOfCap(node.len shl 1)
Expand Down Expand Up @@ -1087,11 +1093,26 @@ when defined(nimFixedForwardGeneric):
dst = cast[T](jsonNode.num)

proc initFromJson[T: SomeFloat](dst: var T; jsonNode: JsonNode; jsonPath: var string) =
verifyJsonKind(jsonNode, {JInt, JFloat}, jsonPath)
if jsonNode.kind == JFloat:
dst = T(jsonNode.fnum)
if jsonNode.kind == JString:
case jsonNode.str
of "nan":
let b = NaN
dst = T(b)
# dst = NaN # would fail some tests because range conversions would cause CT error
# in some cases; but this is not a hot-spot inside this branch and backend can optimize this.
of "inf":
let b = Inf
dst = T(b)
of "-inf":
let b = -Inf
dst = T(b)
else: raise newException(JsonKindError, "expected 'nan|inf|-inf', got " & jsonNode.str)
else:
dst = T(jsonNode.num)
verifyJsonKind(jsonNode, {JInt, JFloat}, jsonPath)
if jsonNode.kind == JFloat:
dst = T(jsonNode.fnum)
else:
dst = T(jsonNode.num)

proc initFromJson[T: enum](dst: var T; jsonNode: JsonNode; jsonPath: var string) =
verifyJsonKind(jsonNode, {JString}, jsonPath)
Expand Down
3 changes: 3 additions & 0 deletions lib/std/jsonutils.nim
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ runnableExamples:
let a = (1.5'f32, (b: "b2", a: "a2"), 'x', @[Foo(t: true, z1: -3), nil], [{"name": "John"}.newStringTable])
let j = a.toJson
assert j.jsonTo(typeof(a)).toJson == j
assert $[NaN, Inf, -Inf, 0.0, -0.0, 1.0, 1e-2].toJson == """["nan","inf","-inf",0.0,-0.0,1.0,0.01]"""
assert 0.0.toJson.kind == JFloat
assert Inf.toJson.kind == JString

import json, strutils, tables, sets, strtabs, options

Expand Down
23 changes: 23 additions & 0 deletions tests/stdlib/tjson.nim
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,27 @@ Note: Macro tests are in tests/stdlib/tjsonmacro.nim
]#

import std/[json,parsejson,strutils]
from std/math import isNaN
when not defined(js):
import std/streams

proc testRoundtrip[T](t: T, expected: string) =
# checks that `T => json => T2 => json2` is such that json2 = json
let j = %t
doAssert $j == expected, $j
doAssert %(j.to(T)) == j

proc testRoundtripVal[T](t: T, expected: string) =
# similar to testRoundtrip, but also checks that the `T => json => T2` is such that `T2 == T`
# note that this isn't always possible, e.g. for pointer-like types or nans
let j = %t
doAssert $j == expected, $j
let j2 = ($j).parseJson
doAssert $j2 == expected, $(j2, t)
let t2 = j2.to(T)
doAssert t2 == t
doAssert $(%* t2) == expected # sanity check, because -0.0 = 0.0 but their json representation differs

let testJson = parseJson"""{ "a": [1, 2, 3, 4], "b": "asd", "c": "\ud83c\udf83", "d": "\u00E6"}"""
# nil passthrough
doAssert(testJson{"doesnt_exist"}{"anything"}.isNil)
Expand Down Expand Up @@ -301,6 +314,16 @@ block: # bug #17383
testRoundtrip(int64.high): "9223372036854775807"
testRoundtrip(uint64.high): "18446744073709551615"

block: # bug #18007
testRoundtrip([NaN, Inf, -Inf, 0.0, -0.0, 1.0]): """["nan","inf","-inf",0.0,-0.0,1.0]"""
# pending https://github.com/nim-lang/Nim/issues/18025 use:
# testRoundtrip([float32(NaN), Inf, -Inf, 0.0, -0.0, 1.0])
let inf = float32(Inf)
testRoundtrip([float32(NaN), inf, -inf, 0.0, -0.0, 1.0]): """["nan","inf","-inf",0.0,-0.0,1.0]"""
when not defined(js): # because of Infinity vs inf
testRoundtripVal([inf, -inf, 0.0, -0.0, 1.0]): """["inf","-inf",0.0,-0.0,1.0]"""
let a = parseJson($(%NaN)).to(float)
doAssert a.isNaN

block:
let a = "18446744073709551615"
Expand Down
21 changes: 20 additions & 1 deletion tests/stdlib/tjsonutils.nim
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,28 @@ discard """

import std/jsonutils
import std/json
from std/math import isNaN

proc testRoundtrip[T](t: T, expected: string) =
# checks that `T => json => T2 => json2` is such that json2 = json
let j = t.toJson
doAssert $j == expected, $j
doAssert $j == expected, "\n" & $j & "\n" & expected
doAssert j.jsonTo(T).toJson == j
var t2: T
t2.fromJson(j)
doAssert t2.toJson == j

proc testRoundtripVal[T](t: T, expected: string) =
# similar to testRoundtrip, but also checks that the `T => json => T2` is such that `T2 == T`
# note that this isn't always possible, e.g. for pointer-like types.
let j = t.toJson
let j2 = $j
doAssert j2 == expected, j2
let j3 = j2.parseJson
let t2 = j3.jsonTo(T)
doAssert t2 == t
doAssert $t2.toJson == j2 # still needed, because -0.0 = 0.0 but their json representation differs

import tables, sets, algorithm, sequtils, options, strtabs
from strutils import contains

Expand Down Expand Up @@ -91,6 +104,12 @@ template fn() =
else:
testRoundtrip(a): "[9223372036854775807,18446744073709551615]"

block: # bug #18007
testRoundtrip((NaN, Inf, -Inf, 0.0, -0.0, 1.0)): """["nan","inf","-inf",0.0,-0.0,1.0]"""
testRoundtrip((float32(NaN), Inf, -Inf, 0.0, -0.0, 1.0)): """["nan","inf","-inf",0.0,-0.0,1.0]"""
testRoundtripVal((Inf, -Inf, 0.0, -0.0, 1.0)): """["inf","-inf",0.0,-0.0,1.0]"""
doAssert ($NaN.toJson).parseJson.jsonTo(float).isNaN

block: # case object
type Foo = object
x0: float
Expand Down