Skip to content

[MINOR][CORE] Add verbatim-message overloads for JavaUtils.checkArgument and checkState - #58743

Open
david-mollitor-db wants to merge 1 commit into
apache:masterfrom
david-mollitor-db:javautils-checkargument-overload
Open

[MINOR][CORE] Add verbatim-message overloads for JavaUtils.checkArgument and checkState#58743
david-mollitor-db wants to merge 1 commit into
apache:masterfrom
david-mollitor-db:javautils-checkargument-overload

Conversation

@david-mollitor-db

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

Add two overloads to JavaUtils that throw the failure message verbatim:

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.

…ent and checkState

`JavaUtils.checkArgument`/`checkState` build the failure 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 like `%TEMP%`, a `LIKE` pattern, a number
such as "50%") makes `String.format` throw an `IllegalFormatException` that
replaces the intended `IllegalArgumentException`/`IllegalStateException` and
hides the real reason the check failed.

Add `checkArgument(boolean, String)` and `checkState(boolean, String)` overloads
that throw the message verbatim, without going through `String.format`. Existing
two-argument call sites bind to these overloads automatically on recompile (a
two-argument call is never ambiguous with the varargs form), so plain-message
checks become format-safe and skip the empty varargs array. 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.

The existing varargs methods are unchanged, so the "keep synced with
CommandBuilderUtils" clones are untouched.

Co-authored-by: Isaac <no-reply@databricks.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant