Update JSON Parsing to Optionally Allow Numeric Booleans

[rdsarvar]

Updates the JSON Parser to allow for numeric booleans.

For context, although numeric booleans are not widely used there are a few specifications that directly require it such as OpenRTB.

I also toyed with the idea of converting from JInt -> JBool in a high level function in order to limit the surface area of the change.

Looking to get feedback on these changes and if there is another means to do the same functionality without having to update the parsePrimitive method.

Bonus points if this change isn't actually required to do the parsing - there's a chance I missed something while reading through.

[thesamet]

Thanks @rdsarvar , the goal of this parser is to be consistent with the protobuf json spec. Since OpenRTB uses protobuf, do you know why their json output is inconsistent with that? If it's inconsistent with this details it might be inconsistent on a few others (which would make this fix insufficient). Is it possible to resolve this within OpenRTB?

[rdsarvar]

Unfortunately I don't know if this is something that we would be able to bring to the OpenRTB specification as it goes through cycles every few years (2.6 was released this year after 6 years). As such this is likely not feasible in the short term (not including the sheer number of companies that this would impact).

I'm not 100% certain on the reasoning behind the Google's JSON schema of OpenRTB leveraging 0 and 1. Based on the OpenRTB spec I would imagine they're doing this to stay within the bounds of the spec itself (there is no notion of boolean, all true/false are integer 0/1). From the top of my head the only thing I can think of is consistency and it shaves off some bytes and mirrors how the data is writing on the protobuf side.

Would there be any other way to leverage this library to parse the numeric bools?

An example that is being tried is the following, but I have yet to look into what the cost of doing this additional switch as it'll get run in our case many billions of times per day:
Edit: Please note that by extending the Parser class you wouldn't be able to set the private configurations like "ignore unknown fields" etc

class TestParser extends Parser {
  override def parseSingleValue(containerCompanion: GeneratedMessageCompanion[_], fd: FieldDescriptor, value: JValue): PValue = {
    (fd.protoType, value) match {
      case (Type.TYPE_BOOL, JInt(i)) => {
        if (i == 0) {
          PBoolean(false)
        } else if (i == 1) {
          PBoolean(true)
        } else {
          throw new JsonFormatException(
            s"Unexpected value ($i) for field ${fd.name} of ${fd.containingMessage.name}"
          )
        }
      }
      case (_, _) => super.parseSingleValue(containerCompanion, fd, value)
    }
  }
}

The main benefit of the optional numeric at the lower level as I see it is it gives free range to users and it's logically colocated with the other type conversions. Although, I can see that from your side by adding additional functionality it could hurt us when (if) the spec were to make a change that conflicts with optional numeric booleans.

[rdsarvar]

Sorry ignore the code snippet implementation in the comment above - this wouldn't be feasible as with the pipelined configuration approach you wouldn't be able to set the private configurations of the super.