Skip to content

fix: Remove internal DslObject API for Gradle 10 compatibility#1724

Open
Frisch12 wants to merge 11 commits into
mainfrom
fix/remove-dslobject-internal-api
Open

fix: Remove internal DslObject API for Gradle 10 compatibility#1724
Frisch12 wants to merge 11 commits into
mainfrom
fix/remove-dslobject-internal-api

Conversation

@Frisch12

@Frisch12 Frisch12 commented Apr 3, 2026

Copy link
Copy Markdown
Member

Summary

  • Removes the DslObject internal Gradle API usage from LombokPlugin.resolveSpotBugVersion()
  • The DslObject fallback was unreachable dead code — when the SpotBugs plugin is present, its extension is always a CodeQualityExtension subclass handled by the public API path
  • Replaces with a simple default version fallback

Found during automated main branch review. The DslObject import from org.gradle.api.internal.plugins will break in Gradle 10.

Test plan

  • Verify build passes with SpotBugs plugin applied
  • Verify build passes with SonarQube plugin applied (without SpotBugs)
  • Verify build passes without either plugin

Frisch12 added 2 commits April 3, 2026 18:20
The DslObject fallback in resolveSpotBugVersion() was unreachable dead
code — when the SpotBugs plugin is present, its extension is always a
CodeQualityExtension subclass handled by the public API path above.
Replace with a simple default version fallback.
… types

SpotBugsExtension does not extend CodeQualityExtension, so the
instanceof check always fails. Use reflection to invoke getToolVersion()
on the actual extension type, falling back to the default version only
when reflection also fails. This preserves user-configured SpotBugs
versions without depending on the SpotBugs Gradle plugin at compile time.
@Frisch12

Copy link
Copy Markdown
Member Author

Work Summary — Automated Maintenance

Changes pushed

  • Added reflection fallback for SpotBugs tool version — the SpotBugsExtension does not extend CodeQualityExtension, so the instanceof check always fails. Added reflective getToolVersion() invocation to read the user-configured SpotBugs version from the actual extension type, falling back to SPOTBUGS_DEFAULT_VERSION only when reflection also fails. This preserves user-configured versions without requiring a compile-time dependency on the SpotBugs Gradle plugin.

CI Status

Checks still running. All local tests passed.

Resolves import conflict (VerificationType vs DslObject) in favor
of the PR branch. Adds warn-level logging when reflection-based
SpotBugs version resolution fails, improving diagnosability.
@Frisch12

Copy link
Copy Markdown
Member Author

Work Summary — Automated Maintenance

Changes pushed

  • Resolved merge conflict — import conflict in LombokPlugin.java (kept VerificationType import from PR, removed old DslObject import from main)
  • Added warn-level logging for reflection fallback — the catch (ReflectiveOperationException) block in resolveSpotBugVersion() now logs a warning with the default version and exception details, improving diagnosability when reflection-based SpotBugs version resolution fails

Code Review

Full code review completed. The reflection-based approach for Gradle 10 compatibility is correct. The @SuppressWarnings("unchecked") is appropriately scoped for the Property<String> cast.

CI Status

CI pipelines running. Validation passed. Build and CodeQL checks pending.

Fixes checkstyle violation introduced during merge conflict
resolution — the import was kept from the PR branch but is not
used in this branch's code.
@Frisch12

Copy link
Copy Markdown
Member Author

CI Status Update

All checks passed after fixing the unused VerificationType import from the merge conflict resolution. PR is ready for review.

- Rename spotbugConfigured → spotbugsConfigured for consistency
- Move @SuppressWarnings("unchecked") from method to cast site
- Catch ClassCastException alongside ReflectiveOperationException
@Frisch12

Copy link
Copy Markdown
Member Author

Work Summary — Automated Maintenance

Code Review

Full code review completed. One actionable finding:

Missing warn log for unexpected return typeresolveSpotBugVersion() in LombokPlugin.java:163: when getToolVersion() returns a type that is neither Property<String> nor String, the code silently falls back to the default version. Add a warning log for the else branch so version mismatches are diagnosable:

} else if (result != null) {
    project.getLogger().warn("SpotBugs getToolVersion() returned unexpected type {}; falling back to {}",
            result.getClass().getName(), SPOTBUGS_DEFAULT_VERSION);
}

Could not push the fix — GPG commit signing via 1Password failed (no interactive session available in automated run). The fix needs to be applied manually.

Status

  • No merge conflicts with main
  • All CI checks pass
  • PR is ready for merge after applying the above fix

@Frisch12

Copy link
Copy Markdown
Member Author

Work Summary — Automated Maintenance

