JSON output no longer quotes all values #1298
Conversation
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/infoset/JsonInfosetInputter.scala
Show resolved
Hide resolved
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/infoset/JsonInfosetInputter.scala
Outdated
Show resolved
Hide resolved
...odil-runtime1/src/main/scala/org/apache/daffodil/runtime1/infoset/JsonInfosetOutputter.scala
Outdated
Show resolved
Hide resolved
daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/infoset/JsonInfosetInputter.scala
Outdated
Show resolved
Hide resolved
parkera101093
force-pushed
the
DAFFODIL-2362
branch
2 times, most recently
from
4f4512e
to
7cba3d7
Compare
| @@ -118,10 +123,11 @@ class JsonInfosetInputter(input: java.io.InputStream) extends InfosetInputter { | |||
| primType: NodeInfo.Kind, | |||
| runtimeProperties: java.util.Map[String, String] | |||
| ): String = { | |||
| if (jsp.getCurrentToken() == JsonToken.VALUE_NULL) { | |||
| if (!jsp.getCurrentToken().isScalarValue()) { | |||
| throw new NonTextFoundInSimpleContentException(jsp.getCurrentToken().asString()) | |||
There was a problem hiding this comment.
I think we can make this diagnostic more helpful to users. Maybe something like this:
"Unexpected array or object '" + getLocalName + "' on line " + jsp.getTokenLocation().getLineNr()We should have a test that this fails correctly for both arrays and objects.
| (simple.metadata.dfdlType == DFDLPrimType.Double && text.equals("-INF")) || | ||
| (simple.metadata.dfdlType == DFDLPrimType.Float && text.equals("NaN")) || | ||
| (simple.metadata.dfdlType == DFDLPrimType.Float && text.equals("INF")) || | ||
| (simple.metadata.dfdlType == DFDLPrimType.Float && text.equals("-INF"))) { |
There was a problem hiding this comment.
It's usually best to avoid string comparisons when there's something more specific available. String comparisons can silently break if we ever decide to change the string representation but don't check the condition. For example, Double and Float have isInfinite and isNaN functions.
This is also a pretty big if-statement. I might suggest moving this all to a separate private function, something like this:
private def needsQuote(simple: InofsetSimpleElement): Boolean = {
simple.metadata.dfdlType match {
case DFDLPrimType.String => true
case DFDLPrimType.HexBinary => true
case DFDLprimType.AnyURI => true
...
// json does not support inf/nan double/float so they must be quoted
case DFDLPrimType.Double => {
val d = simple.getDouble
d.isInfinite || d.isNaN
}
case DFDLPrimType.Float => {
val f = simple.getFloat
f.isInfinity || f.isNan
}
case _ => false
}Then this is statement just becomes if (needsQuotes(simple)) ..... This makes it much easier to understand what this if-statement is checking for without having to decipher it. And it makes it easier to add comments for unexpected edgecases like inf/nan support.
JSONInfosetInputter and Outputter were treating all values as strings. This meant everyhing was quoted and was messier for the user to read. In addition, not all json comes from dfdl, so it makes sense to support numbers. The JSONInfosetInputter's getSimpleText function also throws a NonTextFoundInSimpleContentException if a node in a nonscalar value, since finding a nonscalar (such as an object or array) when your expecting a simple value means the infoset does not match. DAFFODIL-2362 delete me
parkera101093
force-pushed
the
DAFFODIL-2362
branch
from
7cba3d7
to
6861528
Compare
| @@ -118,10 +123,11 @@ class JsonInfosetInputter(input: java.io.InputStream) extends InfosetInputter { | |||
| primType: NodeInfo.Kind, | |||
| runtimeProperties: java.util.Map[String, String] | |||
| ): String = { | |||
| if (jsp.getCurrentToken() == JsonToken.VALUE_NULL) { | |||
| if (!jsp.getCurrentToken().isScalarValue()) { | |||
| throw new NonTextFoundInSimpleContentException("Unexpected array or object '" + getLocalName + "' on line " + jsp.getTokenLocation().getLineNr()) | |||
There was a problem hiding this comment.
Can you add tests for this to make sure this works as expected?
| d.isInfinite || d.isNaN | ||
| } | ||
| case DFDLPrimType.Float => { | ||
| val f = simple.getFloat.toDouble |
There was a problem hiding this comment.
You don't need to convert the float to a double, the Float class has isInfinite and isNan functions.
JSONInfosetInputter and Outputter were treating all values as strings. This meant everyhing was quoted and was messier for the user to read. In addition, not all json comes from dfdl, so it makes sense to support numbers.
As of the time of this commit 156 tests fail as the changes cause some issues with dates and NaN, INF, and -INF values as well as numbers with a large amount of decimals such as 0.1234567898765432123456789 from test decimal_constructor_06.
DAFFODIL-2362