Fixes for guava version bump#2943
Conversation
Newer Guava (pulled in transitively via resource-server 1.5.x) annotates com.google.common.base.Function.apply() with @CheckReturnValue, so ErrorProne now flags the two call sites that discard the return: uPortal-rendering/src/main/java/org/apereo/portal/portlet/container/ cache/LimitingTeeWriter.java:86 cache/LimitingTeeOutputStream.java:84 The callback's Function<T,?> return was always intentionally discarded (the sole caller returns null), so the fix is to name the captured result 'unused' — the convention ErrorProne recognizes as "intentional discard" — with @SuppressWarnings("unused") to also silence the compiler's own unused-variable lint. Master CI passes today because its resource-server 1.3.1 pin pulls an older Guava without the annotation; the check triggers on branches that have bumped to resource-server 1.5.x+ (e.g. uPortal-Project#2915 'Bootstrap / jQuery Update & Fluid Infusion Removal', which fails all 5 CI jobs on this exact error). Landing the fix on master lets that branch rebase and go green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@Naenyn — this is the ErrorProne fix for the Would you mind giving this a quick look when you have a moment? |
|
Swapped out the Validated on master baseline: Force-pushed |
…2945) Replaces the @SuppressWarnings/unused workaround just landed in #2943 with the cleaner design @Naenyn proposed: the callback's return was always discarded (the `?` wildcard on Function was a tell), so java.util.function.Consumer<T> is the correct type. With Consumer, .accept() returns void and ErrorProne's CheckReturnValue check doesn't apply at all — no suppression needed. Adopts @Naenyn's approach verbatim from his remove_fluid-branch work (Naenyn/uPortal@f2e3a459). His force-push came in after #2943 was already squashed and merged, so this PR lands the same set of changes as a follow-up. Scope (6 files, +23/−48): - LimitingTeeWriter.java : Function<T,?> → Consumer<T>, .apply → .accept - LimitingTeeOutputStream.java : same pattern - CachingPortletOutputHandler.java (sole caller): anonymous Function<> classes → concise `input -> clearCachedWriter()` and `input -> clearCachedStream()` lambdas; drop unused import - LimitingTeeWriterTest.java, LimitingTeeOutputStreamTest.java : matching Function-to-lambda updates; drop unused Function import - PortalRawEventsAggregatorImplTest.java : different case — the Function here is part of the code-under-test's own API and can't be changed, so use `Boolean ignored = …` to capture the return (same shape @Naenyn used; aligns with the ErrorProne convention) Verified locally: - :uPortal-rendering:compileJava → BUILD SUCCESSFUL - :uPortal-webapp:compileTestJava → BUILD SUCCESSFUL (master baseline; the 1.5.2-Guava trigger is already covered by @Naenyn's remove_fluid branch building clean with these same hunks.) Co-authored-by: Bill Smith <wsmith@unicon.net> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Adopts @Naenyn's direct-to-the-point fix for the ErrorProne
CheckReturnValueviolations that surface when resource-server's newer Guava transitively lands in the classpath. Cherry-picked verbatim from commitf2e3a459on hisremove_fluidbranch so he keeps authorship.Why this approach beats the
@SuppressWarningsworkaroundMy earlier version of this PR used a
@SuppressWarnings("unused") Object unused = callback.apply(this);pattern to silence ErrorProne while keeping theFunction<T,?>signature. Bill's approach is strictly better: the callback's return value was never used (the?wildcard was a tell), sojava.util.function.Consumer<T>is the correct type. No suppression needed; no weird "unused" variable convention; the ErrorProne violation goes away because there's no return value to check.Scope (6 files, +23/−48)
Source:
LimitingTeeWriter.java—Function<LimitingTeeWriter, ?>→Consumer<LimitingTeeWriter>,.apply()→.accept(), import swapLimitingTeeOutputStream.java— same patternCachingPortletOutputHandler.java(sole caller) — anonymousnew Function<>() { ... return null; }→input -> clearCachedWriter()lambdasTests:
LimitingTeeWriterTest.java,LimitingTeeOutputStreamTest.java— matching Function→Consumer updates with lambdas, dropcom.google.common.base.FunctionimportPortalRawEventsAggregatorImplTest.java— different pattern here: theFunctioncomes from the code-under-test's API, not our types, so it can't be changed to Consumer. Bill uses aBoolean ignored = …capture to satisfyCheckReturnValue. Same shape as the@SuppressWarningspattern but localized to the specific call.Validation
./gradlew :uPortal-rendering:compileJavaon master baseline — BUILD SUCCESSFUL./gradlew :uPortal-webapp:compileTestJavaon master baseline — BUILD SUCCESSFULremove_fluidbranch (which hasresourceServerVersion=1.5.2and therefore triggers the underlying Guava@CheckReturnValuecheck) already builds clean with these exact changes — so we know the fix addresses the real failure mode surfaced by Bootstrap / jQuery Update & Fluid Infusion Removal #2915.Merge order — no change
uPortal-utils-coreso Util.java compiles when resource-server 1.5.2 comes alongresourceServerVersion 1.3.1 → 1.5.2bumpcc @Naenyn — this PR now contains your commit verbatim. If you'd prefer a different attribution style or want me to close and let your own PR carry it instead, just say the word.