SOLR-18302: DAGP inter-module project() api promotions (part 2/3, draft)#4613
SOLR-18302: DAGP inter-module project() api promotions (part 2/3, draft)#4613serhiy-bzhezytskyy wants to merge 7 commits into
Conversation
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.
github-actions
Bot
added
documentation
dsmiley
left a comment
dsmiley
left a comment
There was a problem hiding this comment.
(looked only at the suitable commit)
Looks good... I thought you'd make more changes.
…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
changed the title
…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.
serhiy-bzhezytskyy
force-pushed
the
dagp-pr2-project-api
branch
from
1ffce36 to
2f1a9f5
Compare
github-actions
Bot
removed
documentation
…jetty
Remove testImplementation project(':solr:solrj-jetty') (a 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')). The
project(path: ':solr:solrj', configuration: 'testArtifacts') declaration stays; it
provides solrj's test classes, which is a real dependency. Verified :solr:solrj-jetty
test compiles and all 59 tests pass.
serhiy-bzhezytskyy
force-pushed
the
dagp-pr2-project-api
branch
from
2f1a9f5 to
a98b12b
Compare
…ions (part 3/3) Applies the 90 external-library implementation->api promotions DAGP advises across the modules, for discussion. Verified all 90 against each module's ABI dump: every one is genuinely exposed in public API (return type, parameter, field, or supertype) today, regardless of the declaration -- e.g. CaffeineCache implements caffeine's RemovalListener; ~82 core methods throw ZooKeeper's KeeperException. So this is an api-hygiene question (is the exposure intended, or worth encapsulating?), not the tool over-reaching. jersey-core-server is left as implementation: DAGP advises api (it is ABI-exposed), but its version comes from the implementation-scoped jersey BOM, so as api it fails to resolve. A concrete case of advice that is correct about exposure yet cannot be applied mechanically. Builds on apache#4613. check -x test passes; lockfiles regenerated.
…, doc the lockfile filter
- Convert gradle/validation/dependency-analyze.gradle to Kotlin DSL (.gradle.kts),
per the project's preference for Kotlin build files. Behavior is unchanged;
buildHealth runs and all suppressions (:solr:ui, solrj-jetty, jwt-auth,
report-not-fail) remain active.
- Remove the redundant self-referential testImplementation project(':self')
declarations in api, solrj, solrj-streaming, solrj-zookeeper, and core (two were
left-over 'cutterslade requires we state the obvious' workarounds for the plugin
removed in SOLR-18289). Test source sets already see their own main classes.
- Add a one-line comment explaining why resolveAndLockAll skips the plugin's
synthetic dagp.* configurations.
- Regenerate the changelog entry with the writeChangelog task so it matches the
documented format.
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.
serhiy-bzhezytskyy
force-pushed
the
dagp-pr2-project-api
branch
from
a98b12b to
fa4f0f0
Compare
…ions (part 3/3) Applies the 90 external-library implementation->api promotions DAGP advises across the modules, for discussion. Verified all 90 against each module's ABI dump: every one is genuinely exposed in public API (return type, parameter, field, or supertype) today, regardless of the declaration -- e.g. CaffeineCache implements caffeine's RemovalListener; ~82 core methods throw ZooKeeper's KeeperException. So this is an api-hygiene question (is the exposure intended, or worth encapsulating?), not the tool over-reaching. jersey-core-server is left as implementation: DAGP advises api (it is ABI-exposed), but its version comes from the implementation-scoped jersey BOM, so as api it fails to resolve. A concrete case of advice that is correct about exposure yet cannot be applied mechanically. Builds on apache#4613. check -x test passes; lockfiles regenerated.
| implementation project(':solr:core') | ||
| implementation project(':solr:solrj') | ||
| api project(':solr:core') | ||
| api project(':solr:solrj') |
There was a problem hiding this comment.
It's confusing to me why solrj is listed here at all (whatever configuration scope). solr-core has an "api" config dependency on solrj... thus this means solrJ is exposed at equivalent scope to whoever consumed solr-core. The subproject here is depending on solr-core... so then why also mention solrj at all?
What I'm talking about is probably tangential to this PR because you didn't change the scopes I'm talking about. Note that many of our dependency declarations are redundant due to cutterslade not understanding the nuance of gradle's "api". I'm hoping we can remove these redundant declarations and maybe the dagp thing you are adding could help us identify redundancies?
There was a problem hiding this comment.
Good question, and it's actually the opposite of redundant here — cross-dc-manager uses solrj directly (CloudSolrClient, UpdateRequest, SolrRequest, etc. in ~4 files), so declaring it is the right call rather than leaning on the transitive api from core. The Gradle guidance is "declare what you use directly," so you don't break when core's internals shift.
And yeah — DAGP is exactly the tool for what you're after. It flags both directions: unused declarations you can drop, and transitives you use directly but haven't declared. For this one it agrees with keeping solrj (doesn't flag it for removal). So it should help find the genuinely-redundant cutterslade leftovers without touching the legit direct declarations.
Happy to do a pass on the redundant ones as a follow-up if useful.
There was a problem hiding this comment.
We agree there's a judgement call to make a dependency "api" instead of "implementation"; we're not doing it willy-nilly or simply because some tool found one public API signature using the dependency. If we are diligent about this (I think we are), then why not rely on this judgement on the consuming end to not be redundant? Otherwise, what's the point of "api" at all if we don't take advantage of it.
Case in point: There's no way in hell that SolrJ will stop being a dependency of Solr-core. Any consumer of solr-core will consume solrJ. Even if for some other example where it'a "api" today but we change our mind... well so what? Consumer will find out (won't compile) and add the dependency explicitly then.
There was a problem hiding this comment.
Strong agree — this is a human call, not the tool's. dagp wouldn't flag it for removal (it'd keep solrj — cross-dc uses it directly). I can do a manual pass and put up a PR — WDYT?
There was a problem hiding this comment.
Thanks. I think I'd prefer that you iterate on the part1 PR; no need for additional ones IMO.
2 participants
Draft, for review — part 2 of 3. Builds on #4612 (which must merge first).
Applies DAGP's
implementation -> apiadvice only for the ~36 inter-moduleproject(...)dependencies where the type is genuinely part of the consuming module's public API — e.g.:solr:corepublicly exposes:solr:solrjtypes, soapiis correct and downstream modules compile against them.The ~91 external-library api promotions DAGP also suggests are deliberately not here — see part 3/3 (draft, not for merge), which shows them for discussion. Per @dsmiley's point that DAGP is "api-happy" and liberal
apiis viral/transitive, those need case-by-case judgement.gradlew check -x testpasses.AI-assisted, human-reviewed per AGENTS.md.