david-mollitor-db opened a new pull request, #58743:
URL: https://github.com/apache/spark/pull/58743
### What changes were proposed in this pull request?
Add two overloads to `JavaUtils` that throw the failure message verbatim:
```java
public static void checkArgument(boolean check, String msg) { ... }
public static void checkState(boolean check, String msg) { ... }
```
Unlike the existing `checkArgument`/`checkState(boolean, String, Object...)`
methods, these do
**not** route the message through `String.format`. The existing varargs
methods (and their
"keep synced with `CommandBuilderUtils`" bodies) are left unchanged.
### Why are the changes needed?
`checkArgument`/`checkState` build the message with `String.format(msg,
args)`, so `msg` is a
format string. When a caller has no values to interpolate and passes a plain
or
already-concatenated message, that message is still treated as a format
template: a stray `%` in
it (a percent-encoded token, a Windows path such as `%TEMP%`, a SQL `LIKE`
pattern, a number like
`"50%"`) makes `String.format` throw an `IllegalFormatException` that
replaces the intended
`IllegalArgumentException` / `IllegalStateException` and hides the real
reason the check failed.
The new overloads make the no-argument case format-safe: the message is used
exactly as given, and
no empty varargs array is allocated. Existing two-argument call sites bind
to these overloads
automatically on recompile — a two-argument call is never ambiguous with the
varargs form — so they
gain this safety with no source changes. Callers that do have values to
interpolate keep using the
varargs overload and should pass the values as arguments (`"bad key: %s",
key`) rather than
concatenating them.
### Does this PR introduce _any_ user-facing change?
No. For the plain messages used by existing callers (none of which contain a
`%`), the exception
type and message are unchanged; the overloads only change behavior for a
message that contains a
`%` and no arguments, which previously threw a formatting error.
### How was this patch tested?
Added `testCheckArgumentUsesMessageVerbatim` /
`testCheckStateUsesMessageVerbatim` to
`JavaUtilsSuite`, asserting that a message containing `%` (which would
otherwise make
`String.format` throw) is preserved verbatim in the thrown exception.
`JavaUtilsSuite` passes (4
tests). `common-utils-java`, `network-common`, `network-shuffle`, and
`kvstore` compile cleanly
(confirming existing two-argument callers rebind without ambiguity), and
checkstyle is clean.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Opus 4.8
This pull request and its description were written by Isaac.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
