add a parser option to validate oneofs #236
Conversation
The exception looks like: ``` scalapb.json4s.JsonFormatException: Overlapping keys in oneof: primitive, wrapper ```
sttawm
force-pushed
the
1117-validate-oneofs
branch
from
08551ca to
58c1301
Compare
| @@ -461,6 +461,7 @@ class Printer private (config: Printer.PrinterConfig) { | |||
| object Parser { | |||
| private final case class ParserConfig( | |||
| isIgnoringUnknownFields: Boolean, | |||
| failOnOverlappingOneofKeys: Boolean, | |||
There was a problem hiding this comment.
I'd like the default behavior to be consistent with the Java implementation, despite of a possible breakage for users. Let's rename to ignoreOverlappingOneofKeys defaults to false.
There was a problem hiding this comment.
I updated the name in the latest commit. However, I chose isIgnoringOverlappingOneofFields for consistency. Let me know if in fact you want ignoreOverlappingOneofKeys
|
|
||
| private def validateOneofs[A <: GeneratedMessage](message: PMessage) = { | ||
| message.value.keys | ||
| .groupBy(_.containingOneof) |
There was a problem hiding this comment.
Since I want this logic turned on by default, I'd like to avoid allocation that creates a second copy proportional to each PMessage. Instead, can we maintain a mutable set in fromJsonToPMessage of all the seen oneof descriptors, and check in L624 whether a duplicate hit occurred and throw as soon as that happened?
There was a problem hiding this comment.
I (think) I made this change as you've described in the latest commit.
* fail on overlapping oneof fields by default * update names * use 'field' instead of 'key' for consistency * use 'isIgnoringOverlappingOneofFields' and 'ignoringOverlappingOneofFields' for consistency
|
Thank you! |
|
@thesamet what are your thoughts around releasing this? I guess it has to be a major release since it could break existing clients. |
|
Released 0.11.1. Add a note here regarding the breakage: https://github.com/scalapb/scalapb-json4s/releases/tag/v0.11.1 |
The exception looks like:
Here's a first pass. I'm definitely open to feedback re: different approaches and style.
The java-parser was already throwing an
InvalidProtocolBufferExceptionfor too many keys (and this is now part of the unit tests).I wasn't thrilled with the placement of
validateOneofs(), but it seemed better and easier than working with the generated code to modify theReads