[LIVY-1083] Coerce batch/session Int fields sent as JSON strings - #558
soumyadeeplogin wants to merge 2 commits into
Conversation
driverCores, executorCores and numExecutors are Option[Int], but Scala
type erasure means jackson-module-scala stores a JSON string such as "4"
verbatim as Some("4") without error. The bad value only surfaces later
as a ClassCastException during unboxing in BatchSession/InteractiveSession
app creation, which the servlet returns to callers as an opaque HTTP 500.
Annotate the three fields on CreateBatchRequest and CreateInteractiveRequest
with @JsonDeserialize(contentAs = classOf[Integer]) so Jackson coerces
numeric strings to Int and rejects truly non-numeric strings with a 400
instead of a 500.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Filed LIVY-1083 for this change. |
…RequestSpec CI failed on a 103-character line introduced by the JSON-string coercion test. Split the literal across two concatenated strings to fit under the 100-character limit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #558 +/- ##
============================================
+ Coverage 68.68% 73.10% +4.41%
- Complexity 1218 1295 +77
============================================
Files 106 106
Lines 6815 6874 +59
Branches 836 855 +19
============================================
+ Hits 4681 5025 +344
+ Misses 1666 1344 -322
- Partials 468 505 +37 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Thanks for your contribution @soumyadeeplogin ! I am still testing/reviewing, but I do agree it is better to send HTTP 4xx instead of 5xx on malformed client input. However the downside is some existing clients may already check for HTTP 5xx in this case, so in this rare case it would be an API change. This may not be a big issue (especially prior to the 1.0 release), I just wanted to point it out. |
|
Thanks for the careful look, @gyogal! Good news — this doesn't actually change the status code either way. Malformed/non-coercible input (e.g.
error {
case e: JsonParseException => BadRequest(e.getMessage)
case e: UnrecognizedPropertyException => BadRequest(e.getMessage)
case e: JsonMappingException => BadRequest(e.getMessage)
case e =>
SessionServlet.error("internal error", e)
InternalServerError(e.toString)
}Jackson's |
What changes were proposed in this pull request?
CreateBatchRequestandCreateInteractiveRequestdeclaredriverCores,executorCoresandnumExecutorsasOption[Int]. Because of Scala type erasure,Option[Int]erases toOption[Object], sojackson-module-scalacannot tell the element type isInteger. When a client sends one of these fields as a JSON string (e.g."4"instead of4), Jackson stores it verbatim asSome("4")without error. The bad value only surfaces later as aClassCastExceptionwhen the code unboxes it toIntwhile building the Spark submit command, which the servlet returns to callers as an opaque HTTP 500.This PR annotates the three fields on both request classes with
@JsonDeserialize(contentAs = classOf[java.lang.Integer])so Jackson coerces numeric strings toIntduring deserialization, and rejects truly non-numeric strings with a client-facing 400 (JsonMappingException) instead of a 500 later during app creation.Why are the changes needed?
To fail fast with a clear 400 at the API boundary when a client sends a malformed numeric field, instead of an opaque 500 deep in Spark app submission logic.
Does this PR introduce any user-facing change?
Yes, but only for previously-broken input.
POST /batchesand interactive session creation now accept JSON string values fordriverCores/executorCores/numExecutors(e.g."4") by coercing them to the equivalent int, and return a 400 instead of a 500 for genuinely non-numeric strings (e.g."notanumber","4.5").How was this patch tested?
Added unit tests to
CreateBatchRequestSpecandCreateInteractiveRequestSpeccovering: numeric-as-JSON-number (unchanged behavior), numeric-as-JSON-string (coerced), non-numeric string (400/JsonMappingException), empty string (None), whitespace-padded numeric string (coerced), and fractional numeric string (rejected).Ran
mvn -pl server -am test -DwildcardSuites=org.apache.livy.server.batch.CreateBatchRequestSpec,org.apache.livy.server.interactive.CreateInteractiveRequestSpec; all 14 tests pass.Was this patch authored or co-authored using generative AI tooling?
Yes, this patch was co-authored using Claude Code (Anthropic).