Replace json-simple with Jackson - #139
Conversation
Replace org.json.simple dependency with Jackson (jackson-databind). Update jitsi-utils to 1.0-SNAPSHOT. JSONSerializer/JSONDeserializer now use ObjectNode/ArrayNode.
|
|
||
| if (s.startsWith("-")) | ||
| { | ||
| i = Integer.parseInt(s); |
There was a problem hiding this comment.
I don't understand what this code here is doing. Is it trying to handle the case where ssrc's accidentally got serialized as signed 32-bit values, or is it handling the -1 meaning missing ssrc? Either way I think your changes change the behavior, though I'm not sure if it makes it better or worse. Will we ever expect SSRCs to be JSON strings rather than JSON numbers?
There was a problem hiding this comment.
I don't know what the original intent was, handling -1 seems like a reasonable guess. I reverted the functional change.
Replace jsonMapper.createObjectNode()/createArrayNode() with JsonNodeFactory.instance.objectNode()/arrayNode() everywhere. Remove now-unused jacksonObjectMapper instance and import.
Revert unintentional functional change from json-simple migration: deserializeSSRC was changed from returning int to long. Restore int return type and use asInt() to match original behavior exactly.
| { | ||
| deserializeWebsocket( | ||
| (String)webSocket, | ||
| webSocket.isTextual() ? webSocket.asText() : null, |
There was a problem hiding this comment.
This changes the error path on a non-textual item from a bad cast exception to a silent null in the data - do we care?
| for (Map.Entry<Object, Object> e | ||
| : (Iterable<Map.Entry<Object, Object>>) parameters | ||
| .entrySet()) | ||
| parameters.fields().forEachRemaining(e -> |
There was a problem hiding this comment.
Are forEach and forEachRemaining different here? Why did you use the latter?
| ObjectNode rtcpFb = (ObjectNode) iter; | ||
| JsonNode typeNode = rtcpFb.get(RtcpFbPacketExtension.TYPE_ATTR_NAME); | ||
| JsonNode subtypeNode = rtcpFb.get(RtcpFbPacketExtension.SUBTYPE_ATTR_NAME); | ||
| String type = (typeNode != null && typeNode.isTextual()) ? typeNode.asText() : null; |
There was a problem hiding this comment.
Similarly here - I worry that this will introduce silent nulls into code that's not expecting it, and thus trigger exceptions in different places in the code than would previously have had them.
Replace silent ignore patterns with IllegalArgumentException where a field is present but has an unexpected JSON type. Previously the code would silently skip or return null; now it fails explicitly with a descriptive message.
Replace all silent-ignore type checks with require() calls that throw IllegalArgumentException on unexpected field types. Also fixes headers.fields() deprecated call to use properties().
3 participants
Replace the
org.json.simpledependency with Jackson (jackson-databind).JSONSerializer,JSONDeserializer(colibri),Colibri2JSONSerializer,Colibri2JSONDeserializerto useObjectNode/ArrayNodeObjectNode/ArrayNodeinstead ofJSONObject/JSONArrayjitsi-utilsto1.0-SNAPSHOT