From 58c13019563a2f62468f97226a71306cb68a2f31 Mon Sep 17 00:00:00 2001 From: sttawm Date: Fri, 7 May 2021 10:53:00 -0700 Subject: [PATCH 1/2] add a parser option to validate oneofs The exception looks like: ``` scalapb.json4s.JsonFormatException: Overlapping keys in oneof: primitive, wrapper ``` --- .../scala/scalapb/json4s/JsonFormat.scala | 25 ++++++++++++++++++- .../scala/scalapb/json4s/JavaAssertions.scala | 7 ++++++ .../scala/scalapb/json4s/JsonFormatSpec.scala | 14 +++++++++++ 3 files changed, 45 insertions(+), 1 deletion(-) diff --git a/src/main/scala/scalapb/json4s/JsonFormat.scala b/src/main/scala/scalapb/json4s/JsonFormat.scala index 8723049..303ad34 100644 --- a/src/main/scala/scalapb/json4s/JsonFormat.scala +++ b/src/main/scala/scalapb/json4s/JsonFormat.scala @@ -461,6 +461,7 @@ class Printer private (config: Printer.PrinterConfig) { object Parser { private final case class ParserConfig( isIgnoringUnknownFields: Boolean, + failOnOverlappingOneofKeys: Boolean, mapEntriesAsKeyValuePairs: Boolean, formatRegistry: FormatRegistry, typeRegistry: TypeRegistry @@ -472,6 +473,7 @@ class Parser private (config: Parser.ParserConfig) { this( Parser.ParserConfig( isIgnoringUnknownFields = false, + failOnOverlappingOneofKeys = false, mapEntriesAsKeyValuePairs = false, JsonFormat.DefaultRegistry, TypeRegistry.empty @@ -490,6 +492,7 @@ class Parser private (config: Parser.ParserConfig) { this( Parser.ParserConfig( isIgnoringUnknownFields = false, + failOnOverlappingOneofKeys = false, mapEntriesAsKeyValuePairs = false, formatRegistry, typeRegistry @@ -499,6 +502,9 @@ class Parser private (config: Parser.ParserConfig) { def ignoringUnknownFields: Parser = new Parser(config.copy(isIgnoringUnknownFields = true)) + def failOnOverlappingOneofKeys: Parser = + new Parser(config.copy(failOnOverlappingOneofKeys = true)) + def mapEntriesAsKeyValuePairs: Parser = new Parser(config.copy(mapEntriesAsKeyValuePairs = true)) @@ -527,7 +533,24 @@ class Parser private (config: Parser.ParserConfig) { value: JValue, skipTypeUrl: Boolean )(implicit cmp: GeneratedMessageCompanion[A]): A = { - cmp.messageReads.read(fromJsonToPMessage(cmp, value, skipTypeUrl)) + val message = fromJsonToPMessage(cmp, value, skipTypeUrl) + if (config.failOnOverlappingOneofKeys) { + validateOneofs(message) + } + cmp.messageReads.read(message) + } + + private def validateOneofs[A <: GeneratedMessage](message: PMessage) = { + message.value.keys + .groupBy(_.containingOneof) + .filter(x => x._1.isDefined && x._2.size > 1) + .values + .headOption + .foreach(keys => + throw new JsonFormatException( + s"Overlapping keys in oneof: ${keys.map(_.name).mkString(", ")}" + ) + ) } private def fromJsonToPMessage( diff --git a/src/test/scala/scalapb/json4s/JavaAssertions.scala b/src/test/scala/scalapb/json4s/JavaAssertions.scala index 25d36c5..db41a85 100644 --- a/src/test/scala/scalapb/json4s/JavaAssertions.scala +++ b/src/test/scala/scalapb/json4s/JavaAssertions.scala @@ -31,6 +31,13 @@ class IgnoringUnknownParserContext { ) } +class StrictOneofParserContext { + implicit val pc: ParserContext = ParserContext( + new Parser().failOnOverlappingOneofKeys, + JavaJsonFormat.parser // by default the java parser fails on overlapping keys + ) +} + trait JavaAssertions { self: AnyFlatSpec with Matchers => diff --git a/src/test/scala/scalapb/json4s/JsonFormatSpec.scala b/src/test/scala/scalapb/json4s/JsonFormatSpec.scala index 529578a..095503f 100644 --- a/src/test/scala/scalapb/json4s/JsonFormatSpec.scala +++ b/src/test/scala/scalapb/json4s/JsonFormatSpec.scala @@ -7,6 +7,7 @@ import com.google.protobuf.any.{Any => PBAny} import com.google.protobuf.util.JsonFormat.{TypeRegistry => JavaTypeRegistry} import com.google.protobuf.util.{JsonFormat => JavaJsonFormat} import jsontest.custom_collection.{Guitar, Studio} +import jsontest.oneof.OneOf import jsontest.test._ import jsontest.test3._ import org.json4s.JsonDSL._ @@ -674,6 +675,19 @@ class JsonFormatSpec JsonFormat.parser.fromJsonString[TestAllTypes](javaJson) must be(obj) } + "oneofs" should "fail for overlapping keys if failOnOverlappingOneofKeys" in new StrictOneofParserContext { + val extraKey = """{"primitive": "", "wrapper": ""}""" + assertFails(extraKey, OneOf) + } + + "oneofs" should "not fail for overlapping keys if failOnOverlappingOneofKeys with a scala parser" in new DefaultParserContext { + val extraKey = """{"primitive": "", "wrapper": ""}""" + val parsedScala = pc.scalaParser.fromJsonString[OneOf](extraKey)(OneOf) + parsedScala must be( + OneOf(field = OneOf.Field.Primitive("")) + ) + } + "TestProto" should "be TestJsonWithMapEntriesAsKeyValuePairs when converted to Proto with mapEntriesAsKeyValuePairs setting" in { JsonFormat.printer.mapEntriesAsKeyValuePairs.toJson(TestProto) must be( parse(TestJsonWithMapEntriesAsKeyValuePairs) From 853ffefd746c77e65d29dd43e9c4c4880c4c8de3 Mon Sep 17 00:00:00 2001 From: sttawm Date: Mon, 10 May 2021 10:28:10 -0700 Subject: [PATCH 2/2] code review * fail on overlapping oneof fields by default * update names * use 'field' instead of 'key' for consistency * use 'isIgnoringOverlappingOneofFields' and 'ignoringOverlappingOneofFields' for consistency --- .../scala/scalapb/json4s/JsonFormat.scala | 40 ++++++++----------- .../scala/scalapb/json4s/JavaAssertions.scala | 7 ---- .../scala/scalapb/json4s/JsonFormatSpec.scala | 7 ++-- 3 files changed, 21 insertions(+), 33 deletions(-) diff --git a/src/main/scala/scalapb/json4s/JsonFormat.scala b/src/main/scala/scalapb/json4s/JsonFormat.scala index 303ad34..dd4ad60 100644 --- a/src/main/scala/scalapb/json4s/JsonFormat.scala +++ b/src/main/scala/scalapb/json4s/JsonFormat.scala @@ -461,7 +461,7 @@ class Printer private (config: Printer.PrinterConfig) { object Parser { private final case class ParserConfig( isIgnoringUnknownFields: Boolean, - failOnOverlappingOneofKeys: Boolean, + isIgnoringOverlappingOneofFields: Boolean, mapEntriesAsKeyValuePairs: Boolean, formatRegistry: FormatRegistry, typeRegistry: TypeRegistry @@ -473,7 +473,7 @@ class Parser private (config: Parser.ParserConfig) { this( Parser.ParserConfig( isIgnoringUnknownFields = false, - failOnOverlappingOneofKeys = false, + isIgnoringOverlappingOneofFields = false, mapEntriesAsKeyValuePairs = false, JsonFormat.DefaultRegistry, TypeRegistry.empty @@ -492,7 +492,7 @@ class Parser private (config: Parser.ParserConfig) { this( Parser.ParserConfig( isIgnoringUnknownFields = false, - failOnOverlappingOneofKeys = false, + isIgnoringOverlappingOneofFields = false, mapEntriesAsKeyValuePairs = false, formatRegistry, typeRegistry @@ -502,8 +502,8 @@ class Parser private (config: Parser.ParserConfig) { def ignoringUnknownFields: Parser = new Parser(config.copy(isIgnoringUnknownFields = true)) - def failOnOverlappingOneofKeys: Parser = - new Parser(config.copy(failOnOverlappingOneofKeys = true)) + def ignoringOverlappingOneofFields: Parser = + new Parser(config.copy(isIgnoringOverlappingOneofFields = true)) def mapEntriesAsKeyValuePairs: Parser = new Parser(config.copy(mapEntriesAsKeyValuePairs = true)) @@ -533,24 +533,7 @@ class Parser private (config: Parser.ParserConfig) { value: JValue, skipTypeUrl: Boolean )(implicit cmp: GeneratedMessageCompanion[A]): A = { - val message = fromJsonToPMessage(cmp, value, skipTypeUrl) - if (config.failOnOverlappingOneofKeys) { - validateOneofs(message) - } - cmp.messageReads.read(message) - } - - private def validateOneofs[A <: GeneratedMessage](message: PMessage) = { - message.value.keys - .groupBy(_.containingOneof) - .filter(x => x._1.isDefined && x._2.size > 1) - .values - .headOption - .foreach(keys => - throw new JsonFormatException( - s"Overlapping keys in oneof: ${keys.map(_.name).mkString(", ")}" - ) - ) + cmp.messageReads.read(fromJsonToPMessage(cmp, value, skipTypeUrl)) } private def fromJsonToPMessage( @@ -616,12 +599,23 @@ class Parser private (config: Parser.ParserConfig) { case None => value match { case JObject(fields) => + val usedOneofs = mutable.Set[OneofDescriptor]() val fieldMap = JsonFormat.MemorizedFieldNameMap(cmp.scalaDescriptor) val valueMapBuilder = Map.newBuilder[FieldDescriptor, PValue] fields.foreach { case (name: String, jValue: JValue) => if (fieldMap.contains(name)) { if (jValue != JNull) { val fd = fieldMap(name) + fd.containingOneof.foreach(o => + if ( + !config.isIgnoringOverlappingOneofFields && !usedOneofs + .add(o) + ) { + throw new JsonFormatException( + s"Overlapping field '${name}' in oneof" + ) + } + ) valueMapBuilder += (fd -> parseValue(fd, jValue)) } } else if ( diff --git a/src/test/scala/scalapb/json4s/JavaAssertions.scala b/src/test/scala/scalapb/json4s/JavaAssertions.scala index db41a85..25d36c5 100644 --- a/src/test/scala/scalapb/json4s/JavaAssertions.scala +++ b/src/test/scala/scalapb/json4s/JavaAssertions.scala @@ -31,13 +31,6 @@ class IgnoringUnknownParserContext { ) } -class StrictOneofParserContext { - implicit val pc: ParserContext = ParserContext( - new Parser().failOnOverlappingOneofKeys, - JavaJsonFormat.parser // by default the java parser fails on overlapping keys - ) -} - trait JavaAssertions { self: AnyFlatSpec with Matchers => diff --git a/src/test/scala/scalapb/json4s/JsonFormatSpec.scala b/src/test/scala/scalapb/json4s/JsonFormatSpec.scala index 095503f..41a3fd4 100644 --- a/src/test/scala/scalapb/json4s/JsonFormatSpec.scala +++ b/src/test/scala/scalapb/json4s/JsonFormatSpec.scala @@ -675,14 +675,15 @@ class JsonFormatSpec JsonFormat.parser.fromJsonString[TestAllTypes](javaJson) must be(obj) } - "oneofs" should "fail for overlapping keys if failOnOverlappingOneofKeys" in new StrictOneofParserContext { + "oneofs" should "fail for overlapping keys if failOnOverlappingOneofKeys" in new DefaultParserContext { val extraKey = """{"primitive": "", "wrapper": ""}""" assertFails(extraKey, OneOf) } - "oneofs" should "not fail for overlapping keys if failOnOverlappingOneofKeys with a scala parser" in new DefaultParserContext { + "oneofs" should "not fail for overlapping keys if ignoreOverlappingOneofKeys" in { val extraKey = """{"primitive": "", "wrapper": ""}""" - val parsedScala = pc.scalaParser.fromJsonString[OneOf](extraKey)(OneOf) + val scalaParser = new Parser().ignoringOverlappingOneofFields + val parsedScala = scalaParser.fromJsonString[OneOf](extraKey)(OneOf) parsedScala must be( OneOf(field = OneOf.Field.Primitive("")) )