SOLR-18302: dependency-analysis (DAGP) — reporting + build hygiene by serhiy-bzhezytskyy · Pull Request #4612 · apache/solr

serhiy-bzhezytskyy · 2026-07-05T11:05:03Z

Completes the dependency-analysis follow-up deferred from the Gradle 9 upgrade (SOLR-18289), which dropped the Gradle-9-incompatible ca.cutterslade.analyze plugin.

Adds the Dependency Analysis Gradle Plugin (com.autonomousapps build-health) as an opt-in analyzeDependencies task — it reports advice, doesn't fail the build, since the api-vs-implementation calls need human judgement rather than blanket enforcement.

Changes

Add com.autonomousapps.build-health; config in gradle/validation/dependency-analyze.gradle.kts, wired to an opt-in analyzeDependencies task (not check).
Apply the safe advice: remove genuinely-unused dependency declarations, a few test-scope corrections, kafka-clients test jar re-added as testRuntimeOnly.
Remove redundant project() declarations that solr-core already provides via api (24 across 17 modules) — cutterslade-era "state the obvious" leftovers.
jwt-auth keeps jackson-databind as testImplementation (DAGP advises testRuntimeOnly, but tests compile against mock-oauth2-server's @JsonDeserialize — demoting breaks under -Werror).
Force kotlin-metadata-jvm to Solr's Kotlin so DAGP can analyze :solr:ui (Update kotlin-metadata-jvm to 2.3+ for Kotlin 2.4 support autonomousapps/dependency-analysis-gradle-plugin#1661; temporary).

api-scope promotions DAGP also suggests were considered and set aside — they need case-by-case judgement rather than blanket application (per dev@ discussion).

Testing

gradlew check -x test and full gradlew test pass.

AI assistance disclosure

Per AGENTS.md / how-to-contribute.adoc: prepared with an AI coding agent and reviewed by me for correctness, scope, and alignment with Solr's build conventions.

Completes the dependency-analysis follow-up deferred from the Gradle 9 upgrade (SOLR-18289), which dropped the Gradle-9-incompatible ca.cutterslade.analyze plugin. Adds the Dependency Analysis Gradle Plugin (com.autonomousapps build-health) configured to REPORT (warn), not fail — its advice is guidance for human review, not enforcement. Applies only the uncontroversial advice: - removal of genuinely-unused dependency declarations - a few test-scope corrections (implementation -> testImplementation) Deliberately NOT included here (left for separate case-by-case review): - implementation -> api promotions (DAGP is aggressive here; per dev@ these need human judgement, as liberal api is viral/transitive) Also forces kotlin-metadata-jvm to match Solr's Kotlin so DAGP can analyze the :solr:ui (Compose/KMP) module. Lockfiles regenerated.

dsmiley

Thanks. I suppose for me to truly appreciate it, I'll need to run it and even intentionally make sloppy dependency choices and see how the plugin helps.

dsmiley · 2026-07-07T00:27:21Z

@@ -0,0 +1,7 @@
+title: Add dependency-analysis (build-health) reporting to the build, and apply its uncontroversial advice (unused-dependency removals and test-scope corrections)
+type: changed


Our documentation in 2 places define the meaning of the options for "type" -- and this is undoubtedly a "other" category change and not "changed".

Fixed — changed to type: other. Thanks.

dsmiley · 2026-07-07T00:27:23Z

+- name: PR#4612
+  url: https://github.com/apache/solr/pull/4612


When we have JIRA issues, we favor the JIRA issue instead of PR.

Done — created SOLR-18302 for this work and switched the link to the JIRA. (The PRs were previously mis-titled with SOLR-18296, which is actually the google-java-format issue; corrected now.)

oooh, ok. You didn't need to file a JIRA issue over this matter but now that you did, it's fine.

dsmiley · 2026-07-07T00:36:59Z

  implementation libs.carrotsearch.hppc
-
-  implementation(libs.benmanes.caffeine) {transitive = false}
+  api(libs.benmanes.caffeine) {transitive = false}


a dubious one IMO

You're right that this doesn't belong in this PR — I've reverted it (and the other external implementation → api changes) back to implementation here. This PR is now just the plugin wiring plus safe/mechanical advice; the scope promotions live in the draft PRs (#4613 for inter-module project(), #4614 for external libs) for exactly the case-by-case discussion you asked for.

One correction to my own earlier framing, in case it's useful: I checked these against the plugin's ABI dump, and caffeine genuinely is exposed in solr-core's public API today — CaffeineCache implements com.github.benmanes.caffeine.cache.RemovalListener, and onRemoval(..., RemovalCause) is a public method. So it isn't the tool being over-eager; the type really does leak through the public signature regardless of the declaration. The real question is whether we want solr-core's public API to expose caffeine, or whether that's accidental leakage worth encapsulating — which is the judgment call, and why these are split out rather than applied here.

I'm not surprised that we have a number of dependencies like this that technically are exposed in our public API. But I have some decades here to judge that some of our dependencies are conceptually an internal / niche use and not widespread enough to warrant the "api" transitive implications.

Where I didn't comment, I implicitly concurred with your choice.

Yeah, makes sense. These are in #4614 if it's ever worth revisiting which deserve an api contract. Thanks.

dsmiley · 2026-07-07T00:37:31Z

    exclude group: "org.apache.yetus", module: "audience-annotations"
  })
