Skip to content

Make JDK 17 the minimum Java version#4

Open
greggdonovan wants to merge 31 commits intomasterfrom
claude/java-drop-eol-qlAJz
Open

Make JDK 17 the minimum Java version#4
greggdonovan wants to merge 31 commits intomasterfrom
claude/java-drop-eol-qlAJz

Conversation

@greggdonovan
Copy link
Owner

  • Update lib/java/build.gradle to require JDK 17 minimum
  • Update lib/java/gradle/sourceConfiguration.gradle to target Java 17
  • Update contrib/thrift-maven-plugin/pom.xml to use Java 17
  • Update .github/workflows/build.yml cross-test to use JDK 17
  • Update lib/kotlin/*.gradle.kts files to target JVM 17
  • Add Jdk17Test.java to verify JDK 17 runtime features
  • Apply JDK 17 features:
    • Pattern matching for instanceof in TBaseHelper, TUnion
    • Switch expressions in TBinaryProtocol, TCompactProtocol, TJSONProtocol

https://claude.ai/code/session_01EQ812qEBnt14Gq7gKCriiS

- Update lib/java/build.gradle to require JDK 17 minimum
- Update lib/java/gradle/sourceConfiguration.gradle to target Java 17
- Update contrib/thrift-maven-plugin/pom.xml to use Java 17
- Update .github/workflows/build.yml cross-test to use JDK 17
- Update lib/kotlin/*.gradle.kts files to target JVM 17
- Add Jdk17Test.java to verify JDK 17 runtime features
- Apply JDK 17 features:
  - Pattern matching for instanceof in TBaseHelper, TUnion
  - Switch expressions in TBinaryProtocol, TCompactProtocol, TJSONProtocol

https://claude.ai/code/session_01EQ812qEBnt14Gq7gKCriiS
- Add ErrorProne plugin (v2.46.0) to Gradle build configuration
- Update Gradle to 9.3.1 in CI workflow
- Add pluginManagement to settings.gradle for plugin resolution
- Fix DefaultCharset issues in TSerializer and TDeserializer
  - Use explicit ISO-8859-1 charset for 1:1 byte-to-char mapping
- Fix MissingOverride issues:
  - Option.None.isDefined(), get(), toString()
  - Option.Some.isDefined(), get(), toString()
  - TField.toString()
  - TNonblockingMultiFetchStats.toString()

https://claude.ai/code/session_01EQ812qEBnt14Gq7gKCriiS
Remove obsolete options:
- java5: Java 1.5 compatibility no longer needed
- android_legacy: Android 2.3 compatibility no longer needed
- option_type: Always use java.util.Optional for optional fields

Change defaults:
- jakarta_annotations is now the default (use javax_annotations to opt out)
- Optional fields are always wrapped in java.util.Optional

Remove custom Option class:
- Delete org.apache.thrift.Option (replaced by java.util.Optional)
- Delete TestOptionType.java test
- Delete JavaOptionTypeJdk8Test.thrift test resource

Update build configuration:
- Simplify Gradle test generation tasks
- Remove option_type and jakarta_annotations from generator invocations

Generated code now requires JDK 17 or later.

https://claude.ai/code/session_01EQ812qEBnt14Gq7gKCriiS
The ErrorProne configuration was previously set to skip generated code
(disableWarningsInGeneratedCode = true). This meant the Thrift compiler
output wasn't being validated for best practices.

This change enables ErrorProne checking on generated code to ensure the
Java code generator produces clean, modern code. The MissingSummary check
is disabled since generated code doesn't need javadoc summaries.

https://claude.ai/code/session_01EQ812qEBnt14Gq7gKCriiS
Maven artifact improvements:
- Add maven.compiler.release=17 property to published POM so consumers
  know JDK 17 is required
- Remove obsolete Java 9 compatibility check in javadoc configuration

Remove legacy Android library:
- Remove lib/java/android directory (unmaintained since 2015)
- Android Gradle Plugin 1.5.0 and SDK 23 are incompatible with JDK 17
- Not tested in CI and not published to Maven
- Update CMakeLists.txt to remove Android build support
- Update Makefile.am to remove android from EXTRA_DIST
- Clean up outdated Android comment in THttpClientResponseHandler

https://claude.ai/code/session_01EQ812qEBnt14Gq7gKCriiS
Integrate Uber NullAway 0.13.1 and JSpecify 1.0.0 for comprehensive
null safety analysis of both the library and generated code.

Library changes:
- Add JSpecify as an API dependency (exposed to consumers)
- Add NullAway as an ErrorProne plugin with JSpecifyMode enabled
- Add @NullMarked package-info.java for all org.apache.thrift packages
- Add @nullable annotations to methods/fields that genuinely can be null:
  - TBaseHelper: rightSize(), copyBinary() methods
  - TUnion: value_, setField_ fields and their getters
  - TApplicationException: message_ field
  - TTransport: getBuffer() method
  - StringUtils: bytesToHexString() method
- Deprecate org.apache.thrift.annotation.Nullable in favor of JSpecify

Code generator changes:
- Generate @org.jspecify.annotations.Nullable instead of Thrift annotation
- Generate package-info.java with @NullMarked for each generated package
- Update help text to document JSpecify dependency

This enables static null safety analysis with NullAway, catching potential
NullPointerExceptions at compile time rather than runtime.

https://claude.ai/code/session_01EQ812qEBnt14Gq7gKCriiS
The Thrift-specific @nullable annotation is no longer needed since:
- The code generator now uses @org.jspecify.annotations.Nullable
- The library code uses JSpecify annotations
- JSpecify is a standard dependency

https://claude.ai/code/session_01EQ812qEBnt14Gq7gKCriiS
- Add labels for easier PR categorization (dependencies, java, kotlin, github-actions)
- Group related dependencies to reduce PR noise:
  - static-analysis: ErrorProne, NullAway, SpotBugs
  - testing: JUnit, Mockito
  - apache-http: HttpClient, HttpCore

This ensures automated updates for ErrorProne, NullAway, JSpecify, and other
dependencies added as part of the JDK 17 modernization.

https://claude.ai/code/session_01EQ812qEBnt14Gq7gKCriiS
@greptile-apps
Copy link

greptile-apps bot commented Feb 4, 2026

Greptile Overview

Greptile Summary

This PR updates the Java, Kotlin, Maven, and CI build configurations to require JDK 17 (toolchains/--release 17), removes the legacy Java Option type/test coverage, and applies Java 17 language features (pattern matching for instanceof, switch expressions) in core protocol and helper classes. It also adds package-info.java NullMarked annotations across the Java library and introduces a Jdk17Test to assert JDK 17 runtime behavior.

Overall, the changes align the build + published metadata with a Java 17 baseline across Gradle/Maven/CI and modernize runtime code paths accordingly.

Confidence Score: 3/5

  • This PR is close to safe to merge, but has at least one concrete runtime-nullness bug to address first.
  • Most changes are build/toolchain alignment and Java-17 syntax updates. However, TUnion.toString() can now throw an NPE given the newly annotated nullable value, and there is an unresolved question about Kotlin cross-test server JVM target consistency.
  • lib/java/src/main/java/org/apache/thrift/TUnion.java; verify Kotlin cross-test server module configuration if present

Important Files Changed

Filename Overview
.github/workflows/build.yml Updates CI to use JDK 17 in cross-tests and bumps Gradle wrapper version/hash.
compiler/cpp/src/thrift/generate/t_java_generator.cc Simplifies generator options and defaults to Jakarta annotations; generates NullMarked package-info with fully-qualified JSpecify annotations (style inconsistency but compiles).
contrib/thrift-maven-plugin/pom.xml Ensures Maven plugin compiles with Java 17 compiler properties.
lib/java/build.gradle Adds/updates JDK 17 minimum enforcement and aligns Gradle build with toolchain/release settings.
lib/java/gradle/sourceConfiguration.gradle Targets Java 17 using toolchains and options.release = 17 for consistent bytecode.
lib/java/src/main/java/org/apache/thrift/TUnion.java Applies instanceof pattern matching and @nullable annotations; introduces possible NPE in toString() when value is null.
lib/kotlin/build.gradle.kts Updates Kotlin library build to use Java toolchain 17 and Kotlin JVM target 17.
lib/kotlin/cross-test-client/build.gradle.kts Updates Kotlin cross-test client to target JVM 17 and align with Java 17 toolchain.

Sequence Diagram

sequenceDiagram
  participant Dev as Developer/CI
  participant GHA as GitHub Actions
  participant Gradle as Gradle (Java/Kotlin)
  participant Maven as Maven (plugin)
  participant Thrift as Thrift Java runtime

  Dev->>GHA: push PR (require JDK 17)
  GHA->>GHA: setup-java (17)
  GHA->>Gradle: build/test (toolchain=17, --release 17)
  Gradle->>Thrift: compile Java sources (Java 17 features)
  Gradle->>Thrift: run tests (incl. Jdk17Test)
  GHA->>Maven: build thrift-maven-plugin (source/target 17)
  Maven-->>GHA: artifact built
  Gradle-->>GHA: build/test results
Loading

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

8 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

@greptile-apps
Copy link

greptile-apps bot commented Feb 4, 2026

Additional Comments (2)

lib/java/src/main/java/org/apache/thrift/TUnion.java
Null dereference in toString
getFieldValue() is now annotated @Nullable, but toString() still does sb.append(v.toString()) when v is not a ByteBuffer. If a union instance has setField_ != null while value_ is null (possible via subclasses/serialization bugs), this will throw an NPE. Consider handling null explicitly (e.g., append "null" or guard before deref).

Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/java/src/main/java/org/apache/thrift/TUnion.java
Line: 200:203

Comment:
**Null dereference in toString**
`getFieldValue()` is now annotated `@Nullable`, but `toString()` still does `sb.append(v.toString())` when `v` is not a `ByteBuffer`. If a union instance has `setField_ != null` while `value_` is null (possible via subclasses/serialization bugs), this will throw an NPE. Consider handling null explicitly (e.g., append `"null"` or guard before deref).

How can I resolve this? If you propose a fix, please make it concise.

lib/kotlin/cross-test-client/build.gradle.kts
JDK 17 enforcement gap
This PR updates Kotlin builds to target JVM 17, but only cross-test-client is changed. If lib/kotlin/cross-test-server/build.gradle.kts exists in this repo, it likely still targets an older JVM and will produce mismatched bytecode in cross-tests. Please ensure the server module is updated to the same Java/Kotlin jvmTarget=17 / toolchain settings (or confirm it’s intentionally absent). Is there a lib/kotlin/cross-test-server module in this repository? If so, can you update it to match the client’s JVM 17/toolchain settings?

Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/kotlin/cross-test-client/build.gradle.kts
Line: 1:3

Comment:
**JDK 17 enforcement gap**
This PR updates Kotlin builds to target JVM 17, but only `cross-test-client` is changed. If `lib/kotlin/cross-test-server/build.gradle.kts` exists in this repo, it likely still targets an older JVM and will produce mismatched bytecode in cross-tests. Please ensure the server module is updated to the same Java/Kotlin `jvmTarget=17` / toolchain settings (or confirm it’s intentionally absent). Is there a `lib/kotlin/cross-test-server` module in this repository? If so, can you update it to match the client’s JVM 17/toolchain settings?

How can I resolve this? If you propose a fix, please make it concise.

claude and others added 9 commits February 4, 2026 20:54
Dependency upgrades:
- SpotBugs: plugin 5.2.5→6.4.8, tool 4.5.3→4.9.8
- FindSecBugs: 1.12.0→1.14.0
- PMD: 6.0.0→7.9.0 (with updated ruleset for PMD 7)
- SLF4J: 1.7.36→2.0.17 (switch to slf4j-simple for tests)
- JUnit: 5.9.1→5.11.4
- Mockito: 5.3.0→5.15.2
- HttpClient: 5.2.1→5.4.1
- HttpCore: 5.2→5.3.1
- Jakarta Servlet: 5.0.0→6.1.0
- Jakarta Annotation: 2.1.1→3.0.0
- Tomcat Embed: 10.1.4→11.0.2

Dependabot enhancements:
- Add jakarta group for Jakarta EE APIs
- Add logging group for SLF4J dependencies
- Add PMD and FindSecBugs to static-analysis group

https://claude.ai/code/session_01EQ812qEBnt14Gq7gKCriiS
- Add JSpecify, Spotless to static-analysis group
- Rename apache-http to apache, add Commons Lang and Tomcat Embed

https://claude.ai/code/session_01EQ812qEBnt14Gq7gKCriiS
Reorder imports to follow Google Java Format conventions:
- java.* imports first
- org.apache.* imports next
- org.jspecify.* imports last

https://claude.ai/code/session_01EQ812qEBnt14Gq7gKCriiS
Package-level annotations require a package declaration, so only generate
package-info.java when a Java namespace is specified in the .thrift file.

https://claude.ai/code/session_01EQ812qEBnt14Gq7gKCriiS
…s-test-server

- Fix potential NPE in TUnion.toString() by using String.valueOf(v) instead
  of v.toString() for @nullable getFieldValue() result
- Use local variables after null checks so NullAway can track narrowing
  in toString(), StandardScheme.write(), and TupleScheme.write()
- Add null check in getFieldValue(F) to satisfy NullAway since it delegates
  to @nullable getFieldValue()
- Make deepCopyObject() accept/return @nullable to match @nullable value_
- Add JVM 17 target to Kotlin cross-test-server build to match
  cross-test-client and root module

https://claude.ai/code/session_01EQ812qEBnt14Gq7gKCriiS
Update code formatting tooling to latest versions and apply
reformatting to ensure CI passes spotlessCheck.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Use assignment syntax (=) for property assignments
- Add comment noting SpotBugs enum string coercion needs fixing for Gradle 10

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
ErrorProne 2.46.0 requires JDK 21+. Version 2.42.0 is the last
version compatible with JDK 17.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Throw AssertionError in the private no-arg constructor to satisfy
NullAway's requirement that all @nonnull fields are initialized.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@greggdonovan greggdonovan force-pushed the claude/java-drop-eol-qlAJz branch from b137d09 to aedb3d4 Compare February 5, 2026 01:08
greggdonovan and others added 9 commits February 4, 2026 21:16
- Add @nullable to WriteCallback parameter for Void callbacks
- Suppress StatementSwitchToExpressionSwitch in skip method

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Cast to long before addition to prevent potential integer overflow.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
ErrorProne requires @OverRide annotation when implementing Closeable.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Replace getClass() check with instanceof as recommended by ErrorProne.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add missing @OverRide annotations across transport classes
- Fix NullAway issues with @nullable annotations for nullable fields
- Add @SuppressWarnings("NullAway") where null is intentional state
- Fix empty @param javadoc tags with descriptions
- Add Locale.ROOT to toUpperCase() calls
- Fix synchronized method overrides in TByteArrayOutputStream
- Fix TypeParameterUnusedInFormals in SchemeFactory

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
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.

2 participants