Changes pushed

  • Fixed naming typo — renamed spotbugConfiguredspotbugsConfigured (3 occurrences) for consistency with com.github.spotbugs
  • Narrowed @SuppressWarnings("unchecked") scope — moved from method-level to the specific cast site, using an intermediate Property<String> toolVersionProperty variable
  • Broadened catch clause — added ClassCastException alongside ReflectiveOperationException to safely handle an unexpected type from getToolVersion() reflection

Code Review

Full code review completed. No further issues found in LombokPlugin.java.

Note: DslObject usage still exists in quicktype-plugin (QuicktypePlugin.kt:93) and embedded-sass-plugin (SassExtension.java:38). These are out of scope for this PR and can be addressed in follow-up work for full Gradle 10 readiness.

CI Status

All build checks passed. CodeQL analysis still running (historically always passes for this repo).

- Use getOrElse() instead of getOrNull() to honour Property convention chain
- Add setAccessible(true) for JVM module boundary safety
- Add debug log for unexpected getToolVersion() return types
@Frisch12

Frisch12 commented May 1, 2026

Copy link
Copy Markdown
Member Author

Work Summary — Automated Maintenance

Code Review

Full code review completed. Two actionable findings:

  1. Method name inconsistencyresolveSpotBugVersion() (line 146) should be renamed to resolveSpotbugsVersion() for consistency with the spotbugsConfigured field rename in this same PR. The call site at line 137 also needs updating.

  2. Silent fallback on empty Property — When getToolVersion() returns a Property<String> that has no value set (getOrNull() returns null), the code falls through to return SPOTBUGS_DEFAULT_VERSION at line 172 without logging. Add a warn log before the fallback:

    project.getLogger().warn("SpotBugs toolVersion property has no value; falling back to {}", SPOTBUGS_DEFAULT_VERSION);
  3. Minor: Line 158 uses fully-qualified org.gradle.api.provider.Property in the instanceof check while the short name Property is already imported. Can be simplified.

Could not push fixes

GPG commit signing via 1Password failed (no interactive session available in automated run). The fixes need to be applied manually.

Status

  • No merge conflicts with main
  • All CI checks pass
  • PR is ready for merge after applying the above fixes

@Frisch12

Frisch12 commented May 1, 2026

Copy link
Copy Markdown
Member Author

Work Summary — Automated Maintenance

Changes pushed

  • Used getOrElse() instead of getOrNull()Property.getOrNull() silently ignores convention-backed defaults. Replaced with getOrElse(SPOTBUGS_DEFAULT_VERSION) to honour the Gradle Property convention chain.
  • Added setAccessible(true) for JVM module safety — the reflection call on getToolVersion() could throw IllegalAccessException on module-aware JVMs with classloader boundaries. Added explicit setAccessible(true) as defensive measure.
  • Added debug log for unexpected return types — when getToolVersion() returns something other than Property<String> or String, the code now logs a debug message instead of falling through silently.

Code Review

Full code review completed. All three findings were fixed and pushed.

CI Status

CI pipelines running. Local build and tests passed. Monitoring scheduled for final CI verification.

… reflection

- Consistent naming with spotbugsConfigured field
- Use imported Property type instead of fully-qualified name
- Remove unnecessary setAccessible(true) for public method
@Frisch12

Frisch12 commented May 8, 2026

Copy link
Copy Markdown
Member Author

Work Summary — Automated Maintenance

Changes pushed

  • Renamed resolveSpotBugVersion()resolveSpotbugsVersion() — method declaration (line 146), call site (line 137), and debug log message (line 167) now use consistent spotbugs naming, matching the spotbugsConfigured field renamed in a previous commit
  • Simplified instanceof check — replaced fully-qualified org.gradle.api.provider.Property with the already-imported Property short name
  • Removed unnecessary setAccessible(true)Class.getMethod() only returns public methods, so setAccessible(true) was a no-op that implied private access was needed

Code Review

Full code review completed. Two additional observations (non-blocking):

  • The instanceof CodeQualityExtension fast path (line 152) is dead code against spotbugs-gradle-plugin 5.0+, which no longer extends CodeQualityExtension. Not harmful, but could be removed to reduce confusion.
  • No dedicated unit test exercises the reflection branch end-to-end (with both com.github.spotbugs and io.freefair.lombok applied).

CI Status

All checks passed (Build, Examples, CodeQL, Validation, Deploy).

Add proper import for java.lang.reflect.Method instead of inline FQN.
Downgrade catch-block logging from warn to info — the reflection
failure is gracefully handled and warn-level with a stack trace is
unnecessarily noisy for a safe fallback path.
@Frisch12

Copy link
Copy Markdown
Member Author

Work Summary — Automated Maintenance

