chore: remove the JavaPoet dependency - #3556
Conversation
📝 WalkthroughWalkthroughThe annotation processor now records resolved resource types with compiler APIs. Maven no longer declares JavaPoet. Compilation tests verify mappings for standard hierarchies and generic resources. ChangesAnnotation processor cleanup
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@operator-framework/src/main/java/io/javaoperatorsdk/operator/config/runtime/ControllerConfigurationAnnotationProcessor.java`:
- Line 104: Remove the explanatory comment above the type conversion in
ControllerConfigurationAnnotationProcessor.java (lines 104-104) and remove the
assertMapping Javadoc in ControllerConfigurationAnnotationProcessorTest.java
(lines 73-77); retain the underlying logic unchanged.
In
`@operator-framework/src/test/java/io/javaoperatorsdk/operator/config/runtime/ControllerConfigurationAnnotationProcessorTest.java`:
- Line 27: Update the import of RECONCILERS_RESOURCE_PATH in
ControllerConfigurationAnnotationProcessorTest to comply with the 100-character
limit by running Spotless/Google Java Format; if it remains too long, replace
the static import with RuntimeControllerMetadata and qualify the constant at its
use site.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a2d792fd-baee-4447-8aed-2f6eab3e5cb7
📒 Files selected for processing (4)
operator-framework/pom.xmloperator-framework/src/main/java/io/javaoperatorsdk/operator/config/runtime/ControllerConfigurationAnnotationProcessor.javaoperator-framework/src/test/java/io/javaoperatorsdk/operator/config/runtime/ControllerConfigurationAnnotationProcessorTest.javapom.xml
💤 Files with no reviewable changes (2)
- operator-framework/pom.xml
- pom.xml
| return; | ||
| } | ||
| final TypeName customResourceType = TypeName.get(resourceType); | ||
| // the resolved resource type is always a declared type, so its element is a TypeElement |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the added comments for short logic.
Both comments document short, direct operations. Keep comments only for very long or complex logic.
operator-framework/src/main/java/io/javaoperatorsdk/operator/config/runtime/ControllerConfigurationAnnotationProcessor.java#L104-L104: remove the explanatory comment above the type conversion.operator-framework/src/test/java/io/javaoperatorsdk/operator/config/runtime/ControllerConfigurationAnnotationProcessorTest.java#L73-L77: remove the Javadoc forassertMapping.
As per coding guidelines: do not add comments except for very long or complex logic.
📍 Affects 2 files
operator-framework/src/main/java/io/javaoperatorsdk/operator/config/runtime/ControllerConfigurationAnnotationProcessor.java#L104-L104(this comment)operator-framework/src/test/java/io/javaoperatorsdk/operator/config/runtime/ControllerConfigurationAnnotationProcessorTest.java#L73-L77
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@operator-framework/src/main/java/io/javaoperatorsdk/operator/config/runtime/ControllerConfigurationAnnotationProcessor.java`
at line 104, Remove the explanatory comment above the type conversion in
ControllerConfigurationAnnotationProcessor.java (lines 104-104) and remove the
assertMapping Javadoc in ControllerConfigurationAnnotationProcessorTest.java
(lines 73-77); retain the underlying logic unchanged.
Source: Coding guidelines
| import com.google.testing.compile.Compiler; | ||
| import com.google.testing.compile.JavaFileObjects; | ||
|
|
||
| import static io.javaoperatorsdk.operator.config.runtime.RuntimeControllerMetadata.RECONCILERS_RESOURCE_PATH; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '20,35p' operator-framework/src/test/java/io/javaoperatorsdk/operator/config/runtime/ControllerConfigurationAnnotationProcessorTest.java
python3 - <<'PY'
from pathlib import Path
path = Path("operator-framework/src/test/java/io/javaoperatorsdk/operator/config/runtime/ControllerConfigurationAnnotationProcessorTest.java")
for number, line in enumerate(path.read_text().splitlines(), 1):
if number in range(20, 35):
print(f"{number}: {len(line)} chars: {line}")
PY
rg -n "RECONCILERS_RESOURCE_PATH|RuntimeControllerMetadata" operator-framework/src/test/java/io/javaoperatorsdk/operator/config/runtime/ControllerConfigurationAnnotationProcessorTest.javaRepository: operator-framework/java-operator-sdk
Length of output: 1713
Keep the static import within the 100-character limit.
This import is 109 characters. Run Spotless with Google Java Format. If it remains too long, import RuntimeControllerMetadata and qualify RECONCILERS_RESOURCE_PATH at the use site.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@operator-framework/src/test/java/io/javaoperatorsdk/operator/config/runtime/ControllerConfigurationAnnotationProcessorTest.java`
at line 27, Update the import of RECONCILERS_RESOURCE_PATH in
ControllerConfigurationAnnotationProcessorTest to comply with the 100-character
limit by running Spotless/Google Java Format; if it remains too long, replace
the static import with RuntimeControllerMetadata and qualify the constant at its
use site.
Source: Coding guidelines
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Removes the JavaPoet dependency by using standard annotation processing APIs to resolve fully qualified resource class names, and strengthens processor tests by asserting generated mapping contents.
Changes:
- Removed JavaPoet from root and module POMs.
- Updated
ControllerConfigurationAnnotationProcessorto resolve the resource type’s qualified name viaTypeElement. - Enhanced annotation processor tests to assert the generated mapping resource content.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pom.xml | Drops JavaPoet version property and dependency from dependency management. |
| operator-framework/pom.xml | Removes the module-level JavaPoet dependency. |
| operator-framework/src/main/java/io/javaoperatorsdk/operator/config/runtime/ControllerConfigurationAnnotationProcessor.java | Replaces JavaPoet TypeName usage with TypeElement#getQualifiedName() resolution. |
| operator-framework/src/test/java/io/javaoperatorsdk/operator/config/runtime/ControllerConfigurationAnnotationProcessorTest.java | Adds assertions on the generated mapping resource file content. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // the resolved resource type is always a declared type, so its element is a TypeElement | ||
| final var customResourceType = | ||
| (TypeElement) processingEnv.getTypeUtils().asElement(resourceType); | ||
| controllersResourceWriter.add( | ||
| controllerClassSymbol.getQualifiedName().toString(), customResourceType.toString()); | ||
| controllerClassSymbol.getQualifiedName().toString(), | ||
| customResourceType.getQualifiedName().toString()); |
| private static void assertMapping(Compilation compilation, String expectedMapping) { | ||
| CompilationSubject.assertThat(compilation) | ||
| .generatedFile(StandardLocation.CLASS_OUTPUT, RECONCILERS_RESOURCE_PATH) | ||
| .contentsAsUtf8String() | ||
| .contains(expectedMapping); |
baf062a to
461088d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
operator-framework/src/test/java/io/javaoperatorsdk/operator/config/runtime/ControllerConfigurationAnnotationProcessorTest.java (1)
102-106: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAssert complete mapping records.
Use an exact line assertion instead of
contains(expectedMapping). Usesplit(",", -1)so trailing empty fields cannot passhasSize(2).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@operator-framework/src/test/java/io/javaoperatorsdk/operator/config/runtime/ControllerConfigurationAnnotationProcessorTest.java` around lines 102 - 106, Update assertMapping to assert the complete generated mapping line rather than using contains(expectedMapping), and split the relevant mapping with split(",", -1) so trailing empty fields are retained and validated with the expected field count.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@operator-framework/src/test/java/io/javaoperatorsdk/operator/config/runtime/ControllerConfigurationAnnotationProcessorTest.java`:
- Around line 135-140: Update assertLoadableMapping to validate each mapping
name through ClassMappingProvider’s runtime ClassUtils.getClass loading path,
using a classloader that includes the compilation output. Preserve the existing
syntax assertions, and ensure missing but syntactically valid class names fail
instead of passing.
---
Outside diff comments:
In
`@operator-framework/src/test/java/io/javaoperatorsdk/operator/config/runtime/ControllerConfigurationAnnotationProcessorTest.java`:
- Around line 102-106: Update assertMapping to assert the complete generated
mapping line rather than using contains(expectedMapping), and split the relevant
mapping with split(",", -1) so trailing empty fields are retained and validated
with the expected field count.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 24aad668-bf54-4a26-8a41-31b971659801
📒 Files selected for processing (2)
operator-framework/src/test/java/io/javaoperatorsdk/operator/config/runtime/ControllerConfigurationAnnotationProcessorTest.javaoperator-framework/src/test/resources/compile-fixtures/GenericResourceReconciler.java
| assertThat(name) | ||
| .as("recorded class name '%s' must be loadable at runtime", name) | ||
| .doesNotContain("<") | ||
| .doesNotContain(">") | ||
| .doesNotContain(" ") | ||
| .matches("[\\p{L}_$][\\p{L}\\p{N}_$]*(\\.[\\p{L}_$][\\p{L}\\p{N}_$]*)*"); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
file="$(git ls-files | grep -F 'operator-framework/src/test/java/io/javaoperatorsdk/operator/config/runtime/ControllerConfigurationAnnotationProcessorTest.java' | head -n 1)"
printf '%s\n' "FILE=$file"
wc -l "$file"
sed -n '1,220p' "$file"
printf '\n-- ClassMappingProvider references --\n'
rg -n -C 4 'ClassMappingProvider|assertLoadableMapping|assertMapping|loadClass|Class\.forName' operator-framework/src operator-framework/pom.xml pom.xml 2>/dev/null || trueRepository: operator-framework/java-operator-sdk
Length of output: 21788
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '-- ClassMappingProvider --'
cat -n operator-framework/src/main/java/io/javaoperatorsdk/operator/config/runtime/ClassMappingProvider.java
printf '%s\n' '-- Compile-testing dependency declarations and API usage --'
rg -n -C 3 'compile-testing|com.google.testing.compile|Compilation\.|classLoader|ClassLoader' \
operator-framework/pom.xml pom.xml operator-framework/src test 2>/dev/null || true
printf '%s\n' '-- Relevant fixture declarations --'
for f in $(git ls-files 'operator-framework/src/test/resources/compile-fixtures/*.java' \
| grep -E '(GenericResourceReconciler|MultilevelReconciler|AbstractReconciler|ReconcilerImplemented)'); do
echo "### $f"
sed -n '1,180p' "$f"
doneRepository: operator-framework/java-operator-sdk
Length of output: 16543
🌐 Web query:
com.google.testing.compile compile-testing 0.23.0 Compilation classLoader compiledClass API
💡 Result:
In Google Compile-Testing 0.23.0, the Compilation class does not expose a ClassLoader or a method to retrieve a "compiledClass" directly [1]. Instead, the Compilation class is designed to represent the outcome of a compilation process, providing access to the compilation status (success/failure), diagnostics (errors/warnings), source files, and generated files [1]. To interact with the classpath during compilation tests—such as ensuring specific libraries or classes are available to the compiler—you use methods on the Compiler class rather than the Compilation result [2][3][4]. Key methods include: - withClasspath(Iterable): Allows you to explicitly provide a list of files or JARs as the classpath [3]. - withClasspathFrom(ClassLoader): Allows you to derive the compilation classpath from an existing ClassLoader (e.g., this.getClass.getClassLoader), which is useful for making classes available to the test compilation that are already on your test runtime classpath [3][4]. The Compilation class itself focuses on verification of the output, typically used in conjunction with Truth assertions (e.g., assertThat(compilation).succeeded) to validate that the compilation behaved as expected [1][3]. If you need to load and inspect classes generated by the compilation, that typically falls outside the primary scope of the Compile-Testing library's assertions, which are centered on verifying compiler behavior and diagnostics [5][1].
Citations:
- 1: https://github.com/google/compile-testing/blob/master/src/main/java/com/google/testing/compile/Compilation.java
- 2: https://github.com/google/compile-testing/blob/master/src/main/java/com/google/testing/compile/Compiler.java
- 3: https://github.com/google/compile-testing/blob/master/src/test/java/com/google/testing/compile/CompilerTest.java
- 4: https://stackoverflow.com/questions/66185065/how-to-provide-some-library-from-maven-to-google-compile-testing
- 5: https://github.com/google/compile-testing
Resolve each mapping name with the runtime loading path.
assertLoadableMapping checks only syntax. It does not call ClassMappingProvider’s ClassUtils.getClass path, so a valid-looking but missing class name can pass. Use a classloader that includes the compilation output, or rename the helper to indicate syntax-only validation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@operator-framework/src/test/java/io/javaoperatorsdk/operator/config/runtime/ControllerConfigurationAnnotationProcessorTest.java`
around lines 135 - 140, Update assertLoadableMapping to validate each mapping
name through ClassMappingProvider’s runtime ClassUtils.getClass loading path,
using a classloader that includes the compilation output. Preserve the existing
syntax assertions, and ensure missing but syntactically valid class names fail
instead of passing.
fs-alecbg
left a comment
fs-alecbg
left a comment
There was a problem hiding this comment.
Looks great to me! Thanks for doing this so quickly
An integration test did fail, https://github.com/operator-framework/java-operator-sdk/actions/runs/31527708308/job/93899970031?pr=3556. It looks to me like this is a flaky test and is unrelated to these changes.
JavaPoet was only used in ControllerConfigurationAnnotationProcessor to turn the resolved resource TypeMirror into its fully qualified name, which the standard annotation processing API can do on its own: the resolved type is always a declared type, so its element is a TypeElement and its qualified name is exactly the dot separated name ClassMappingProvider loads at runtime. JavaPoet has not been released since 2024, which makes it unusable for users whose organizations do not approve unmaintained dependencies. Also assert the content of the generated mapping resource in the processor tests, which previously only checked that compilation succeeded and so could not have caught a wrong resource class name.
ea18649 to
c72eb81
Compare
JavaPoet was only used in ControllerConfigurationAnnotationProcessor to turn
the resolved resource TypeMirror into its fully qualified name, which the
standard annotation processing API can do on its own: the resolved type is
always a declared type, so its element is a TypeElement and its qualified name
is exactly the dot separated name ClassMappingProvider loads at runtime.
JavaPoet has not been released since 2024, which makes it unusable for users
whose organizations do not approve unmaintained dependencies.
Also assert the content of the generated mapping resource in the processor
tests, which previously only checked that compilation succeeded and so could
not have caught a wrong resource class name.
Summary by CodeRabbit
Bug Fixes
Tests
Chores