Remove the Guava dependency - #352
Open
royteeuwen wants to merge 11 commits into
Open
Conversation
Additive only: Guava is still on the classpath and no existing call site changed. New MediaType replaces com.google.common.net.MediaType. Its toString() is a wire value, so the rendering is pinned by tests against values captured from Guava 33.6.0-jre. Parameters other than charset are preserved because a recorded multipart request round-trips its boundary back onto the wire. Maps.copyOf and Maps.toUnmodifiableMap preserve insertion order. Map.copyOf is not usable here: it randomises iteration order once per JVM run, and header and query maps are rendered in iteration order. The collector also keeps Guava's IllegalArgumentException on duplicate keys, reachable from a repeated form field. HttpHeaders constants keep canonical casing rather than reusing Netty's lower-case HttpHeaderNames, which would break HttpMessage.hasContent().
TypeToken.of(getClass()).getRawType() is just getClass(), verified to be reference-identical including for anonymous subclasses. Failover collects its statuses instead of using Set.of, which would reject the repeated status in failover(file, 400, 400) where ImmutableSet.copyOf silently de-duplicated. nullToEmpty around String.replaceFirst was dead - replaceFirst never returns null. Removed @subscribe from MocoMonitor. Moco has had no EventBus since 8fa72e1 (2013), so this is inert internally. It is not inert for consumers: Guava's EventBus resolves @subscribe across interfaces, so anyone registering a MocoMonitor on their own bus had these four methods wired up, and after this they are silently never registered. Undocumented and incoherent as an extension point - onMessageArrived and onUnexpectedMessage both subscribe to Request, so one post fires both - but it needs a release note. StartTask logs elapsed milliseconds instead of Guava's auto-scaled Stopwatch rendering.
Needed to stay a real type rather than inlined string building, because DefaultHttpMessage and RestBaseSetting expose protected toStringHelper() and subclasses extend the chain through super.toStringHelper().add(...). Rendering verified byte-identical to Guava for empty, null, omitNullValues, String[], int[], nested arrays, maps and chained cases.
checkNotNull becomes Objects.requireNonNull, which throws the same NullPointerException with the same message. Only the two-argument form was ever used, so no template/varargs overload is needed. checkArgument and isNullOrEmpty move to the existing moco util classes, which already followed this shim pattern.
Header constants keep Guava's canonical casing, pinned by a test. Netty's HttpHeaderNames are lower-case and would have broken the exact-key lookups in getHeader and HttpMessage.hasContent(). MediaType.toString() is the Content-Type on the wire, so it was differentially tested against Guava 33.6.0-jre across the constants, parse round-trips, type/subtype/charset accessors and withCharset. The only divergence is that trailing whitespace is now tolerated rather than rejected, which is documented and cannot introduce a throw where Guava succeeded. FileContentType drops two toJavaUtil() calls now that charset() returns java.util.Optional. HttpDumpers replaces Guava's is() with type and subtype equality, which is what it reduced to for a parameterless FORM_DATA.
ByteStreams/CharStreams become InputStream.readAllBytes(); both the old and new forms use the default charset. Files.asCharSink(APPEND) becomes Files.writeString with CREATE/WRITE/APPEND. CharSource.wrap().readFirstLine() becomes BufferedReader.readLine(), which also yields null for empty input. Closeables.closeQuietly gives way to try-with-resources. Both call sites wrap a ByteArrayInputStream from MessageContent.toInputStream(), whose close() is a no-op, so swallowing the close failure was never doing anything. util.Resources keeps Guava's lookup order - thread context class loader in preference to this class's own - and throws rather than returning null. That ordering matters because Moco runs embedded in other projects' test frameworks and from the shrunk standalone jar.
…alents
Splitter needed care. String.split takes a regex, so Splitter.on('.') had to
become split("\\.", -1) rather than split("."), which matches any character. The
-1 limit is required everywhere because String.split drops trailing empty
strings while Guava's Splitter keeps them, and SSE parsing delimits events on
blank lines.
Multimaps become plain Map-of-collection fields. Semantics differ per site and
had to be preserved: ActionMonitor keeps List semantics so a repeated query
parameter is not collapsed, while ChannelSessionGroup and WatcherService keep
Set semantics. WatcherService also prunes a key when its last value goes,
because Guava did that and both the containsKey check and the isEmpty check that
stops the service depend on it. ChannelSessionGroup moves to ConcurrentHashMap
with newKeySet, which is safer than the previous synchronized view that was
iterated outside any lock.
Maps.transformEntries was a lazy view immediately copied, so the laziness was
unused; the anonymous EntryTransformer collapses into a stream collect.
HttpDumpers loses a pointless intermediate collect, and String.join replaces
Joiner - the only difference being that a null element now renders as "null"
instead of raising NullPointerException on an error path.
This is the breaking part of the migration. Public signatures widen from
ImmutableMap/ImmutableList to Map/List, which is binary-incompatible - the erased
return type changes - and source-breaking for anyone implementing HttpMessage,
HttpRequest, Content or SettingFetcher, or assigning getHeaders() to an
ImmutableMap variable.
Map.of and Map.copyOf are deliberately avoided on header, query, form, cookie and
template-variable paths. They randomise iteration order once per JVM run, and
those maps are rendered in iteration order. This was not theoretical: converting
HttpRequestDumperTest.should_dump_queries to Map.of made it pass or fail
depending on the run. It now builds a LinkedHashMap, and a companion test asserts
that header and query order survives. The suite was run four times in fresh JVMs
to confirm nothing else is salt-dependent.
Static lookup tables that are only ever read by key keep Map.of / Map.ofEntries,
since order genuinely does not matter there.
List.copyOf takes a Collection where ImmutableList.copyOf accepted any Iterable,
so CollectionHandler streams instead, and mount() uses List.of on its varargs
array.
Test helpers take List<Map.Entry<String, String>> rather than a Map, because some
tests deliberately send a repeated header name - of("foo", "bar", "foo", "bar2") -
which Map.of rejects.
Dropped from all four modules and from the version catalog. The standalone jar shrinks by 214KB and the uber jar by about 3MB. Two things surfaced only once the dependency was actually gone. org.jspecify:jspecify was reaching moco-core transitively through Guava, and two @nonnull annotations depended on it. It is now declared explicitly as compileOnly, so it stays out of the published POM and the shrunk jar. The proguard rule -dontwarn com.google.** was also covering json-path's optional Gson backends, which have nothing to do with Guava. Deleting it outright un-suppressed 75 unresolved references, so it is replaced with narrow rules for com.google.gson and org.jspecify that say what they are for. Added a checkNoGuava task wired into check, verified to fail when an import is reintroduced, so this cannot regress one file at a time.
Maps.copyOf and Maps.toUnmodifiableMap shared their names with Map.copyOf and Collectors.toUnmodifiableMap, which behave the opposite way: the JDK versions randomise iteration order once per JVM run. Same-named helpers invite exactly the substitution that must not happen, and that substitution is what made HttpRequestDumperTest flaky earlier in this branch. They are now orderedCopyOf and toOrderedMap, with the reasoning in the javadoc. Audited every method on the util classes this migration added or extended: all of them have production call sites, so nothing was left behind as dead code.
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.
Removes the Guava dependency from all four modules. Rationale, sources and the open questions are in #351 — please read that first, since this PR is only worth reviewing if you want the change at all.
Short version of the why: Guava types appear in Moco's public signatures, and on AEM LTS / AEMaaCS
com.google.common.*is no longer available as a public package. That already forced a workaround in a downstream Moco integration (wttech/aem-stubs#64), where a Groovy static-star import ofMocohad to be replaced by a reflection loop because the static-star import triggers cross-bundle resolution of the Guava types in those signatures. Shading would not have helped; only taking Guava out of the API does.Timing: 2.0.0 is unreleased and already breaking (socket API removal, Java 17 baseline), so this costs nothing extra in semver terms. After 2.0.0 ships it would need 3.0.0.
Result
com.google.commonreference anywhere in the sources; no Guava in any module's dependency tree.:moco-runner:proguardCheckpasses.Reviewing
11 commits, one per step, each compiling and passing the full suite on its own — so it is much easier to read commit by commit than as a single diff.
If the API change is the sticking point, it splits cleanly:
added guava-free util scaffolding…replaced remaining guava Iterables.isEmpty) are internals only, no public signature changes.replaced guava Immutable collections,removed the guava dependency,renamed the ordered map helpers) contain the API widening, the dependency removal, the docs and the release notes.The first group stands on its own if you would rather not take the second. Happy to reopen this as two PRs on that boundary.
Worth knowing before you review
MediaTypebecomescom.github.dreamhead.moco.util.MediaType. ItstoString()is theContent-Typeon the wire, so it was differential-tested against Guava 33.6.0-jre and matches in 34 of 35 cases; the exception is that trailing whitespace is tolerated rather than rejected, which is documented and cannot introduce a throw where Guava succeeded. Non-charset parameters are preserved because a recorded multipart request round-trips itsboundarythroughparse.Map.of/Map.copyOfrandomise iteration order once per JVM, and headers and queries are rendered in that order — convertingHttpRequestDumperTesttoMap.ofgenuinely made it pass or fail per run. Those paths useMaps.orderedCopyOf/Maps.toOrderedMap, named deliberately unlike the JDK methods they must not be swapped for. There is a test pinning header and query order, and the suite was run four times in fresh JVMs.Content-Length, not Netty's lower-casecontent-length), because lookups are exact-key andHttpMessage.hasContent()depends on one. Pinned by a test.org.jspecify:jspecifywas arriving only transitively via Guava and two@NonNullannotations need it; it is now declared explicitly ascompileOnly. Open to dropping the annotations instead — see question 3 in Remove the Guava dependency in 2.0.0 #351.-dontwarn com.google.**inproguard.prowas also masking json-path's optional Gson backends. Removing it un-suppressed 75 unresolved references and broke the ProGuard gate, so it is replaced with narrow rules forcom.google.gsonandorg.jspecify.checkNoGuavatask is wired intocheck, verified to fail when an import is reintroduced.Breaking changes are written up in
moco-doc/ReleaseNotes.mdunder a new 2.0.0 section, including theMocoMonitor@Subscriberemoval, which fails silently for anyone registering aMocoMonitoron their own GuavaEventBus.