Migrate to Gradle 9.5.1 and Java 25#1889
Open
Thevakumar-Luheerathan wants to merge 9 commits into
Open
Conversation
…27-050400-74f7e6bf - Upgrade Gradle wrapper to 9.5.1 - Replace com.gradle.enterprise with com.gradle.develocity plugin - Upgrade net.researchgate.release to 3.1.0 for Gradle 9 compatibility - Replace all deprecated buildDir references with layout.buildDirectory - Replace Project.exec() calls with ExecOperations injection pattern - Remove deprecated sourceCompatibility/targetCompatibility (replaced by toolchain) - Add Java 25 toolchain to all Java subprojects - Upgrade Ballerina Gradle plugin to 4.0.0 (Java 25 + Gradle 9.5.1 compatible) - Update ballerinaLangVersion to 2201.14.0-20260527-050400-74f7e6bf - Update all platform.java21 TOML sections to platform.java25 - Add patchBallerinaScripts task to fix bal binary permissions and remove --sun-misc-unsafe-memory-access=allow flag (removed in Java 25) - Upgrade SpotBugs Gradle plugin to 6.5.1 for Java 25 class file support - Add SpotBugs exclusions for THROWS/AT/USELESS_STRING detectors introduced in SpotBugs 4.9.x (bundled in plugin >= 6.2.0) - Update CI workflow to use pull-request-build-template.yml@java-25-migration
Thevakumar-Luheerathan
requested review from
TharmiganK,
daneshk,
lnash94 and
shafreenAnfar
as code owners
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR upgrades the project to Java 25, modernizes Gradle infrastructure to 9.5.1 with layout.buildDirectory APIs, introduces ExecOperations-based helpers and script-patching tasks for Ballerina tooling compatibility, fixes OpenAPI YAML schema property ordering via Jackson serializer patches, and updates manifests and test resources. ChangesJava 25 and Gradle infrastructure modernization
🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
Upgrades the build toolchain to Gradle 9.5.1 and the Java 25 toolchain, swaps deprecated/legacy Gradle plugins for their maintained replacements, and bumps the Ballerina language distribution to a 2201.14.0 nightly. The Gradle scripts are migrated from the now-removed Project.buildDir/Project.exec() APIs to layout.buildDirectory and injected ExecOperations, with a new patchBallerinaScripts task added to make the unpacked Ballerina distribution compatible with the Java 25 runtime.
Changes:
- Migrate Gradle wrapper to 9.5.1, set Java 25 toolchain in subprojects, and update CI to JDK 25.
- Replace
com.github.johnrengelman.shadow→com.gradleup.shadow,gradle.enterprise→develocity, droporg.javamodularity.moduleplugin, and bump SpotBugs/JaCoCo/release/Ballerina plugin versions. - Replace
project.buildDir/project.exec {}withlayout.buildDirectoryandExecOperationshelpers; addpatchBallerinaScriptsto strip--sun-misc-unsafe-memory-access=allowand injectJAVA_HOMEinto distribution scripts; broadenspotbugs-exclude.xml.
Reviewed changes
Copilot reviewed 22 out of 23 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| gradle/wrapper/gradle-wrapper.properties | Bumps Gradle wrapper to 9.5.1. |
| gradle.properties | Bumps plugin/dep versions and Ballerina lang to 2201.14.0 nightly. |
| settings.gradle | Switches shadow plugin coords, drops java-modularity, swaps gradleEnterprise→develocity, adds mavenLocal(). |
| build.gradle | Drops java-modularity, adds Java 25 toolchain to all java subprojects, modernizes JaCoCo output locations. |
| gradle/javaProject.gradle | Removes sourceCompatibility, switches build paths and SpotBugs/JaCoCo report config to layout.buildDirectory. |
| config/checkstyle/build.gradle | Replaces buildDir with layout.buildDirectory. |
| config/resources/ToolBallerina.toml | Updates distribution to 2201.14.0 nightly. |
| openapi-tool/Ballerina.toml | Updates distribution to 2201.14.0 nightly. |
| openapi-tool/build.gradle | Replaces project.exec with ExecOperations helper. |
| openapi-cli/build.gradle | Switches shadow plugin and buildDir references. |
| openapi-cli/src/test/resources/expected_gen/ballerina_project/service_type.bal | Adds new expected generated service type fixture. |
| openapi-core/build.gradle | Switches shadow plugin and buildDir references. |
| openapi-validator/build.gradle | Switches shadow plugin and buildDir references. |
| ballerina-to-openapi/build.gradle | Switches shadow plugin and buildDir references. |
| openapi-bal-task-plugin/build.gradle | Switches shadow plugin coords. |
| openapi-extension-tests/build.gradle | Switches shadow plugin, buildDir, and JaCoCo report paths. |
| openapi-integration-tests/build.gradle | Switches shadow plugin, buildDir, and JaCoCo report paths. |
| openapi-client-native/build.gradle | Adds ExecOperations helper and replaces project.exec/exec usages. |
| openapi-client-native/build-configs/resources/Ballerina.toml | Bumps distribution and platform to java25. |
| openapi-client-native/ballerina-tests/Ballerina.toml | Bumps distribution and platform to java25. |
| module-ballerina-openapi/build.gradle | Adds ExecOperations helper, new patchBallerinaScripts task, updates platform to java25, modernizes buildDir usage. |
| spotbugs-exclude.xml | Broadly excludes THROWS, AT, USELESS_STRING, DM bug codes. |
| .github/workflows/pull-request.yml | Bumps setup-java to v4 and target JDK to 25. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+45
to
+57
| <!-- Pre-existing issues surfaced by SpotBugs 4.9.x (bundled in plugin >= 6.2.0) --> | ||
| <Match> | ||
| <BugCode name="THROWS"/> | ||
| </Match> | ||
| <Match> | ||
| <BugCode name="AT"/> | ||
| </Match> | ||
| <Match> | ||
| <BugCode name="USELESS_STRING"/> | ||
| </Match> | ||
| <Match> | ||
| <BugCode name="DM"/> | ||
| </Match> |
| } | ||
|
|
||
| repositories { | ||
| mavenLocal() |
Comment on lines
+27
to
+60
| abstract class PatchBallerinaOpenAPIScriptsTask extends DefaultTask { | ||
| @Inject abstract JavaToolchainService getJavaToolchainService() | ||
|
|
||
| @TaskAction | ||
| void patch() { | ||
| def launcher = javaToolchainService.launcherFor { spec -> | ||
| spec.languageVersion.set(JavaLanguageVersion.of(25)) | ||
| }.get() | ||
| def java25Home = launcher.executablePath.asFile.parentFile.parentFile.absolutePath | ||
|
|
||
| def binDirs = [ | ||
| project.layout.buildDirectory.dir("target/extracted-distributions/jballerina-tools-zip/jballerina-tools-${project.ballerinaLangVersion}/bin").get().asFile, | ||
| new File("${project.rootDir}/target/ballerina-runtime/bin") | ||
| ] | ||
| binDirs.each { binDir -> | ||
| if (binDir.exists()) { | ||
| binDir.eachFile { file -> | ||
| file.setExecutable(true) | ||
| if (!file.name.endsWith('.bat') && !file.name.endsWith('.cmd')) { | ||
| def original = file.text | ||
| def lines = original.split('\n').toList() | ||
| lines = lines.findAll { !it.contains('--sun-misc-unsafe-memory-access=allow') } | ||
| def shebangIdx = lines.findIndexOf { it.startsWith('#!') } | ||
| if (shebangIdx >= 0) { | ||
| lines.add(shebangIdx + 1, "export JAVA_HOME='${java25Home}'") | ||
| } | ||
| def patched = lines.join('\n') + '\n' | ||
| if (patched != original) { file.text = patched } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
Comment on lines
12
to
+17
| - uses: actions/checkout@v2 | ||
| - name: Set up JDK 21 | ||
| uses: actions/setup-java@v2 | ||
| - name: Set up JDK 25 | ||
| uses: actions/setup-java@v4 | ||
| with: | ||
| distribution: 'temurin' | ||
| java-version: 21.0.3 | ||
| java-version: 25 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 @.github/workflows/pull-request.yml:
- Line 14: Find both occurrences of the GitHub Action reference "uses:
actions/setup-java@v4" and replace the mutable tag with the action's immutable
commit SHA (e.g., change to "uses: actions/setup-java@<commit-sha>"); ensure you
use the same verified full SHA for both instances so the workflow pins to a
specific commit and update any related comments if present to note the pinned
SHA and why it was chosen.
In `@build.gradle`:
- Around line 127-133: The createProperties task currently only gets run via
buildProject; wire it into the standard build lifecycle so the properties file
openapi-cli/src/main/resources/openapi-client-native-version.properties is
always regenerated for CI builds: make createProperties a dependency of the
openapi-cli resource processing or build (for example add dependsOn to the
openapi-cli processResources task or to the openapi-cli project’s build task) so
that createProperties runs before resources are packaged; ensure this change
affects the same task name createProperties and removes reliance solely on
buildProject so OpenApiCmd / OpenAPICodeGeneratorTool always sees an up-to-date
client-native version.
In `@module-ballerina-openapi/build.gradle`:
- Around line 49-54: The patch that inserts "export JAVA_HOME='${java25Home}'"
in unpackJballerinaTools is not idempotent and will duplicate the export each
run; before adding the line (where shebangIdx is computed and lines.add is
called) check whether a line matching /^export\s+JAVA_HOME=/ (or lines.any {
it.contains("export JAVA_HOME") }) already exists in lines (preferably only
searching the region immediately after the shebang) and only insert when absent,
or replace any existing matching line so that the file.text update happens only
once.
In `@settings.gradle`:
- Line 29: settings.gradle currently adds mavenLocal() inside pluginManagement {
repositories { ... } } before gradlePluginPortal(), which can let local
artifacts override pinned plugin versions; change this so mavenLocal() is only
added when an opt-in project property is set (e.g., check
project.hasProperty("useMavenLocal") || findProperty("useMavenLocal") == "true")
and otherwise omit it, leaving gradlePluginPortal() as the default resolver;
update the code that constructs pluginManagement.repositories (look for the
pluginManagement.repositories block and the mavenLocal() call) to wrap the
mavenLocal() addition in that conditional or remove it by default and document
the -PuseMavenLocal flag.
In `@spotbugs-exclude.xml`:
- Around line 45-57: The current spotbugs-exclude.xml entries match only BugCode
names (THROWS, AT, USELESS_STRING, DM) which suppress them globally; update each
<Match> for BugCodes THROWS, AT, USELESS_STRING, and DM to include narrow scope
constraints (e.g., add a <Class> or <classregex> element and, when appropriate,
a <Method> or <methodregex> element) that target the specific offending
classes/methods rather than the whole project; locate the existing <Match>
blocks for BugCode name="THROWS"/"AT"/"USELESS_STRING"/"DM" and replace the
global suppression with class- or method-scoped suppressions referencing the
exact class names or regexes that caused the SpotBugs warnings.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4e020d39-e5bd-482d-a4e8-7775410ec9c5
📒 Files selected for processing (23)
.github/workflows/pull-request.ymlballerina-to-openapi/build.gradlebuild.gradleconfig/checkstyle/build.gradleconfig/resources/ToolBallerina.tomlgradle.propertiesgradle/javaProject.gradlegradle/wrapper/gradle-wrapper.propertiesmodule-ballerina-openapi/build.gradleopenapi-bal-task-plugin/build.gradleopenapi-cli/build.gradleopenapi-cli/src/test/resources/expected_gen/ballerina_project/service_type.balopenapi-client-native/ballerina-tests/Ballerina.tomlopenapi-client-native/build-configs/resources/Ballerina.tomlopenapi-client-native/build.gradleopenapi-core/build.gradleopenapi-extension-tests/build.gradleopenapi-integration-tests/build.gradleopenapi-tool/Ballerina.tomlopenapi-tool/build.gradleopenapi-validator/build.gradlesettings.gradlespotbugs-exclude.xml
| - name: Set up JDK 21 | ||
| uses: actions/setup-java@v2 | ||
| - name: Set up JDK 25 | ||
| uses: actions/setup-java@v4 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify all tag-based action references still present in workflow files.
rg -n 'uses:\s*[^@[:space:]]+@v[0-9]+(\.[0-9]+)?(\.[0-9]+)?\s*$' .github/workflowsRepository: ballerina-platform/openapi-tools
Length of output: 1369
Pin actions/setup-java to an immutable commit SHA (avoid @v4).
In .github/workflows/pull-request.yml, both uses: actions/setup-java@v4 entries (lines 14 and 36) are mutable tag references—update each to the full commit SHA.
🧰 Tools
🪛 zizmor (1.25.2)
[error] 14-14: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/pull-request.yml at line 14, Find both occurrences of the
GitHub Action reference "uses: actions/setup-java@v4" and replace the mutable
tag with the action's immutable commit SHA (e.g., change to "uses:
actions/setup-java@<commit-sha>"); ensure you use the same verified full SHA for
both instances so the workflow pins to a specific commit and update any related
comments if present to note the pinned SHA and why it was chosen.
Comment on lines
+49
to
+54
| def shebangIdx = lines.findIndexOf { it.startsWith('#!') } | ||
| if (shebangIdx >= 0) { | ||
| lines.add(shebangIdx + 1, "export JAVA_HOME='${java25Home}'") | ||
| } | ||
| def patched = lines.join('\n') + '\n' | ||
| if (patched != original) { file.text = patched } |
There was a problem hiding this comment.
Non-idempotent JAVA_HOME injection duplicates the export on every run.
unpackJballerinaTools stays up-to-date across incremental builds, so the bin scripts persist already-patched. Since this task has no up-to-date check and inserts export JAVA_HOME=... after the shebang unconditionally, each subsequent build/test adds another duplicate export line and rewrites the file. Guard the insertion against an existing export.
🛠️ Proposed guard to keep the patch idempotent
def shebangIdx = lines.findIndexOf { it.startsWith('#!') }
- if (shebangIdx >= 0) {
+ if (shebangIdx >= 0 && !lines.any { it.trim().startsWith("export JAVA_HOME=") }) {
lines.add(shebangIdx + 1, "export JAVA_HOME='${java25Home}'")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def shebangIdx = lines.findIndexOf { it.startsWith('#!') } | |
| if (shebangIdx >= 0) { | |
| lines.add(shebangIdx + 1, "export JAVA_HOME='${java25Home}'") | |
| } | |
| def patched = lines.join('\n') + '\n' | |
| if (patched != original) { file.text = patched } | |
| def shebangIdx = lines.findIndexOf { it.startsWith('#!') } | |
| if (shebangIdx >= 0 && !lines.any { it.trim().startsWith("export JAVA_HOME=") }) { | |
| lines.add(shebangIdx + 1, "export JAVA_HOME='${java25Home}'") | |
| } | |
| def patched = lines.join('\n') + '\n' | |
| if (patched != original) { file.text = patched } |
🤖 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 `@module-ballerina-openapi/build.gradle` around lines 49 - 54, The patch that
inserts "export JAVA_HOME='${java25Home}'" in unpackJballerinaTools is not
idempotent and will duplicate the export each run; before adding the line (where
shebangIdx is computed and lines.add is called) check whether a line matching
/^export\s+JAVA_HOME=/ (or lines.any { it.contains("export JAVA_HOME") })
already exists in lines (preferably only searching the region immediately after
the shebang) and only insert when absent, or replace any existing matching line
so that the file.text update happens only once.
| } | ||
|
|
||
| repositories { | ||
| mavenLocal() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify whether this repo has an intentional workflow that requires resolving plugins from mavenLocal().
# Expected result: no matches beyond settings.gradle unless local plugin publication is a documented/dev-only flow.
rg -n --hidden -S 'mavenLocal\(|publishToMavenLocal|publishAllPublicationsToMavenLocal|pluginManagement' -g '!**/.git/**'Repository: ballerina-platform/openapi-tools
Length of output: 522
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect settings.gradle around pluginManagement.repositories (line ~18-35)
nl -ba settings.gradle | sed -n '1,120p'
# 2) Look for any property-based gating or documentation for using mavenLocal()
rg -n --hidden -S 'mavenLocal\(\)|pluginManagement|gradlePluginPortal\(|mavenLocal.*(enabled|enable|localOnly|property|flag|only)' -g '!**/.git/**'
# 3) Check README context around publishToMavenLocal
nl -ba README.md | sed -n '1,120p'Repository: ballerina-platform/openapi-tools
Length of output: 119
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- settings.gradle (1-120) ----"
cat -n settings.gradle | sed -n '1,120p'
echo "---- rg: mavenLocal / pluginManagement / gradlePluginPortal / gating keywords ----"
rg -n --hidden -S 'mavenLocal\(\)|enable.*mavenLocal|mavenLocal.*(enabled|enable|property|flag|localOnly)|gradlePluginPortal\(|pluginManagement' -g '!**/.git/**' .
echo "---- README.md (35-120) ----"
cat -n README.md | sed -n '35,120p'Repository: ballerina-platform/openapi-tools
Length of output: 4394
Gate mavenLocal() in pluginManagement.repositories behind an opt-in flag.
settings.gradle puts mavenLocal() before gradlePluginPortal() inside pluginManagement { repositories { ... } }, so Gradle can resolve build plugins from the developer’s local Maven cache and potentially override the intended/pinned plugin versions—making plugin resolution non-reproducible across machines. Add mavenLocal() only when a local-only property/flag is set (e.g., -PuseMavenLocal=true), or remove it from the default path.
🤖 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 `@settings.gradle` at line 29, settings.gradle currently adds mavenLocal()
inside pluginManagement { repositories { ... } } before gradlePluginPortal(),
which can let local artifacts override pinned plugin versions; change this so
mavenLocal() is only added when an opt-in project property is set (e.g., check
project.hasProperty("useMavenLocal") || findProperty("useMavenLocal") == "true")
and otherwise omit it, leaving gradlePluginPortal() as the default resolver;
update the code that constructs pluginManagement.repositories (look for the
pluginManagement.repositories block and the mavenLocal() call) to wrap the
mavenLocal() addition in that conditional or remove it by default and document
the -PuseMavenLocal flag.
Comment on lines
+45
to
+57
| <!-- Pre-existing issues surfaced by SpotBugs 4.9.x (bundled in plugin >= 6.2.0) --> | ||
| <Match> | ||
| <BugCode name="THROWS"/> | ||
| </Match> | ||
| <Match> | ||
| <BugCode name="AT"/> | ||
| </Match> | ||
| <Match> | ||
| <BugCode name="USELESS_STRING"/> | ||
| </Match> | ||
| <Match> | ||
| <BugCode name="DM"/> | ||
| </Match> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read-only verification: inspect breadth of new SpotBugs exclusions and compare with existing scoped rules.
set -euo pipefail
echo "== spotbugs-exclude.xml: global BugCode suppressions =="
rg -n '<BugCode name="' spotbugs-exclude.xml -C2
echo
echo "== spotbugs-exclude.xml: scoped Match entries (Class/Method + Bug) =="
rg -n '<Match>|<Class name=|<Method name=|<Bug pattern=' spotbugs-exclude.xml -C1Repository: ballerina-platform/openapi-tools
Length of output: 2039
Avoid whole-program SpotBugs BugCode suppressions; scope THROWS/AT/USELESS_STRING/DM.
In spotbugs-exclude.xml (lines 45-57), each <Match> only specifies <BugCode name="THROWS|AT|USELESS_STRING|DM"/> without any <Class>/<Method> constraints, so the suppression applies project-wide. Scope each exclusion to specific <Class>/classregex (and <Method> where applicable) to keep the baseline narrow.
🤖 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 `@spotbugs-exclude.xml` around lines 45 - 57, The current spotbugs-exclude.xml
entries match only BugCode names (THROWS, AT, USELESS_STRING, DM) which suppress
them globally; update each <Match> for BugCodes THROWS, AT, USELESS_STRING, and
DM to include narrow scope constraints (e.g., add a <Class> or <classregex>
element and, when appropriate, a <Method> or <methodregex> element) that target
the specific offending classes/methods rather than the whole project; locate the
existing <Match> blocks for BugCode name="THROWS"/"AT"/"USELESS_STRING"/"DM" and
replace the global suppression with class- or method-scoped suppressions
referencing the exact class names or regexes that caused the SpotBugs warnings.
- openapi-extension-tests: replace eager layout.buildDirectory.get().asFile with lazy Provider<Directory> to fix null basedir on Windows (Gradle 9.5.1) - openapi-tool: add PatchBallerinaToolScriptsTask to set execute permissions and remove --sun-misc-unsafe-memory-access=allow flag from bal scripts unpacked by the Ballerina plugin (mirrors module-ballerina-openapi approach) - module-ballerina-openapi/Dependencies.toml: update distribution version to 2201.14.0-20260527-050400-74f7e6bf and openapi package to 2.4.2 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Nested copy{} blocks inside a copy{} spec called project.copy{} with
a relative into() path, whose basedir resolved to null under Gradle 9.5.1.
Restructure into flat, sequential copy calls with absolute destination
paths and guard checks for optional source directories.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Thevakumar-Luheerathan
force-pushed
the
java-25-migration
branch
from
39bf932 to
3edeabe
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@ballerina-to-openapi/src/main/java/io/ballerina/openapi/service/mapper/model/OASResult.java`:
- Around line 43-74: The YAML mapper patching logic is duplicated between
OASResult.patchYamlMapperForSchemaOrder and SubCmdBase causing potential
double-patching; extract the logic into a new utility (e.g., YamlMapperConfig)
with a single static AtomicBoolean guard (patched) and a public
ensureSchemaPropertyOrder() method that performs the current patch (including
the BeanSerializerModifier and ensureDefaultBeforeEnum behavior). Replace the
patchYamlMapperForSchemaOrder implementations in OASResult and SubCmdBase to
simply call YamlMapperConfig.ensureSchemaPropertyOrder() and remove their local
yamlMapperPatched flags and duplicated code; also fix the small style issue in
ensureDefaultBeforeEnum by declaring defaultIdx and enumIdx on separate lines
(int defaultIdx = -1; int enumIdx = -1;) to satisfy static analysis.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: addc8e9c-6de5-4812-9f98-a35edcb3f7fe
📒 Files selected for processing (14)
ballerina-to-openapi/src/main/java/io/ballerina/openapi/service/mapper/model/OASResult.javabuild.gradleopenapi-bal-task-plugin/build.gradleopenapi-cli/build.gradleopenapi-cli/src/main/java/io/ballerina/openapi/cmd/SubCmdBase.javaopenapi-cli/src/main/java/module-info.javaopenapi-integration-tests/src/test/resources/ballerina_sources/examples/response_example/result.yamlopenapi-integration-tests/src/test/resources/ballerina_sources/project_non_openapi_annotation/result.yamlopenapi-integration-tests/src/test/resources/ballerina_sources/project_non_openapi_annotation_with_base_path/result.yamlopenapi-integration-tests/src/test/resources/ballerina_sources/project_non_openapi_annotation_without_base_path/result.yamlopenapi-integration-tests/src/test/resources/ballerina_sources/project_openapi_bal_ext/result_0.yamlopenapi-integration-tests/src/test/resources/ballerina_sources/project_openapi_bal_ext/result_1.yamlopenapi-integration-tests/src/test/resources/ballerina_sources/project_openapi_examples/result.yamlopenapi-integration-tests/src/test/resources/ballerina_sources/project_openapi_with_included_test/result.yaml
✅ Files skipped from review due to trivial changes (8)
- openapi-cli/src/main/java/module-info.java
- openapi-integration-tests/src/test/resources/ballerina_sources/project_non_openapi_annotation_with_base_path/result.yaml
- openapi-integration-tests/src/test/resources/ballerina_sources/examples/response_example/result.yaml
- openapi-integration-tests/src/test/resources/ballerina_sources/project_non_openapi_annotation/result.yaml
- openapi-integration-tests/src/test/resources/ballerina_sources/project_openapi_bal_ext/result_0.yaml
- openapi-integration-tests/src/test/resources/ballerina_sources/project_openapi_bal_ext/result_1.yaml
- openapi-integration-tests/src/test/resources/ballerina_sources/project_openapi_examples/result.yaml
- openapi-integration-tests/src/test/resources/ballerina_sources/project_openapi_with_included_test/result.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- openapi-bal-task-plugin/build.gradle
- openapi-integration-tests/src/test/resources/ballerina_sources/project_non_openapi_annotation_without_base_path/result.yaml
- openapi-cli/build.gradle
- build.gradle
Comment on lines
+43
to
+74
| private static boolean yamlMapperPatched = false; | ||
|
|
||
| private static void patchYamlMapperForSchemaOrder() { | ||
| if (!yamlMapperPatched) { | ||
| yamlMapperPatched = true; | ||
| Yaml.mapper().setSerializerFactory( | ||
| Yaml.mapper().getSerializerFactory().withSerializerModifier(new BeanSerializerModifier() { | ||
| @Override | ||
| public List<BeanPropertyWriter> orderProperties(SerializationConfig config, | ||
| BeanDescription beanDesc, List<BeanPropertyWriter> beanProperties) { | ||
| if (Schema.class.isAssignableFrom(beanDesc.getBeanClass())) { | ||
| ensureDefaultBeforeEnum(beanProperties); | ||
| } | ||
| return beanProperties; | ||
| } | ||
| }) | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| private static void ensureDefaultBeforeEnum(List<BeanPropertyWriter> props) { | ||
| int defaultIdx = -1, enumIdx = -1; | ||
| for (int i = 0; i < props.size(); i++) { | ||
| String name = props.get(i).getName(); | ||
| if ("default".equals(name)) { defaultIdx = i; } | ||
| else if ("enum".equals(name)) { enumIdx = i; } | ||
| } | ||
| if (enumIdx != -1 && defaultIdx != -1 && enumIdx < defaultIdx) { | ||
| BeanPropertyWriter defaultProp = props.remove(defaultIdx); | ||
| props.add(enumIdx, defaultProp); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Extract shared YAML mapper patching logic to avoid duplication and double-patching.
This exact implementation is duplicated in SubCmdBase.java (lines 107-138). Both classes patch the same global Yaml.mapper() but maintain independent yamlMapperPatched flags, so if both code paths execute within the same JVM, the mapper is patched twice.
Consider extracting this to a shared utility class with a single static guard:
// e.g., io.ballerina.openapi.core.util.YamlMapperConfig
public final class YamlMapperConfig {
private static final AtomicBoolean patched = new AtomicBoolean(false);
public static void ensureSchemaPropertyOrder() {
if (patched.compareAndSet(false, true)) {
// patching logic
}
}
}Also, per static analysis, declare enumIdx on a separate line (line 64):
- int defaultIdx = -1, enumIdx = -1;
+ int defaultIdx = -1;
+ int enumIdx = -1;🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 64-64: Declare "enumIdx" on a separate line.
🤖 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
`@ballerina-to-openapi/src/main/java/io/ballerina/openapi/service/mapper/model/OASResult.java`
around lines 43 - 74, The YAML mapper patching logic is duplicated between
OASResult.patchYamlMapperForSchemaOrder and SubCmdBase causing potential
double-patching; extract the logic into a new utility (e.g., YamlMapperConfig)
with a single static AtomicBoolean guard (patched) and a public
ensureSchemaPropertyOrder() method that performs the current patch (including
the BeanSerializerModifier and ensureDefaultBeforeEnum behavior). Replace the
patchYamlMapperForSchemaOrder implementations in OASResult and SubCmdBase to
simply call YamlMapperConfig.ensureSchemaPropertyOrder() and remove their local
yamlMapperPatched flags and duplicated code; also fix the small style issue in
ensureDefaultBeforeEnum by declaring defaultIdx and enumIdx on separate lines
(int defaultIdx = -1; int enumIdx = -1;) to satisfy static analysis.
Source: Linters/SAST tools
Thevakumar-Luheerathan
force-pushed
the
java-25-migration
branch
from
af1b3ca to
b654265
Compare
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test plan
Summary
This pull request modernizes the build and toolchain by migrating to Gradle 9.5.1, adopting the Java 25 toolchain across the project, and updating the Ballerina distribution to 2201.14.0-20260527-050400-74f7e6bf. The changes update build scripts, CI, and tests to ensure compatibility with the newer toolchain and improve build reliability and maintainability.
Key Changes
Upgrade build tools and toolchain
Migrate deprecated Gradle APIs and modernize tasks
Plugins and dependency updates
Build stability and packaging improvements
Output consistency and test updates
Impact
These changes align the project with current Gradle and Java toolchains, reduce future deprecation risk, and improve build reliability and consistency of generated OpenAPI artifacts. CI builds are expected to pass with the updated toolchain and configuration.