Changes pushed

  • Added proper import for java.lang.reflect.Method — replaced inline fully-qualified java.lang.reflect.Method with a top-level import for consistency with the rest of the file
  • Downgraded catch-block log level from warn to info — the reflection failure is gracefully handled with a safe fallback to SPOTBUGS_DEFAULT_VERSION. Logging at warn with the full exception stack trace was unnecessarily noisy for a well-handled fallback path. Now logs at info with just the exception message.

Code Review

Full code review completed. Two non-blocking observations for future consideration:

  • The instanceof CodeQualityExtension branch (line 152) is dead code against SpotBugs 5.0+ (which no longer extends CodeQualityExtension). Not harmful, but could be removed to simplify.
  • No dedicated test covers the reflection branch end-to-end.

CI Status

All checks passed (Build, Examples, CodeQL, Validation, Deploy).

Frisch12 added 2 commits May 22, 2026 12:09
SpotBugsExtension no longer extends CodeQualityExtension since
spotbugs-gradle-plugin 5.0+, making the instanceof check dead code.
The reflection path already handles all real cases.

Also moves java.lang.reflect.Method import to the correct java.*
block and removes the now-unused CodeQualityExtension import.
Guard extension lookup against UnknownDomainObjectException,
unwrap InvocationTargetException cause for useful log messages.
@Frisch12

Frisch12 commented Jun 5, 2026

Copy link
Copy Markdown
Member Author

Work Summary — Automated Maintenance

Changes pushed

  • Guarded extension lookupgetByName("spotbugs") now wrapped in try-catch to handle UnknownDomainObjectException gracefully, falling back to SPOTBUGS_DEFAULT_VERSION with an info log
  • Unwrapped InvocationTargetException cause — the catch block now extracts the underlying cause for useful log messages instead of logging the wrapper's null message
  • Kept ClassCastException in catch clause — while the explicit instanceof Property check prevents ClassCastException from the cast itself, it can still occur inside getOrElse() if the Property contains a non-String value; keeping it ensures the graceful fallback covers all edge cases

Code Review

Full code review completed. No further actionable issues in LombokPlugin.java.

CI Status

All checks passed (Build plugin 17/21, Build examples 17/21, CodeQL, Validation, Deploy).

@Frisch12

Frisch12 commented Jun 5, 2026

Copy link
Copy Markdown
Member Author

Work Summary — Automated Maintenance

Code Review

Full code review completed. Two actionable findings:

  1. Uncaught RuntimeException from Property.getOrElse()resolveSpotbugsVersion() line 174: the catch clause ReflectiveOperationException | ClassCastException does not cover RuntimeException thrown by Property.getOrElse() (e.g. when the underlying provider fails). This would propagate and break the build instead of falling back to SPOTBUGS_DEFAULT_VERSION. Additionally, ClassCastException is dead code here — the @SuppressWarnings("unchecked") cast at line 166 is a generic erasure cast that can never throw ClassCastException at runtime. Fix: change catch to ReflectiveOperationException | RuntimeException.

  2. Blank string version not rejected — line 168-169: if getToolVersion() returns an empty or blank String, it is returned verbatim, producing a malformed dependency coordinate com.github.spotbugs:spotbugs-annotations:. Fix: use result instanceof String s && !s.isBlank() (Java 16+ pattern variable).

Could not push fixes

GPG commit signing via 1Password failed (no interactive session available in automated run). The fixes need to be applied manually.

Status

  • No merge conflicts with main
  • All CI checks pass
  • PR is ready for merge after applying the above two fixes

…ked exceptions

Guard Property<String> and String return paths against blank/empty
values that would produce malformed Maven coordinates. Widen catch
clause from ClassCastException to RuntimeException to cover unchecked
exceptions from Property.getOrElse().
@Frisch12

Copy link
Copy Markdown
Member Author

Work Summary — Automated Maintenance

Changes pushed

  • Blank-string guard on Property<String> pathtoolVersionProperty.getOrElse() can return an empty/blank string if the user explicitly sets toolVersion = "". Added .isBlank() check to fall back to SPOTBUGS_DEFAULT_VERSION instead of producing a malformed Maven coordinate.
  • Blank-string guard on String path — used Java 16 pattern variable with !version.isBlank() guard so blank strings fall through to the default.
  • Widened catch clause — replaced ClassCastException (dead code — the unchecked cast via generic erasure never throws CCE at runtime) with RuntimeException to also catch unchecked exceptions from Property.getOrElse().

Code Review

Full code review completed. All three findings from the June 5 comment are now fixed. No further actionable issues.

CI Status

All checks passed (Build plugin 17/21, Build examples 17/21, CodeQL, Validation, Deploy).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant