From 4ac0430a6b8b2cc14d6173920a53abe785ad7f3b Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Tue, 4 Aug 2026 22:02:46 +0200 Subject: [PATCH 1/3] Make a null `recursive` consistently mean non-recursive in `ChangePackage` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `recursive` is `@Nullable` with `required = false` and no documented default, and null was read with two different meanings inside the same recipe: the preconditions and the `SourceFileWithReferences` path treated it as non-recursive, while `getNewPackageName` and `isTargetRecursivePackageName` treated it as recursive. For Java sources the precondition gated on the non-recursive reading and the visitor body then acted on the recursive one, so a subpackage type was renamed only when the file independently satisfied the non-recursive precondition — i.e. only when it also referenced a type sitting directly in `oldPackageName`: // no change, the only import is in a subpackage import cucumber.api.java.en.Given; // both rewritten, including the subpackage one import cucumber.api.java.Before; import cucumber.api.java.en.Given; Null now means non-recursive everywhere, routed through a single `isRecursive()` accessor, and documented on the `@Option`. Non-recursive means "the package itself and the types declared directly in it", matching the `@DocumentExample` `renameUsingSimplePackageName`, and it now means that for Java and non-Java sources alike: - `visitFieldAccess` no longer rewrites `oldPackageName` when it is only the leading segments of a subpackage-qualified name such as `oldPackageName.sub.Type`. - `PackageMatcher.matchesReference` now selects `oldPackageName.Type` when non-recursive, which `getReplacement` was already prepared to rename but the matcher never produced. Previously non-recursive mode was degenerate for properties/YAML/XML/service-provider files: it matched only a bare reference to the package name itself. - The `JavaSourceFile` package-declaration precondition used a boundary-less `startsWith`, a third reading of the option; it now mirrors the visitor. --- .../openrewrite/java/ChangePackageTest.java | 160 +++++++++++++++++- .../org/openrewrite/java/ChangePackage.java | 49 ++++-- .../org/openrewrite/java/PackageMatcher.java | 22 ++- .../resources/META-INF/rewrite/recipes.csv | 2 +- 4 files changed, 214 insertions(+), 19 deletions(-) diff --git a/rewrite-java-test/src/test/java/org/openrewrite/java/ChangePackageTest.java b/rewrite-java-test/src/test/java/org/openrewrite/java/ChangePackageTest.java index 803de188e35..917dfb504ae 100644 --- a/rewrite-java-test/src/test/java/org/openrewrite/java/ChangePackageTest.java +++ b/rewrite-java-test/src/test/java/org/openrewrite/java/ChangePackageTest.java @@ -16,6 +16,7 @@ package org.openrewrite.java; import org.intellij.lang.annotations.Language; +import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; import org.openrewrite.DocumentExample; import org.openrewrite.InMemoryExecutionContext; @@ -340,6 +341,7 @@ class Test { @Test void renamePackageRecursive() { rewriteRun( + spec -> spec.recipe(new ChangePackage("org.openrewrite", "org.openrewrite.test", true)), java( """ package org.openrewrite.internal; @@ -573,6 +575,153 @@ class A { ); } + /** + * A null {@code recursive} means non-recursive, and it means that for every source kind and + * regardless of what else the source happens to reference. Before this was pinned down, a null + * {@code recursive} was read as non-recursive by the precondition but as recursive by the + * visitor, so a subpackage type was renamed only when the same file also referenced a type + * sitting directly in {@code oldPackageName}. + */ + @Nested + class NullRecursiveDefaultsToNonRecursive { + + private static final JavaParser.Builder cucumber = JavaParser.fromJavaVersion().dependsOn( + """ + package cucumber.api.java; + public @interface Before {} + """, + """ + package cucumber.api.java.en; + public @interface Given {} + """ + ); + + @Test + void javaSourceWithOnlySubpackageReference() { + rewriteRun( + spec -> spec.recipe(new ChangePackage("cucumber.api.java", "io.cucumber.java", null)) + .parser(cucumber), + java( + """ + import cucumber.api.java.en.Given; + + class A { + @Given + void given() {} + } + """ + ) + ); + } + + @Test + void javaSourceWithSubpackageAndDirectReference() { + rewriteRun( + spec -> spec.recipe(new ChangePackage("cucumber.api.java", "io.cucumber.java", null)) + .parser(cucumber), + java( + """ + import cucumber.api.java.Before; + import cucumber.api.java.en.Given; + + class A { + @Before + void before() {} + + @Given + void given() {} + } + """, + """ + import io.cucumber.java.Before; + import cucumber.api.java.en.Given; + + class A { + @Before + void before() {} + + @Given + void given() {} + } + """ + ) + ); + } + + @Test + void javaSourceWithOnlySubpackageReferenceOptingIn() { + rewriteRun( + spec -> spec.recipe(new ChangePackage("cucumber.api.java", "io.cucumber.java", true)) + .parser(cucumber), + java( + """ + import cucumber.api.java.en.Given; + + class A { + @Given + void given() {} + } + """, + """ + import io.cucumber.java.en.Given; + + class A { + @Given + void given() {} + } + """ + ) + ); + } + + @Test + void referenceSourceWithOnlySubpackageReference() { + rewriteRun( + spec -> spec.recipe(new ChangePackage("cucumber.api.java", "io.cucumber.java", null)), + properties( + """ + given=cucumber.api.java.en.Given + """, + spec -> spec.path("application.properties") + ) + ); + } + + @Test + void referenceSourceWithSubpackageAndDirectReference() { + rewriteRun( + spec -> spec.recipe(new ChangePackage("cucumber.api.java", "io.cucumber.java", null)), + properties( + """ + before=cucumber.api.java.Before + given=cucumber.api.java.en.Given + """, + """ + before=io.cucumber.java.Before + given=cucumber.api.java.en.Given + """, + spec -> spec.path("application.properties") + ) + ); + } + + @Test + void referenceSourceWithOnlySubpackageReferenceOptingIn() { + rewriteRun( + spec -> spec.recipe(new ChangePackage("cucumber.api.java", "io.cucumber.java", true)), + properties( + """ + given=cucumber.api.java.en.Given + """, + """ + given=io.cucumber.java.en.Given + """, + spec -> spec.path("application.properties") + ) + ); + } + } + @Issue("https://github.com/openrewrite/rewrite/issues/1997") @Test void typeParameter() { @@ -2182,8 +2331,17 @@ void changePackageInServiceProviderFileNonRecursive() { rewriteRun( spec -> spec.recipe(new ChangePackage("org.foo", "org.bar", false)), text( - "org.foo.MyImpl\n", + """ + org.foo.MyImplA + org.foo.sub.MyImplB + """, + """ + org.bar.MyImplA + org.foo.sub.MyImplB + """, spec -> spec.path("META-INF/services/org.foo.MyInterface") + .afterRecipe(pt -> assertThat(pt.getSourcePath().toString().replace('\\', '/')) + .isEqualTo("META-INF/services/org.bar.MyInterface")) ) ); } diff --git a/rewrite-java/src/main/java/org/openrewrite/java/ChangePackage.java b/rewrite-java/src/main/java/org/openrewrite/java/ChangePackage.java index d824e672986..e20bbd86e3d 100644 --- a/rewrite-java/src/main/java/org/openrewrite/java/ChangePackage.java +++ b/rewrite-java/src/main/java/org/openrewrite/java/ChangePackage.java @@ -57,11 +57,16 @@ public class ChangePackage extends Recipe { @With @Option(displayName = "Recursive", - description = "Recursively change subpackage names", + description = "Recursively change subpackage names. Defaults to `false`, renaming only types " + + "directly in `oldPackageName`; set to `true` to also rename types in its subpackages.", required = false) @Nullable Boolean recursive; + private boolean isRecursive() { + return Boolean.TRUE.equals(recursive); + } + @Override public String getInstanceNameSuffix() { return String.format("`%s` to `%s`", oldPackageName, newPackageName); @@ -84,16 +89,16 @@ public TreeVisitor getVisitor() { @Override public @Nullable Tree preVisit(@Nullable Tree tree, ExecutionContext ctx) { stopAfterPreVisit(); + boolean recursive = isRecursive(); + String recursivePackageNamePrefix = oldPackageName + "."; if (tree instanceof JavaSourceFile) { JavaSourceFile cu = (JavaSourceFile) tree; if (cu.getPackageDeclaration() != null) { String original = PackageNameUtils.getPackageName(cu.getPackageDeclaration()); - if (original.startsWith(oldPackageName)) { + if (original.equals(oldPackageName) || recursive && original.startsWith(recursivePackageNamePrefix)) { return SearchResult.found(cu); } } - boolean recursive = Boolean.TRUE.equals(ChangePackage.this.recursive); - String recursivePackageNamePrefix = oldPackageName + "."; for (J.Import anImport : cu.getImports()) { String importedPackage = anImport.getPackageName(); if (importedPackage.equals(oldPackageName) || recursive && importedPackage.startsWith(recursivePackageNamePrefix)) { @@ -115,10 +120,9 @@ public TreeVisitor getVisitor() { } } else if (tree instanceof SourceFileWithReferences) { SourceFileWithReferences cu = (SourceFileWithReferences) tree; - boolean recursive = Boolean.TRUE.equals(ChangePackage.this.recursive); - String recursivePackageNamePrefix = oldPackageName + "."; + PackageMatcher matcher = new PackageMatcher(oldPackageName, recursive); for (Reference ref : cu.getReferences().getReferences()) { - if (ref.getValue().equals(oldPackageName) || recursive && ref.getValue().startsWith(recursivePackageNamePrefix)) { + if (matcher.matchesReference(ref)) { return SearchResult.found(cu); } } @@ -141,8 +145,7 @@ public boolean isAcceptable(SourceFile sourceFile, ExecutionContext ctx) { } else if (tree instanceof SourceFileWithReferences) { SourceFileWithReferences sourceFile = (SourceFileWithReferences) tree; SourceFileWithReferences.References references = sourceFile.getReferences(); - boolean recursive = Boolean.TRUE.equals(ChangePackage.this.recursive); - PackageMatcher matcher = new PackageMatcher(oldPackageName, recursive); + PackageMatcher matcher = new PackageMatcher(oldPackageName, isRecursive()); Map> matches = new HashMap<>(); for (Reference ref : references.findMatches(matcher)) { matches.computeIfAbsent(ref.getTree(), k -> new java.util.ArrayList<>()).add(ref); @@ -170,7 +173,8 @@ public J visitFieldAccess(J.FieldAccess fieldAccess, ExecutionContext ctx) { if (parent != null && // Ensure the parent isn't a J.FieldAccess OR the parent doesn't match the target package name. (!(parent.getValue() instanceof J.FieldAccess) || - (!(((J.FieldAccess) parent.getValue()).isFullyQualifiedClassReference(newPackageName))))) { + (!(((J.FieldAccess) parent.getValue()).isFullyQualifiedClassReference(newPackageName)))) && + (isRecursive() || qualifiesTypeDirectlyInOldPackage(parent))) { f = TypeTree.build(((JavaType.FullyQualified) newPackageType).getFullyQualifiedName()) .withPrefix(f.getPrefix()); @@ -179,6 +183,25 @@ public J visitFieldAccess(J.FieldAccess fieldAccess, ExecutionContext ctx) { return f; } + /** + * Whether this occurrence of {@code oldPackageName} qualifies a type declared directly in it, + * as opposed to being the leading segments of a subpackage-qualified name such as + * {@code oldPackageName.sub.Type}. Only the former may be renamed when non-recursive. + */ + private boolean qualifiesTypeDirectlyInOldPackage(Cursor parent) { + if (!(parent.getValue() instanceof J.FieldAccess)) { + return true; + } + J.FieldAccess qualified = (J.FieldAccess) parent.getValue(); + JavaType.FullyQualified fq = TypeUtils.asFullyQualified(qualified.getType()); + if (fq != null) { + return oldPackageName.equals(fq.getPackageName()); + } + String nextSegment = qualified.getSimpleName(); + return "*".equals(nextSegment) || + !nextSegment.isEmpty() && Character.isUpperCase(nextSegment.charAt(0)); + } + @Override public J visitPackage(J.Package pkg, ExecutionContext ctx) { String original = PackageNameUtils.getPackageName(pkg); @@ -282,7 +305,7 @@ public J postVisit(J tree, ExecutionContext ctx) { String oldSubPkg = oldPackageName + changingTo.substring(newPackageName.length()); sf = maybeExpandStarImport(sf, changingTo, oldSubPkg, ctx); } - if (Boolean.TRUE.equals(recursive)) { + if (isRecursive()) { for (J.Import anImport : sf.getImports()) { if (!anImport.isStatic() && "*".equals(anImport.getQualid().getSimpleName())) { String pkg = anImport.getPackageName(); @@ -518,7 +541,7 @@ private JavaType.FullyQualified findType(String fqn, JavaSourceFile cu) { } private String getNewPackageName(String packageName) { - return (recursive == null || recursive) && !newPackageName.endsWith(packageName.substring(oldPackageName.length())) ? + return isRecursive() && !newPackageName.endsWith(packageName.substring(oldPackageName.length())) ? newPackageName + packageName.substring(oldPackageName.length()) : newPackageName; } @@ -529,7 +552,7 @@ private boolean isTargetFullyQualifiedType(JavaType.@Nullable FullyQualified fq) } private boolean isTargetRecursivePackageName(String packageName) { - return (recursive == null || recursive) && + return isRecursive() && packageName.startsWith(oldPackageName + ".") && !packageName.startsWith(newPackageName); } diff --git a/rewrite-java/src/main/java/org/openrewrite/java/PackageMatcher.java b/rewrite-java/src/main/java/org/openrewrite/java/PackageMatcher.java index 92536dc8785..dbf9eb23bfe 100644 --- a/rewrite-java/src/main/java/org/openrewrite/java/PackageMatcher.java +++ b/rewrite-java/src/main/java/org/openrewrite/java/PackageMatcher.java @@ -39,14 +39,28 @@ public PackageMatcher(@Nullable String targetPackage, boolean recursive) { @Override public boolean matchesReference(Reference reference) { if (reference.getKind() == Reference.Kind.TYPE || reference.getKind() == Reference.Kind.PACKAGE) { - String recursivePackageNamePrefix = targetPackage + "."; - if (reference.getValue().equals(targetPackage) || recursive && reference.getValue().startsWith(recursivePackageNamePrefix)) { - return true; - } + return matchesValue(reference.getValue()); } return false; } + /** + * Matches the target package itself and, when non-recursive, types declared directly in it. + * Subpackages and the types in them match only when recursive. + */ + boolean matchesValue(String value) { + if (targetPackage == null) { + return false; + } + if (value.equals(targetPackage)) { + return true; + } + if (!value.startsWith(targetPackage + ".") || value.length() <= targetPackage.length() + 1) { + return false; + } + return recursive || Character.isUpperCase(value.charAt(targetPackage.length() + 1)); + } + @Override public Reference.Renamer createRenamer(String newName) { return reference -> getReplacement(reference.getValue(), targetPackage, newName); diff --git a/rewrite-java/src/main/resources/META-INF/rewrite/recipes.csv b/rewrite-java/src/main/resources/META-INF/rewrite/recipes.csv index db51e96a643..4fcb69ad50a 100644 --- a/rewrite-java/src/main/resources/META-INF/rewrite/recipes.csv +++ b/rewrite-java/src/main/resources/META-INF/rewrite/recipes.csv @@ -14,7 +14,7 @@ maven,org.openrewrite:rewrite-java,org.openrewrite.java.ChangeMethodInvocationRe maven,org.openrewrite:rewrite-java,org.openrewrite.java.ChangeMethodName,Change method name,Rename a method.,1,,Java,,Basic building blocks for transforming Java code.,"[{""name"":""methodPattern"",""type"":""String"",""displayName"":""Method pattern"",""description"":""A [method pattern](https://docs.openrewrite.org/reference/method-patterns) is used to find matching method invocations. For example, to find all method invocations in the Guava library, use the pattern: `com.google.common..*#*(..)`.

The pattern format is `#()`.

`..*` includes all subpackages of `com.google.common`.
`*(..)` matches any method name with any number of arguments.

For more specific queries, like Guava's `ImmutableMap`, use `com.google.common.collect.ImmutableMap#*(..)` to narrow down the results."",""example"":""org.mockito.Matchers anyVararg()"",""required"":true},{""name"":""newMethodName"",""type"":""String"",""displayName"":""New method name"",""description"":""The method name that will replace the existing name."",""example"":""any"",""required"":true},{""name"":""matchOverrides"",""type"":""Boolean"",""displayName"":""Match on overrides"",""description"":""When enabled, find methods that are overrides of the method pattern.""},{""name"":""ignoreDefinition"",""type"":""Boolean"",""displayName"":""Ignore type definition"",""description"":""When set to `true` the definition of the old type will be left untouched. This is useful when you're replacing usage of a class but don't want to rename it.""}]", maven,org.openrewrite:rewrite-java,org.openrewrite.java.ChangeMethodTargetToStatic,Change method target to static,Change method invocations to static method calls.,1,,Java,,Basic building blocks for transforming Java code.,"[{""name"":""methodPattern"",""type"":""String"",""displayName"":""Method pattern"",""description"":""The original method call may or may not be a static method invocation. A [method pattern](https://docs.openrewrite.org/reference/method-patterns) is used to find matching method invocations. For example, to find all method invocations in the Guava library, use the pattern: `com.google.common..*#*(..)`.

The pattern format is `#()`.

`..*` includes all subpackages of `com.google.common`.
`*(..)` matches any method name with any number of arguments.

For more specific queries, like Guava's `ImmutableMap`, use `com.google.common.collect.ImmutableMap#*(..)` to narrow down the results."",""example"":""com.google.common.collect.ImmutableSet of(..)"",""required"":true},{""name"":""fullyQualifiedTargetTypeName"",""type"":""String"",""displayName"":""Fully-qualified target type name"",""description"":""A fully-qualified class name of the type upon which the static method is defined."",""example"":""java.util.Set"",""required"":true},{""name"":""returnType"",""type"":""String"",""displayName"":""Return type after change"",""description"":""Sometimes changing the target type also changes the return type. In the Guava example, changing from `ImmutableSet#of(..)` to `Set#of(..)` widens the return type from Guava's `ImmutableSet` to just `java.util.Set`."",""example"":""java.util.Set""},{""name"":""matchOverrides"",""type"":""Boolean"",""displayName"":""Match on overrides"",""description"":""When enabled, find methods that are overrides of the method pattern.""},{""name"":""matchUnknownTypes"",""type"":""Boolean"",""displayName"":""Match unknown types"",""description"":""When enabled, include method invocations which appear to match if full type information is missing. Using matchUnknownTypes can improve recipe resiliency for an AST with missing type information, but also increases the risk of false-positive matches on unrelated method invocations.""}]", maven,org.openrewrite:rewrite-java,org.openrewrite.java.ChangeMethodTargetToVariable,Change method target to variable,Change method invocations to method calls on a variable.,1,,Java,,Basic building blocks for transforming Java code.,"[{""name"":""methodPattern"",""type"":""String"",""displayName"":""Method pattern"",""description"":""A [method pattern](https://docs.openrewrite.org/reference/method-patterns) is used to find matching method invocations. For example, to find all method invocations in the Guava library, use the pattern: `com.google.common..*#*(..)`.

The pattern format is `#()`.

`..*` includes all subpackages of `com.google.common`.
`*(..)` matches any method name with any number of arguments.

For more specific queries, like Guava's `ImmutableMap`, use `com.google.common.collect.ImmutableMap#*(..)` to narrow down the results."",""example"":""org.mycorp.A method(..)"",""required"":true},{""name"":""variableName"",""type"":""String"",""displayName"":""Variable name"",""description"":""Name of variable to use as target for the modified method invocation."",""example"":""foo"",""required"":true},{""name"":""variableType"",""type"":""String"",""displayName"":""Variable type"",""description"":""Type attribution to use for the return type of the modified method invocation."",""example"":""java.lang.String"",""required"":true},{""name"":""matchOverrides"",""type"":""Boolean"",""displayName"":""Match on overrides"",""description"":""When enabled, find methods that are overrides of the method pattern.""}]", -maven,org.openrewrite:rewrite-java,org.openrewrite.java.ChangePackage,Rename package name,"A recipe that will rename a package name in package statements, imports, and fully-qualified types.",1,,Java,,Basic building blocks for transforming Java code.,"[{""name"":""oldPackageName"",""type"":""String"",""displayName"":""Old package name"",""description"":""The package name to replace."",""example"":""com.yourorg.foo"",""required"":true},{""name"":""newPackageName"",""type"":""String"",""displayName"":""New package name"",""description"":""New package name to replace the old package name with."",""example"":""com.yourorg.bar"",""required"":true},{""name"":""recursive"",""type"":""Boolean"",""displayName"":""Recursive"",""description"":""Recursively change subpackage names""}]", +maven,org.openrewrite:rewrite-java,org.openrewrite.java.ChangePackage,Rename package name,"A recipe that will rename a package name in package statements, imports, and fully-qualified types.",1,,Java,,Basic building blocks for transforming Java code.,"[{""name"":""oldPackageName"",""type"":""String"",""displayName"":""Old package name"",""description"":""The package name to replace."",""example"":""com.yourorg.foo"",""required"":true},{""name"":""newPackageName"",""type"":""String"",""displayName"":""New package name"",""description"":""New package name to replace the old package name with."",""example"":""com.yourorg.bar"",""required"":true},{""name"":""recursive"",""type"":""Boolean"",""displayName"":""Recursive"",""description"":""Recursively change subpackage names. Defaults to `false`, renaming only types directly in `oldPackageName`; set to `true` to also rename types in its subpackages.""}]", maven,org.openrewrite:rewrite-java,org.openrewrite.java.ChangePackageInStringLiteral,Rename package name in String literals,A recipe that will rename a package name in String literals.,1,,Java,,Basic building blocks for transforming Java code.,"[{""name"":""oldPackageName"",""type"":""String"",""displayName"":""Old package name"",""description"":""The package name to replace."",""example"":""com.yourorg.foo"",""required"":true},{""name"":""newPackageName"",""type"":""String"",""displayName"":""New package name"",""description"":""New package name to replace the old package name with."",""example"":""com.yourorg.bar"",""required"":true}]", maven,org.openrewrite:rewrite-java,org.openrewrite.java.ChangeStaticFieldToMethod,Change static field access to static method access,Migrate accesses to a static field to invocations of a static method.,1,,Java,,Basic building blocks for transforming Java code.,"[{""name"":""oldClassName"",""type"":""String"",""displayName"":""Old class name"",""description"":""The fully qualified name of the class containing the field to replace."",""example"":""java.util.Collections"",""required"":true},{""name"":""oldFieldName"",""type"":""String"",""displayName"":""Old field name"",""description"":""The simple name of the static field to replace."",""example"":""EMPTY_LIST"",""required"":true},{""name"":""newClassName"",""type"":""String"",""displayName"":""New class name"",""description"":""The fully qualified name of the class containing the method to use. Leave empty to keep the same class."",""example"":""java.util.List""},{""name"":""newTarget"",""type"":""String"",""displayName"":""New field target"",""description"":""An optional method target that can be used to specify a static field within the new class."",""example"":""OK_RESPONSE""},{""name"":""newMethodName"",""type"":""String"",""displayName"":""New method name"",""description"":""The simple name of the method to use. The method must be static and have no arguments."",""example"":""of"",""required"":true}]", maven,org.openrewrite:rewrite-java,org.openrewrite.java.ChangeType,Change type,Change a given type to another.,1,,Java,,Basic building blocks for transforming Java code.,"[{""name"":""oldFullyQualifiedTypeName"",""type"":""String"",""displayName"":""Old fully-qualified type name"",""description"":""Fully-qualified class name of the original type."",""example"":""org.junit.Assume"",""required"":true},{""name"":""newFullyQualifiedTypeName"",""type"":""String"",""displayName"":""New fully-qualified type name"",""description"":""Fully-qualified class name of the replacement type, or the name of a primitive such as \""int\"". The `OuterClassName$NestedClassName` naming convention should be used for nested classes."",""example"":""org.junit.jupiter.api.Assumptions"",""required"":true},{""name"":""ignoreDefinition"",""type"":""Boolean"",""displayName"":""Ignore type definition"",""description"":""When set to `true` the definition of the old type will be left untouched. This is useful when you're replacing usage of a class but don't want to rename it.""}]", From 240bb740aad5cc0e9a8c5960b07d6211d0ead92b Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Tue, 4 Aug 2026 23:02:24 +0200 Subject: [PATCH 2/3] Collapse the duplicated type-vs-subpackage convention into one helper --- .../org/openrewrite/java/PackageMatcher.java | 22 ++++++++++++------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/rewrite-java/src/main/java/org/openrewrite/java/PackageMatcher.java b/rewrite-java/src/main/java/org/openrewrite/java/PackageMatcher.java index dbf9eb23bfe..cb4fd7ecf5b 100644 --- a/rewrite-java/src/main/java/org/openrewrite/java/PackageMatcher.java +++ b/rewrite-java/src/main/java/org/openrewrite/java/PackageMatcher.java @@ -52,13 +52,19 @@ boolean matchesValue(String value) { if (targetPackage == null) { return false; } - if (value.equals(targetPackage)) { - return true; - } - if (!value.startsWith(targetPackage + ".") || value.length() <= targetPackage.length() + 1) { - return false; - } - return recursive || Character.isUpperCase(value.charAt(targetPackage.length() + 1)); + return value.equals(targetPackage) || + value.startsWith(targetPackage + ".") && (recursive || namesTypeDirectlyIn(value, targetPackage)); + } + + /** + * Whether the segment following {@code pkg} names a type rather than a subpackage, inferred from + * a leading capital. A dotted string alone cannot say where the package ends, so this convention + * is what the reference model already uses to tell {@link Reference.Kind#TYPE} from + * {@link Reference.Kind#PACKAGE} when the providers in rewrite-properties, rewrite-yaml, + * rewrite-xml and the service-provider reader build these references in the first place. + */ + private boolean namesTypeDirectlyIn(String value, String pkg) { + return value.length() > pkg.length() + 1 && Character.isUpperCase(value.charAt(pkg.length() + 1)); } @Override @@ -71,7 +77,7 @@ String getReplacement(String value, @Nullable String oldValue, String newValue) if (value.equals(oldValue)) { return newValue; } else if (value.startsWith(oldValue)) { - if (recursive || value.length() > oldValue.length() + 1 && Character.isUpperCase(value.charAt(oldValue.length() + 1))) { + if (recursive || namesTypeDirectlyIn(value, oldValue)) { return newValue + value.substring(oldValue.length()); } } From 5c3bdcb6c0654ec3886e748624a89a85ed5313af Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Wed, 5 Aug 2026 12:29:59 +0200 Subject: [PATCH 3/3] Address review: flatten the field-access guards, condense the tests `qualifiesTypeDirectlyInOldPackage` answered a question its early return could not sensibly answer: when nothing further qualifies the occurrence it is not "a type directly in the old package", it is a bare reference to `oldPackageName` itself. Flatten `visitFieldAccess` into guard clauses so the parent is destructured once and each bail-out carries its reason, and let the helper only ever answer about a real enclosing `J.FieldAccess`. The six tests were three shapes across two source kinds. Since `rewriteRun` takes several sources, the subpackage-only versus mixed contrast -- the asymmetry this pins -- now sits in a single run instead of being split across two tests that had to be read side by side. Same coverage, two tests. --- .../openrewrite/java/ChangePackageTest.java | 77 ++++++------------- .../org/openrewrite/java/ChangePackage.java | 45 ++++++----- 2 files changed, 49 insertions(+), 73 deletions(-) diff --git a/rewrite-java-test/src/test/java/org/openrewrite/java/ChangePackageTest.java b/rewrite-java-test/src/test/java/org/openrewrite/java/ChangePackageTest.java index 917dfb504ae..ec5cd0474a9 100644 --- a/rewrite-java-test/src/test/java/org/openrewrite/java/ChangePackageTest.java +++ b/rewrite-java-test/src/test/java/org/openrewrite/java/ChangePackageTest.java @@ -597,10 +597,11 @@ class NullRecursiveDefaultsToNonRecursive { ); @Test - void javaSourceWithOnlySubpackageReference() { + void nullRecursiveIsNonRecursiveForEverySourceKind() { rewriteRun( spec -> spec.recipe(new ChangePackage("cucumber.api.java", "io.cucumber.java", null)) .parser(cucumber), + // Only a subpackage type: untouched. java( """ import cucumber.api.java.en.Given; @@ -610,21 +611,14 @@ class A { void given() {} } """ - ) - ); - } - - @Test - void javaSourceWithSubpackageAndDirectReference() { - rewriteRun( - spec -> spec.recipe(new ChangePackage("cucumber.api.java", "io.cucumber.java", null)) - .parser(cucumber), + ), + // Both: the type directly in the old package moves, the subpackage one stays put. java( """ import cucumber.api.java.Before; import cucumber.api.java.en.Given; - class A { + class B { @Before void before() {} @@ -636,7 +630,7 @@ void given() {} import io.cucumber.java.Before; import cucumber.api.java.en.Given; - class A { + class B { @Before void before() {} @@ -644,12 +638,29 @@ void before() {} void given() {} } """ + ), + properties( + """ + given=cucumber.api.java.en.Given + """, + spec -> spec.path("application.properties") + ), + properties( + """ + before=cucumber.api.java.Before + given=cucumber.api.java.en.Given + """, + """ + before=io.cucumber.java.Before + given=cucumber.api.java.en.Given + """, + spec -> spec.path("application-extra.properties") ) ); } @Test - void javaSourceWithOnlySubpackageReferenceOptingIn() { + void recursiveOptsIntoSubpackagesForEverySourceKind() { rewriteRun( spec -> spec.recipe(new ChangePackage("cucumber.api.java", "io.cucumber.java", true)) .parser(cucumber), @@ -670,45 +681,7 @@ class A { void given() {} } """ - ) - ); - } - - @Test - void referenceSourceWithOnlySubpackageReference() { - rewriteRun( - spec -> spec.recipe(new ChangePackage("cucumber.api.java", "io.cucumber.java", null)), - properties( - """ - given=cucumber.api.java.en.Given - """, - spec -> spec.path("application.properties") - ) - ); - } - - @Test - void referenceSourceWithSubpackageAndDirectReference() { - rewriteRun( - spec -> spec.recipe(new ChangePackage("cucumber.api.java", "io.cucumber.java", null)), - properties( - """ - before=cucumber.api.java.Before - given=cucumber.api.java.en.Given - """, - """ - before=io.cucumber.java.Before - given=cucumber.api.java.en.Given - """, - spec -> spec.path("application.properties") - ) - ); - } - - @Test - void referenceSourceWithOnlySubpackageReferenceOptingIn() { - rewriteRun( - spec -> spec.recipe(new ChangePackage("cucumber.api.java", "io.cucumber.java", true)), + ), properties( """ given=cucumber.api.java.en.Given diff --git a/rewrite-java/src/main/java/org/openrewrite/java/ChangePackage.java b/rewrite-java/src/main/java/org/openrewrite/java/ChangePackage.java index e20bbd86e3d..112f532a056 100644 --- a/rewrite-java/src/main/java/org/openrewrite/java/ChangePackage.java +++ b/rewrite-java/src/main/java/org/openrewrite/java/ChangePackage.java @@ -168,36 +168,39 @@ private class JavaChangePackageVisitor extends JavaVisitor { public J visitFieldAccess(J.FieldAccess fieldAccess, ExecutionContext ctx) { J f = super.visitFieldAccess(fieldAccess, ctx); - if (((J.FieldAccess) f).isFullyQualifiedClassReference(oldPackageName)) { - Cursor parent = getCursor().getParent(); - if (parent != null && - // Ensure the parent isn't a J.FieldAccess OR the parent doesn't match the target package name. - (!(parent.getValue() instanceof J.FieldAccess) || - (!(((J.FieldAccess) parent.getValue()).isFullyQualifiedClassReference(newPackageName)))) && - (isRecursive() || qualifiesTypeDirectlyInOldPackage(parent))) { - - f = TypeTree.build(((JavaType.FullyQualified) newPackageType).getFullyQualifiedName()) - .withPrefix(f.getPrefix()); + if (!((J.FieldAccess) f).isFullyQualifiedClassReference(oldPackageName)) { + return f; + } + Cursor parent = getCursor().getParent(); + if (parent == null) { + return f; + } + if (parent.getValue() instanceof J.FieldAccess) { + J.FieldAccess enclosing = (J.FieldAccess) parent.getValue(); + if (enclosing.isFullyQualifiedClassReference(newPackageName)) { + // Already rewritten to the new package. + return f; + } + if (!isRecursive() && !namesTypeDirectlyInOldPackage(enclosing)) { + // Leading segments of a subpackage-qualified name such as oldPackageName.sub.Type. + return f; } } - return f; + return TypeTree.build(((JavaType.FullyQualified) newPackageType).getFullyQualifiedName()) + .withPrefix(f.getPrefix()); } /** - * Whether this occurrence of {@code oldPackageName} qualifies a type declared directly in it, - * as opposed to being the leading segments of a subpackage-qualified name such as - * {@code oldPackageName.sub.Type}. Only the former may be renamed when non-recursive. + * Whether the name enclosing this occurrence of {@code oldPackageName} is a type declared + * directly in it, rather than a subpackage. Uses the same leading-capital convention as + * {@link PackageMatcher} where type attribution is unavailable. */ - private boolean qualifiesTypeDirectlyInOldPackage(Cursor parent) { - if (!(parent.getValue() instanceof J.FieldAccess)) { - return true; - } - J.FieldAccess qualified = (J.FieldAccess) parent.getValue(); - JavaType.FullyQualified fq = TypeUtils.asFullyQualified(qualified.getType()); + private boolean namesTypeDirectlyInOldPackage(J.FieldAccess enclosing) { + JavaType.FullyQualified fq = TypeUtils.asFullyQualified(enclosing.getType()); if (fq != null) { return oldPackageName.equals(fq.getPackageName()); } - String nextSegment = qualified.getSimpleName(); + String nextSegment = enclosing.getSimpleName(); return "*".equals(nextSegment) || !nextSegment.isEmpty() && Character.isUpperCase(nextSegment.charAt(0)); }