-  implementation(libs.apache.zookeeper.jute) {
+  api(libs.apache.zookeeper.jute) {


Oh? I thought this was only used for embedded ZK but maybe I'm wrong

Reverted to implementation here (moved to #4614). For the record though, this one is heavily exposed: ~82 public methods across core declare throws org.apache.zookeeper.KeeperException, so ZooKeeper is genuinely part of the public API surface today, not just embedded-ZK internals. Whether that's desirable is #4614 discussion.

My comment is respect to "jute", not ZK.

Ah, my bad — misread that as ZK. Checked jute: the jar's a bit odd, it also ships org.apache.zookeeper.data.*, and core does expose Stat (getStat() returns it). So technically leaked, but agree it's niche. Left as implementation here anyway.

dsmiley · 2026-07-07T00:37:45Z

  runtimeOnly libs.dropwizard.metrics.core

-  implementation(libs.jayway.jsonpath, {
+  api(libs.jayway.jsonpath, {


woah?! why?

Reverted to implementation here. The ABI reason it was flagged: public static Configuration jsonPathConfiguration() returns a com.jayway.jsonpath type — a real (if probably unintended) public exposure, so a good candidate for encapsulating rather than promoting. Parked in #4614.

dsmiley · 2026-07-07T00:38:19Z

  api libs.opentelemetry.api
  implementation libs.opentelemetry.context
-  implementation(libs.opentelemetry.exporter.prometheus) {
+  api(libs.opentelemetry.exporter.prometheus) {


ehh... not sure why a solr-core user requires this?

Agreed it's surprising — reverted here. It's flagged because FilterablePrometheusMetricReader extends io.opentelemetry.exporter.prometheus.PrometheusMetricReader (public class, public supertype). So the exposure is real, but "should a core consumer see it" is exactly the right question — #4614 territory, not this PR.

dsmiley · 2026-07-07T01:01:51Z

I was hoping the new mechanism would be able to remove these super-obvious testImplementation declarations across our modules. One or two might even have a snide remark comment. of course the tests depend on the module they are testing!! It was a deficiency of the previous system that we needed to state the obvious.

Agreed — and done. I removed testImplementation project(':solr:solrj-jetty') (the module test-depending on itself; the test source set already sees its own main classes) and the duplicate testImplementation project(':solr:solrj') (already api project(':solr:solrj')). Kept the configuration: 'testArtifacts' one since that provides solrj's test classes. Verified test compiles and all 59 tests pass. (In fairness to the tool: it flags these via transitive-usage analysis rather than a redundancy check, and solrj-jetty was the only genuinely-redundant case repo-wide — but it's exactly the kind of state-the-obvious declaration you meant, so glad to drop it.)

I'd love to see all self testing testImplementation declarations removed; my comment is meant categorically, just just in this instance. If the tool isn't going to tell us about them all, hopefully at least it won't complain. The previous tool complained, which made me hate it. Any way, I can tend to this if you don't.

Done — removed the self-referential testImplementation project(':self') in api, solrj, solrj-streaming, solrj-zookeeper, and core. Two still had the old // cutterslade requires we state the obvious comment, so those workarounds are gone with it.

dsmiley · 2026-07-07T01:05:48Z

not critical but there was some project lazy-consensus on embracing kotlin for the gradle build. When I recently created a new build file, I found the syntax helpful to avoid problems due to groovy's untyped nature.

Good to know, thanks. The build is currently all Groovy (solr/ui/build.gradle.kts is the only Kotlin build file, and that's because the UI is a KMP module), so I kept this consistent with the other gradle/validation/*.gradle scripts for now. Happy to write it in Kotlin DSL instead if that's the direction you'd prefer for new build files — just say the word.

yes, kotlin please

Done — converted it to .gradle.kts. buildHealth still runs and the suppressions all work.

…oss-dc-manager tests) DAGP flagged the kafka-clients test-classifier jar as an unused dependency, so the earlier commit removed it. That was a false positive: no test compiles against it, but EmbeddedKafkaCluster (from the kafka-streams test jar) loads org.apache.kafka.test.TestCondition from it at runtime, so the *IntegrationTest suites failed with ClassNotFoundException on CI. Re-add it as testRuntimeOnly (runtime-only is the accurate scope, vs the testImplementation it originally had) with a comment, and restore its license sha1. A concrete example of why DAGP runs report-not-fail and its advice needs case-by-case judgement.

serhiy-bzhezytskyy · 2026-07-07T05:50:18Z

Pushed a fix for the cross-dc-manager CI failure — and it's a nice illustration of why this PR runs DAGP in report-not-fail mode.

DAGP flagged the kafka-clients test-classifier jar as an unused dependency (nothing compiles against it), so I'd removed it. But it's needed at runtime: EmbeddedKafkaCluster (from the kafka-streams test jar) loads org.apache.kafka.test.TestCondition from it, so the *IntegrationTest suites failed with ClassNotFoundException. Re-added as testRuntimeOnly (the accurate scope — runtime, not compile) with a comment.

Reproduced locally and both previously-failing suites (ZkConfigIntegrationTest, SolrAndKafkaIntegrationTest) now pass. This is precisely the kind of case where the tool's advice needs human judgement rather than blanket enforcement.

…nts, fix changelog Responding to review feedback on the safe-advice PR: - Revert all external implementation->api promotions that had leaked in from the api-scope work (caffeine, zookeeper, jute, jsonpath, opentelemetry-sdk + exporter-prometheus in core; curator-client/jute in solrj-zookeeper; zookeeper +tests in test-framework; calcite in sql; cloud-storage in gcs; awssdk-s3 in s3). None are in the modules' public ABI and DAGP does not advise them; they belong in the separate api-scope discussion PRs, not here. Verified no downstream module relies on the former transitive leak. - Revert solrj-jetty entirely: its changes were api promotions + manual cleanups, none part of the safe mechanical advice. - Restore opentelemetry-sdk-metrics in test-framework: DAGP flagged it unused, but JettySolrRunner uses MetricReader from it and the prometheus exporter is declared transitive=false, so it is required (compile break otherwise). - Restore the gjf-SOLR-18296 changelog entry that had been deleted by a rebase artifact; it belongs to the separate google-java-format change. - Remove code comments that narrated the change/tool advice (per AGENTS.md); keep only terse why-comments matching existing style. - Changelog: type changed->other; link to SOLR-18302 (the correct JIRA). - Regenerate lockfiles/licenses.

Applies DAGP's api advice ONLY for inter-module project() dependencies that are genuinely part of the consuming module's public API (e.g. solr-core's public types expose solrj), where 'api' is correct so downstream modules compile. External-library api promotions are intentionally excluded here (see part 3/3); per dev@ feedback those need case-by-case judgement rather than blanket promotion. Builds on apache#4612. Lockfiles regenerated.

dsmiley · 2026-07-08T02:58:44Z

Please look over my changes.

Not sure if we really need a part 2, let alone 3.

serhiy-bzhezytskyy · 2026-07-08T05:09:49Z

Looked them over — all good. The opt-in analyzeDependencies task is better than wiring it into check, agree; the advice needs a human looking at it, not a gate. Docs read well too, and nice catch on the spotless lockfile configs.

And yeah — fine to drop 2 and 3. 1 is the one worth landing. The project() promotions in 2 were minor and 3 was only ever illustrative, so no loss. Thanks for jumping in on this.

(CI red is the flaky TestGracefulJettyShutdown — passes locally, unrelated to the diff.)

Remove 24 project() declarations across 17 modules that solr-core already provides via its 'api'-scoped dependencies (solrj, solrj-zookeeper, solrj-streaming, api). Modules depending on solr-core receive these transitively, so re-declaring them is redundant -- leftovers from cutterslade, which didn't understand gradle's 'api'. solrj-jetty is kept where declared: core has it as 'implementation' (not re-exposed), so consumers that use it must declare it directly.

serhiy-bzhezytskyy · 2026-07-09T11:24:57Z

Folded the redundancy cleanup in here. Removed 24 redundant declarations across 17 modules — the ones core provides via api (solrj, solrj-zookeeper, solrj-streaming, api), so modules depending on core don't re-declare them.

One more I held off on: core exposes HttpJettySolrClient in its public API (e.g. getDefaultHttpSolrClient()), and DAGP flags solrj-jetty for api. Promoting it would let ~4 more modules drop their solrj-jetty declaration too — but that's an implementation→api promotion, which we said we'd keep out of this PR. Want it in, or leave it?

Full ./gradlew test passes (the one red is the flaky TestGracefulJettyShutdown, unrelated).

dsmiley

Thanks.

Was DAGP useful in this task? Sorry if I'm being redundant maybe but I want to ensure we're adding something useful, otherwise it's more burden/complexity in the most complex gradle build I've ever seen.

I don't want solrj-jetty to be api but I've internally debated it. Want to err on the side of concealing an implementation detail. Solrj base is increasingly capable and thus few modules truly need to reference solrj-jetty.

dsmiley · 2026-07-09T13:52:30Z

@malliaridis this PR and another I have will add more Kotlin formatted build files. Our .editorconfig that I contributed says kotlin build files should have 4-space indentation, despite the general default of 2-space. I simply did that by observation of your choice. Do you think kotlin build files should be 4-space indented for some reason (why)?

dsmiley · 2026-07-09T13:53:19Z

          it.canBeResolved &&
-              !it.attributes.keySet().any {attr -> attr.name.startsWith("dagp.")}
+              !it.attributes.keySet().any {attr -> attr.name.startsWith("dagp.")} &&
+              !it.name.startsWith("spotless")


CC @malliaridis you might want to know about this line I added to get unstable spotless configuration noise out of the lockfile

dsmiley · 2026-07-09T13:55:05Z

  implementation platform(project(':platform'))
  implementation project(':solr:core')
-  implementation project(':solr:solrj')
-  implementation project(':solr:solrj-streaming')


TBH, I want to revisit solrj-streaming being "api" from core. I don't think it should be.

These classes (CatStream, ClassifyStream, AnalyzeEvaluator) are registered as streaming expression functions — SolrDefaultStreamFactory wires them: withFunctionName("cat", CatStream.class), etc. They exist specifically to plug into the streaming framework. They must extend TupleStream/Expressible — that's the whole point of them.

I just tried it — if core has solrj-streaming as implementation instead of api, it builds fine as long as sql/benchmark declare it themselves (sql tests pass). But DAGP immediately says it should be api again.

So it is part of core's public API now and I don't see simple way to fix it.

The "sql" module is definitely the unusual exception with regards to it's dependency on solrj-streaming from within Solr. I think that should be explicit -- not "api".

Agreed, sql is the exception — it's built directly on streaming (~11 files). Added implementation project(':solr:solrj-streaming') back to sql, explicit. Tests pass.

serhiy-bzhezytskyy · 2026-07-09T15:14:49Z

Regarding "solrj-jetty": Agreed on concealing it. Though heads up — CoreContainer.getDefaultHttpSolrClient() returns HttpJettySolrClient as a public method, so solr-core actually does leak solrj-jetty into its public API today.

serhiy-bzhezytskyy · 2026-07-09T15:47:19Z

Regarding "Was DAGP useful in this task?" — it helped a bit: finding unused declarations and test-scope mixups that compile fine so you'd never catch them by eye — the unused removals here came from it. And the solrj-streaming experiment above is a good example — it's what tells me the api scope matches what core actually exposes.

But it's compile-time only — it can't see runtime usage. It flagged kafka-clients:test as unused, but removing it broke the CrossDC integration tests at runtime (EmbeddedKafkaCluster loads TestCondition from it); only the test run caught it. And the redundant-declaration cleanup was manual — DAGP wants to keep those.

So does it bring big value? IMO no. Lucene doesn't use any dependency-analysis plugin and its build is fine. I'd keep it as the opt-in analyzeDependencies (not a gate), but fine to drop it.

serhiy-bzhezytskyy · 2026-07-09T15:59:49Z

Double-checked before leaving it — ran buildHealth again on the cleaned-up build to see what DAGP flags now. Two things, and both are wrong for us: it calls opentelemetry-sdk-metrics unused (it's not — JettySolrRunner uses it at runtime, the compile-blindness thing again), and it wants test-framework to re-declare the api project(:api/:solrj) we just removed as redundant. So right now it's mostly arguing against the decisions we made here.

The cleanup value it did give is already in this PR either way. So I'll stand by "opt-in, not a gate" — but no strong case that it earns ongoing keep.

dsmiley · 2026-07-09T18:41:03Z

RE CoreContainer.getDefaultHttpSolrClient() yes I'm quite aware of that one. Albeit the callers almost always don't actually care that it's specifically of a Jetty nature. So again, I think modules depending on Solr generally don't care; save to a HttpSolrClient type without caring about specifically what subtype it is. Perhaps the return type should be made HttpSolrClient and we just accept a bit of casting in rare spots.

@dsmiley

…lrClient Return the base HttpSolrClient type instead of the concrete HttpJettySolrClient, so consumers don't couple to the Jetty implementation. Modules generally don't care about the subtype (per @dsmiley) -- confirmed: the one module caller (cross-dc) uses only base SolrClient methods, so cross-dc can now drop its solrj-jetty dependency. Widened 4 internal callers to the base type (ColStatus, IterativeMergeStrategy, SolrConfigHandler.PerReplicaCallable) or the base API (PackageAPI now uses processWithBaseUrl). Five callers keep a cast to HttpJettySolrClient because they genuinely use Jetty-only API: Builder.withHttpClient (shares Jetty transport internals), getHttpClient() for raw proxying, and the async requestWithBaseUrl(baseUrl, fn) overload. Pure compile-time change; runtime behavior is unchanged. Experimental; for discussion on whether to keep here or split out.

serhiy-bzhezytskyy · 2026-07-10T09:25:39Z

Pushed the getDefaultHttpSolrClient() → HttpSolrClient change here to keep it in context. It works out how you expected — modules don't care about the subtype: cross-dc was the only module calling it, uses base methods only, so cross-dc drops solrj-jetty now. Inside core, 4 callers widened cleanly; 5 keep a cast where they actually use Jetty-only API (Builder.withHttpClient sharing transport internals, getHttpClient() for raw proxying, the async requestWithBaseUrl(baseUrl, fn) overload). Pure compile-time change, runtime unchanged. Full core + cross-dc tests pass.

This PR has grown a lot though — DAGP reporting, the redundant-declaration cleanup, and now this return-type change are really three separate things. How do you want to handle it? Options I see:

split into separate PRs (DAGP / redundancy cleanup / getDefaultHttpSolrClient) so each can be judged on its own,
or keep as-is if you'd rather review together.

Let me know what action I should take with this PR and the topics touched here. Hope I'm contributing value, not noise.

Per review: sql is built directly on streaming expressions (it uses org.apache.solr.client.solrj.io.* across ~11 files), so it should declare solrj-streaming explicitly rather than rely on getting it transitively through solr-core's api. The earlier redundant-declaration cleanup removed it as 'provided by core'; sql is the exception where the dependency is real and direct.

SOLR-18296: add changelog entry for dependency-analysis PR (apache#4612)

03a76c4

This was referenced Jul 5, 2026

SOLR-18302: DAGP inter-module project() api promotions (part 2/3, draft) #4613

Closed

SOLR-18302: DAGP external-lib api advice — ILLUSTRATIVE, not for merge (part 3/3) #4614

Closed

dsmiley reviewed Jul 7, 2026

View reviewed changes

serhiy-bzhezytskyy changed the title ~~SOLR-18296: dependency-analysis (DAGP) — reporting + safe advice (part 1/3)~~ SOLR-18302: dependency-analysis (DAGP) — reporting + safe advice (part 1/3) Jul 7, 2026

github-actions Bot removed documentation Improvements or additions to documentation module:sql cat:api labels Jul 7, 2026

github-actions Bot added documentation Improvements or additions to documentation and removed module:clustering module:langid module:scripting labels Jul 8, 2026

github-actions Bot added module:extraction module:jwt-auth module:ltr module:s3-repository module:gcs-repository module:sql module:clustering module:langid module:scripting labels Jul 9, 2026

serhiy-bzhezytskyy changed the title ~~SOLR-18302: dependency-analysis (DAGP) — reporting + safe advice (part 1/3)~~ SOLR-18302: dependency-analysis (DAGP) — reporting + build hygiene Jul 9, 2026

dsmiley reviewed Jul 9, 2026

View reviewed changes

github-actions Bot added jetty-server cat:search cat:cloud cat:packagemanager labels Jul 10, 2026

		@@ -0,0 +1,7 @@
		title: Add dependency-analysis (build-health) reporting to the build, and apply its uncontroversial advice (unused-dependency removals and test-scope corrections)
		type: changed

		- name: PR#4612
		url: https://github.com/apache/solr/pull/4612

Uh oh!

Conversation

serhiy-bzhezytskyy commented Jul 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Testing

AI assistance disclosure

Uh oh!

dsmiley left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serhiy-bzhezytskyy Jul 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serhiy-bzhezytskyy commented Jul 7, 2026

Uh oh!

dsmiley commented Jul 8, 2026

Uh oh!

serhiy-bzhezytskyy commented Jul 8, 2026

Uh oh!

serhiy-bzhezytskyy commented Jul 9, 2026

Uh oh!

dsmiley left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

serhiy-bzhezytskyy commented Jul 5, 2026 •

edited

Loading

serhiy-bzhezytskyy Jul 7, 2026 •

edited

Loading