[MINOR][CORE] Document the JavaUtils.checkArgument format-string contract and fix a concatenating caller - #58740
Open
david-mollitor-db wants to merge 1 commit into
Conversation
…ract and fix a concatenating caller
`JavaUtils.checkArgument`/`checkState` build the failure message with
`String.format(msg, args)`, so `msg` is a format string and callers are meant
to pass runtime values as trailing `args` (e.g. `"bad key: %s", key`). The
JavaDoc did not say this, and at least one caller concatenated the value into
the message instead:
JavaUtils.checkArgument(appShuffleInfo != null,
"application " + appId + " is not registered or NM was restarted.");
Concatenating is unsafe: on the failure path `String.format` treats the
already-interpolated text as the format string, so a stray `%` in the value
(a percent-encoded token, a Windows path like `%TEMP%`, a `LIKE` pattern, a
number such as "50%") makes it throw an `IllegalFormatException` that replaces
the intended `IllegalArgumentException` and hides the real reason the check
failed. It also builds the message eagerly on every call, including the common
success path.
This change:
- Expands the `checkArgument`/`checkState` JavaDoc to describe the
`String.format` contract and warn against interpolating values into `msg`,
with the correct `%s` idiom.
- Converts the `RemoteBlockPushResolver.validateAndGetAppShuffleInfo` caller
to pass `appId` as a format argument. The produced message is unchanged.
Co-authored-by: Isaac <no-reply@databricks.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
JavaUtils.checkArgumentandJavaUtils.checkStatebuild their failure message withString.format(msg, args), somsgis a format string and callers are meant to pass runtimevalues as trailing
args(e.g.checkArgument(cond, "bad key: %s", key)). This was not documented,and one caller concatenated the value into the message instead. This PR:
Expands the
checkArgument/checkStateJavaDoc to describe theString.formatcontract andwarn against interpolating values into
msg, showing the correct%sidiom.Fixes the concatenating caller in
RemoteBlockPushResolver.validateAndGetAppShuffleInfo:Why are the changes needed?
Concatenating a value into
msgis unsafe. On the failure pathString.formattreats thealready-interpolated text as the format string, so a stray
%in the value (a percent-encodedtoken, a Windows path such as
%TEMP%, a SQLLIKEpattern, a number like"50%") makes it throwan
IllegalFormatExceptionthat replaces the intendedIllegalArgumentExceptionand hides the realreason the check failed. It also builds the message eagerly on every call, including the common
success path where it is immediately discarded. Documenting the contract and correcting the one
concatenating caller prevents this.
Does this PR introduce any user-facing change?
No. The message produced by the corrected caller is identical (
%sis substituted withappId),and the remaining changes are documentation only.
How was this patch tested?
Existing
RemoteBlockPushResolverSuitepasses (46 tests; the two tests that assert the"application ... is not registered" message still hold, since the produced text is unchanged).
Checkstyle is clean on both
common-utils-javaandnetwork-shuffle.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.