Make a null recursive consistently mean non-recursive in ChangePackage

[timtebeek]

ChangePackage.recursive is @Nullable with required = false and no documented default, and null was read with two different meanings inside the same recipe:

Read as non-recursive when null (Boolean.TRUE.equals)	Read as recursive when null (recursive == null || recursive)
condition precondition, JavaSourceFile branch	getNewPackageName
condition precondition, SourceFileWithReferences branch	isTargetRecursivePackageName
PackageMatcher built in preVisit
star-import expansion in postVisit

For Java sources the precondition gated on the non-recursive reading while the visitor body then acted on the recursive one. The result: a subpackage type was renamed only if the file independently satisfied the non-recursive precondition — i.e. only if it also referenced a type sitting directly in oldPackageName.

With new ChangePackage("cucumber.api.java", "io.cucumber.java", null):

// before: no change at all, the only import is in a subpackage
import cucumber.api.java.en.Given;

// before: both rewritten, including the subpackage one
import cucumber.api.java.Before;
import cucumber.api.java.en.Given;

Whether subpackages got renamed depended on unrelated properties of the source file. The four tests in NullRecursiveDefaultsToNonRecursive were written against the old code first and reproduce exactly this split.

The default: null means non-recursive

Both readings have real costs; this picks the conservative one.

• required = false on a Boolean with no stated default means false throughout OpenRewrite, and Boolean.TRUE.equals(...) was already the idiom at 4 of the 6 sites plus the entire non-Java path.
• The failure modes are asymmetric. Under non-recursive, a rename that should have happened doesn't — visible, and fixed with a one-line recursive: true in the recipe that wants it. Under recursive, a rename happens that shouldn't, producing package names that never existed, and the fix requires auditing every call site to add recursive: false.
• Defaulting to recursive would silently widen the blast radius of 282 declarative call sites that omit the option (see below), in a patch release, with no way for a user pinned to an existing recipe YAML to opt out — including onto properties/YAML/XML/service-provider files those recipes have never touched.
• It is the only default under which Java and non-Java sources can agree on a bounded, statable semantic.

Non-recursive now means "the package itself and the types declared directly in it", matching the recipe's own @DocumentExample (renameUsingSimplePackageName renames import org.openrewrite.Foo with recursive = false). All readings route through a single isRecursive() accessor, and the @Option description states the default.

Two sites beyond the six, needed to make that semantic true

Fixing only the six left "non-recursive" meaning two different things, which is the same class of defect:

1. visitFieldAccess was unconditionally recursive. In import cucumber.api.java.en.Given; the inner cucumber.api.java node is itself a fully-qualified reference matching oldPackageName, so it was rewritten regardless of the option — this is the actual mechanism by which subpackage imports leaked through. It now only fires when the occurrence qualifies a type declared directly in oldPackageName (checked via type attribution, falling back to a naming heuristic, with * allowed for star imports).

2. PackageMatcher.matchesReference was degenerate when non-recursive. It matched only value.equals(targetPackage), so a reference like cucumber.api.java.Before was never selected — meaning non-recursive ChangePackage did nothing at all to properties/YAML/XML/service-provider files unless a reference was the bare package name. getReplacement was already prepared to rename exactly those values; the matcher just never produced them. This is a deliberate, bounded widening for non-Java sources (direct types only, never subpackages).

The JavaSourceFile package-declaration precondition also used a boundary-less startsWith — a third reading of the option — and now mirrors the visitor.

Test changes

Six new tests in NullRecursiveDefaultsToNonRecursive cover a subpackage-only source and a mixed source for both a JavaSourceFile and a SourceFileWithReferences input, plus opt-in (recursive = true) counterparts for both source kinds.

Two existing tests pinned the old split semantics and were updated deliberately — they were the only failures across rewrite-java, rewrite-java-test, rewrite-groovy, rewrite-kotlin, rewrite-maven, rewrite-properties, rewrite-yaml, rewrite-xml and rewrite-java-tck:

• renamePackageRecursive relied on the class-level recursive = null default to get recursive behaviour; it now passes true explicitly, which is what its name always claimed.
• changePackageInServiceProviderFileNonRecursive asserted that ChangePackage("org.foo", "org.bar", false) leaves org.foo.MyImpl unchanged, which contradicts the @DocumentExample. It now asserts the direct entry is renamed and a org.foo.sub.MyImplB entry is left alone, so it still tests non-recursion.

Downstream impact

Across the locally checked-out rewrite-* / recipes-* modules (source trees only, build outputs excluded): 411 declarative ChangePackage usages — 282 omit recursive, 124 set true, 5 set false. Of the omitters, 260 have no sibling rule covering a subpackage, so those are the ones that could have been relying on the recursive reading.

I validated the heaviest consumers by publishing this branch and its parent commit to mavenLocal and diffing full test runs against each repo's origin/main:

Repo	ChangePackage uses	Baseline	With this PR
rewrite-migrate-java (OSS)	55	green	green
rewrite-spring-v1 (OSS)	50	green	green
rewrite-apache (OSS)	41	green	green via openrewrite/rewrite-apache#141 (merged)
rewrite-spring (proprietary)	213	green	1 failure → moderneinc/rewrite-spring#409

3 regressions across ~359 call sites, all the same shape — a rule that omits recursive while depending on it — and each fixed by a single recursive: true line:

• org.apache.commons.math → org.apache.commons.math3, which must carry org.apache.commons.math.stat.
• org.apache.hc.core5.http.io.entity.mime → org.apache.hc.client5.http.entity.mime, step 2 of a three-step chain. Without recursion the chain stalls at org.apache.hc.core5.http.io.entity.mime.content.StringBody, a package that doesn't exist in httpclient5. Worth noting the failure mode is still under-application, but a stalled chain can leave an intermediate name behind rather than an obviously missing rename.
• org.springframework.session.hazelcast → com.hazelcast.spring.session, which must carry ...hazelcast.config.annotation.web.http. Its test passes today only because the fixture also imports a type sitting directly in the old package; a config class importing just EnableHazelcastHttpSession was never migrated at all. That is a latent bug this PR surfaces rather than causes.

Both downstream PRs are green with and without this one, so they merge independently and in any order.

I then swept the remaining 13 consumers the same way — rewrite-cucumber-jvm, rewrite-hibernate, rewrite-jackson, rewrite-jenkins, rewrite-logging-frameworks, rewrite-micrometer, rewrite-micronaut, rewrite-netty, rewrite-rewrite, rewrite-spring-to-quarkus, rewrite-static-analysis, rewrite-struts, rewrite-testing-frameworks — and all 13 show zero new failures. (rewrite-spring-to-quarkus has one pre-existing MigrateDatabaseDriversTest failure about Maven <scope>, identical at baseline.)

That contradicts my own prediction: I had flagged cucumber.api → io.cucumber and org.apache.log4j → org.apache.logging.log4j as likeliest to break and neither did. Their suites don't exercise a subpackage-only source, so this confirms no test regression rather than proving those renames are unaffected in the field — a repo importing only cucumber.api.java.en.Given would still stop being migrated until that recipe sets recursive: true. Both were already broken for that shape before this PR, since the precondition never matched such a file.

17 repos validated in total, 3 regressions, all fixed and merged.

No usages of PackageMatcher outside ChangePackage in this repo.

[timtebeek]

