Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add field name to error when failing to parse a field of type Message #402

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add field name to error when failing to parse a field of type Message #402

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 14 additions & 6 deletions src/main/scala/scalapb/json4s/JsonFormat.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -651,7 +651,7 @@ class Parser private (config: Parser.ParserConfig) {

PMessage(valueMapBuilder.result())
case _ =>
throw new JsonFormatException(s"Expected an object, found ${value}")
throw new JsonFormatException(s"Expected an object for ${cmp.scalaDescriptor.fullName}, found ${value}")
}
}
}
Expand Down Expand Up @@ -723,11 +723,19 @@ class Parser private (config: Parser.ParserConfig) {
res.fold[PValue](PEmpty)(PEnum.apply)
}
case ScalaType.Message(md) =>
fromJsonToPMessage(
containerCompanion.messageCompanionForFieldNumber(fd.number),
value,
false
)
try {
fromJsonToPMessage(
containerCompanion.messageCompanionForFieldNumber(fd.number),
value,
false
)
} catch {
case ex: JsonFormatException =>
throw new JsonFormatException(
s"Failed parsing field ${fd.name}: ${ex.getMessage}",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With nested messages, wouldn't this lead to something that looks like:
"Failed parsing field x: failed parsing field y: failed parsing field z: reason"? Can this be changed somehow so the full path appears once?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @thesamet , sorry the person who opened that PR left my company so didn't follow up.
Indeed for this input """{"optMessage": { "optMessage": 39} }""" the error message would be

Failed parsing field opt_message: Failed parsing field opt_message: Expected an object for jsontest.MyTest, found JInt(39)

I don't know well this code base and I'm not seeing any trivial way to change it to a full path instead. I guess that might require subtyping JsonFormatException to accumulate a path for failure in nested objects, and I have no idea if adding that kind of complexity would be worth it or acceptable for the project.

Current implementation may not be the cleanest message, but it's simple and at least it's giving a helpful message, while without this implementation the message would be instead

Expected an object for jsontest.MyTest, found JInt(39)

Leaving the user clueless about where this is happening, especially for another type of error like "expected string and got number".

ex
)
}
case st =>
JsonFormat.parsePrimitive(
fd.protoType,
Expand Down
11 changes: 11 additions & 0 deletions src/test/scala/scalapb/json4s/JsonFormatSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -456,6 +456,17 @@ class JsonFormatSpec
)
}

"TestProto" should "fail to parse a field of type Message when it's not a json object" in {
val jsonFormatException = intercept[JsonFormatException] {
new Parser().fromJsonString[MyTest](
"""{"optMessage": 39}"""
)
}
jsonFormatException.getMessage must be (
"Failed parsing field opt_message: Expected an object for jsontest.MyTest, found JInt(39)"
)
}

"TestProto" should "parse original field names" in {
new Parser().fromJsonString[MyTest]("""{"opt_enum":1}""") must be(
MyTest(optEnum = Some(MyEnum.V1))
Expand Down