@sambsnyd ready for review when you have a moment. Downstream validation is complete — 17 consumer repos, 3 regressions, all the same shape and all fixed and merged (openrewrite/rewrite-apache#141, moderneinc/rewrite-spring#409). The judgement call worth your eye is the default itself, written up under "The default: null means non-recursive", and the two sites beyond the six in the original report.

[timtebeek]

Full audit of declarative ChangePackage usages

The sweep above proves no test regressions, but the tests can't see the underlying bug by construction — a rule that omits recursive while depending on it silently fails only for source files that reference nothing directly in oldPackageName, and no fixture looks like that.

So I audited all 285 recursive-omitting rules at each repo's latest main against the published artifacts: enumerate the old package's subpackages from the real jar, apply prefix substitution, and check whether the result exists in the new jar. That gives a yes/no per rule rather than a guess.

7 rules across 4 repos need recursive: true, each now a PR with a test that fails without it:

PR	rule	evidence
openrewrite/rewrite-testing-frameworks#1072	com.github.javafaker → net.datafaker	3/3 subpackages map
openrewrite/rewrite-testing-frameworks#1072	org.fest.assertions.api → org.assertj.core.api	.filter maps
openrewrite/rewrite-migrate-java#1185	com.fasterxml.jackson.jaxrs.json → ...jakarta.rs.json	.annotation maps
openrewrite/rewrite-micronaut#144	io.micronaut.configuration.security.ldap → io.micronaut.security.ldap	3/3 map
moderneinc/rewrite-spring#410	...actuate.autoconfigure.logging → ...opentelemetry.autoconfigure.logging	1/1
moderneinc/rewrite-spring#410	...autoconfigure.web.servlet → ...webmvc.autoconfigure	1/1
moderneinc/rewrite-spring#410	...actuate.autoconfigure.metrics.export → ...micrometer.metrics.autoconfigure.export	18/20

Where recursion would be wrong

The audit is at least as useful for what it rules out. Several renames look like prefix substitutions but are redistributions, and blanket recursion would invent packages that never existed:

• rewrite-apache — all 8 httpclient rules with subpackages fail the check. org.apache.http → org.apache.hc.core5.http would map 32 of 40 subpackages to non-existent targets (org.apache.http.annotation → org.apache.hc.core5.http.annotation, which doesn't exist — it's org.apache.hc.core5.annotation). The recipe's non-recursive-plus-explicit-enumeration design is correct; no changes needed beyond the merged Fix parsing issue when a block is suffixed with a multi-comment #141.
• rewrite-spring — of 18 rules with subpackages, 15 already have explicit sibling rules, e.g. ...autoconfigure.jms.activemq → org.springframework.boot.activemq.autoconfigure, not ...jms.autoconfigure.activemq. Recursion would override the more specific mapping.
• rewrite-logging-frameworks — org.apache.log4j → org.apache.logging.log4j: log4j 1.x subpackages have no log4j2 counterparts. This was my top suspect earlier and the artifacts show it must stay non-recursive.
• rewrite-migrate-java — com.alibaba.fastjson → com.alibaba.fastjson2 (only .annotation has a counterpart; no .parser / .serializer / .support.*) and com.sun.net.ssl → javax.net.ssl (no .internal).

Everything else — the DataNucleus set, org.apache.shiro.codec, javax.annotation.security / .sql, the Micronaut Cassandra rule, io.micrometer.prometheus, com.tngtech.archunit.library.plantuml, the Codahale and Spring Boot web-context rules — are leaf packages with no subpackages, so the default is already right for them.

None of these downstream PRs depend on this one; each is green with and without it.

[timtebeek]

Third-party recipe modules

Also audited the external modules bundled into rewrite-third-party, by unpacking their published jars and applying the same artifact-backed check.

module	ChangePackage rules	at risk
org.apache.camel.upgrade:camel-upgrade-recipes	0	—
org.apache.wicket:wicket-migration	0	—
software.amazon.awssdk:v2-migration	0	—
ai.timefold.solver:timefold-solver-migration	6, all set recursive explicitly	—
org.axonframework:axon-migration	2 omit	none (both leaf packages)
io.quarkus:quarkus-update-recipes	3 omit	1 → quarkusio/quarkus-updates#516
com.oracle.weblogic.rewrite:rewrite-weblogic	4 omit	1 → oracle/rewrite-recipes#109

• quarkus — org.apache.camel.test.junit5 → org.apache.camel.test.junit6 needs recursion; .params and .util both map (camel-test-junit5 4.8.1 → camel-test-junit6 4.21.0).
• weblogic — jakarta.sql.rowset → javax.sql.rowset needs recursion; .serial and .spi both exist in the JDK.

One near miss worth recording: quarkus's javax.transaction → jakarta.transaction must stay non-recursive. javax.transaction.xa is Java SE, not Jakarta EE, and recursion would rewrite javax.transaction.xa.XAResource to a jakarta.transaction.xa package that does not exist. Camel and Wicket — two of the modules I was asked about — turn out not to use ChangePackage at all.

Both PRs are raised from forks and are correct independently of this one.