Add OfflineDiacritization JAVA bindings - #3669
Merged
csukuangfj merged 1 commit intoJun 12, 2026
Merged
Conversation
dosubot
Bot
added
the
size:L
📝 WalkthroughWalkthroughThis PR adds complete Java/JNI support for offline ONNX-based diacritization. It introduces configuration classes that specify model paths and parameters, a Java JNI wrapper for lifecycle management, native C++ bindings that parse configs and delegate to underlying native code, build system integration, and a working example with automated test execution. ChangesOffline Diacritization Java API and JNI Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces offline diacritization support (using a CATT model) to the Java API of sherpa-onnx, adding Java wrappers, a JNI implementation, and an integration example. The review feedback suggests several improvements to robustness and usability: adding defensive null and state checks in both Java and JNI layers to prevent JVM crashes, implementing AutoCloseable on OfflineDiacritization for safer native resource management, and adding missing getter methods to OfflineDiacritizationModelConfig.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
matiaslin
force-pushed
the
offline-diacritization-java-bindings
branch
from
June 10, 2026 19:48
4859acf to
c1b7f85
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
sherpa-onnx/java-api/src/main/java/com/k2fsa/sherpa/onnx/OfflineDiacritization.java (1)
20-23: ⚖️ Poor tradeoffConsider replacing finalize() with try-with-resources.
finalize()has been deprecated since Java 9 and is unreliable for resource cleanup (non-deterministic timing, no guarantee it will run). Although the project currently targets Java 8 (per Makefile--release 8), implementingAutoCloseablewould enable safer try-with-resources patterns and improve forward compatibility.♻️ Optional refactor to implement AutoCloseable
-public class OfflineDiacritization { +public class OfflineDiacritization implements AutoCloseable { private long ptr = 0; public OfflineDiacritization(OfflineDiacritizationConfig config) { LibraryLoader.maybeLoad(); ptr = newFromFile(config); if (ptr == 0) { throw new IllegalArgumentException("Invalid OfflineDiacritizationConfig: failed to create native OfflineDiacritization"); } } public String addDiacritics(String text) { return addDiacritics(ptr, text); } `@Override` - protected void finalize() throws Throwable { - release(); + public void close() { + release(); } // You'd better call it manually if it is not used anymore - public void release() { + public synchronized void release() { if (this.ptr == 0) { return; } delete(this.ptr); this.ptr = 0; }Users can then write:
try (OfflineDiacritization diacrt = new OfflineDiacritization(config)) { String result = diacrt.addDiacritics(text); }🤖 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 `@sherpa-onnx/java-api/src/main/java/com/k2fsa/sherpa/onnx/OfflineDiacritization.java` around lines 20 - 23, The class OfflineDiacritization currently uses a deprecated finalize() to call release(); implement AutoCloseable instead: add "implements AutoCloseable" to the OfflineDiacritization class, create a public close() method that delegates to the existing release() logic (or refactor release() to be the shared cleanup entry), remove finalize(), and update any usages to support try-with-resources; ensure release()/close() is idempotent and thread-safe if necessary.sherpa-onnx/java-api/src/main/java/com/k2fsa/sherpa/onnx/OfflineDiacritizationModelConfig.java (1)
24-30: ⚡ Quick winAdd missing public getters for completeness.
The class exposes
getCattEncoder()andgetCattDecoder()but lacks public getters fornumThreads,debug, andprovider. This prevents callers from inspecting the full configuration after construction, breaking the symmetry between builder setters and config getters.➕ Proposed fix to add the missing getters
public String getCattDecoder() { return cattDecoder; } +public int getNumThreads() { + return numThreads; +} + +public boolean getDebug() { + return debug; +} + +public String getProvider() { + return provider; +} + public static class Builder {🤖 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 `@sherpa-onnx/java-api/src/main/java/com/k2fsa/sherpa/onnx/OfflineDiacritizationModelConfig.java` around lines 24 - 30, Add the missing public getters to OfflineDiacritizationModelConfig to match the existing getCattEncoder()/getCattDecoder() and the builder setters: implement public int getNumThreads() returning the existing numThreads field, public boolean isDebug() (or getDebug() if you prefer JavaBean style) returning the debug field, and public String getProvider() returning the provider field; keep the same visibility and naming pattern as the existing getters so callers can inspect the full configuration after construction.java-api-examples/OfflineAddDiacritization.java (1)
22-37: ⚡ Quick winRelease native resources explicitly in the example.
Line 22 creates a native-backed
OfflineDiacritization, but it is not explicitly released after use (Lines 31-36). Please wrap usage intry/finallyand callrelease()so this sample is safe in long-lived processes.♻️ Suggested change
- OfflineDiacritization diacrt = new OfflineDiacritization(config); + OfflineDiacritization diacrt = new OfflineDiacritization(config); - String[] sentences = - new String[] { - "وقالت مجلة نيوزويك الأمريكية التحديث الجديد ل إنستجرام يمكن أن يساهم في إيقاف وكشف الحسابات المزورة بسهولة شديدة", - "اللغة العربية من أقدم اللغات السامية", - }; - - System.out.println("---"); - for (String text : sentences) { - String out = diacrt.addDiacritics(text); - System.out.printf("Input: %s\n", text); - System.out.printf("Output: %s\n", out); - System.out.println("---"); - } + try { + String[] sentences = + new String[] { + "وقالت مجلة نيوزويك الأمريكية التحديث الجديد ل إنستجرام يمكن أن يساهم في إيقاف وكشف الحسابات المزورة بسهولة شديدة", + "اللغة العربية من أقدم اللغات السامية", + }; + + System.out.println("---"); + for (String text : sentences) { + String out = diacrt.addDiacritics(text); + System.out.printf("Input: %s\n", text); + System.out.printf("Output: %s\n", out); + System.out.println("---"); + } + } finally { + diacrt.release(); + }🤖 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 `@java-api-examples/OfflineAddDiacritization.java` around lines 22 - 37, The OfflineDiacritization instance created as "OfflineDiacritization diacrt = new OfflineDiacritization(config);" holds native resources and must be released; wrap its usage in a try/finally (or try-with-resources if supported) so that after calling diacrt.addDiacritics(...) for each sentence you always call diacrt.release() in the finally block to free native resources and avoid leaks.
🤖 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 `@java-api-examples/run-offline-add-diacritics.sh`:
- Line 35: The JVM option -Djava.library.path currently uses an unquoted $PWD
which will split on spaces; update the script so the -Djava.library.path
assignment quotes the path (e.g. -Djava.library.path="$PWD/../build/lib") to
prevent argument splitting when directories contain spaces; locate the
occurrence of -Djava.library.path in the run-offline-add-diacritics.sh
invocation and wrap the quoted expansion accordingly.
In
`@sherpa-onnx/java-api/src/main/java/com/k2fsa/sherpa/onnx/OfflineDiacritization.java`:
- Around line 8-14: The OfflineDiacritization constructor must validate its
OfflineDiacritizationConfig parameter before calling native code: check that the
config argument is not null at the start of
OfflineDiacritization(OfflineDiacritizationConfig config) and throw an
appropriate IllegalArgumentException (or NullPointerException) with a clear
message if it is null; only call LibraryLoader.maybeLoad() and
newFromFile(config) after the null check, and keep using the existing ptr check
and error message if newFromFile returns 0.
- Around line 25-32: The release() method in OfflineDiacritization is racy and
can double-free the native pointer; make it thread-safe by guarding access to
the ptr field (e.g., synchronize the release() method or use an atomic
compare-and-set on ptr) so only one thread can call delete(this.ptr) and set
this.ptr = 0; ensure you reference the ptr field and the native delete(long)
call in your change and preserve behavior when ptr == 0.
In `@sherpa-onnx/jni/offline-diacritization.cc`:
- Around line 108-124: In
Java_com_k2fsa_sherpa_onnx_OfflineDiacritization_addDiacritics check the result
of env->GetStringUTFChars (ptext) for nullptr before using it; if ptext is
nullptr, signal an OOM to Java (e.g. env->ThrowNew on
java/lang/OutOfMemoryError) and return nullptr instead of calling
diacrt->AddDiacritics or ReleaseStringUTFChars, otherwise proceed as now and
ensure ReleaseStringUTFChars is only called when ptext was non-null (references:
function Java_com_k2fsa_sherpa_onnx_OfflineDiacritization_addDiacritics, symbols
GetStringUTFChars, ptext, ReleaseStringUTFChars, diacrt->AddDiacritics,
SafeNewStringUTF).
---
Nitpick comments:
In `@java-api-examples/OfflineAddDiacritization.java`:
- Around line 22-37: The OfflineDiacritization instance created as
"OfflineDiacritization diacrt = new OfflineDiacritization(config);" holds native
resources and must be released; wrap its usage in a try/finally (or
try-with-resources if supported) so that after calling diacrt.addDiacritics(...)
for each sentence you always call diacrt.release() in the finally block to free
native resources and avoid leaks.
In
`@sherpa-onnx/java-api/src/main/java/com/k2fsa/sherpa/onnx/OfflineDiacritization.java`:
- Around line 20-23: The class OfflineDiacritization currently uses a deprecated
finalize() to call release(); implement AutoCloseable instead: add "implements
AutoCloseable" to the OfflineDiacritization class, create a public close()
method that delegates to the existing release() logic (or refactor release() to
be the shared cleanup entry), remove finalize(), and update any usages to
support try-with-resources; ensure release()/close() is idempotent and
thread-safe if necessary.
In
`@sherpa-onnx/java-api/src/main/java/com/k2fsa/sherpa/onnx/OfflineDiacritizationModelConfig.java`:
- Around line 24-30: Add the missing public getters to
OfflineDiacritizationModelConfig to match the existing
getCattEncoder()/getCattDecoder() and the builder setters: implement public int
getNumThreads() returning the existing numThreads field, public boolean
isDebug() (or getDebug() if you prefer JavaBean style) returning the debug
field, and public String getProvider() returning the provider field; keep the
same visibility and naming pattern as the existing getters so callers can
inspect the full configuration after construction.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d727feec-4644-4b95-b164-cae75af86e54
📒 Files selected for processing (10)
.github/workflows/run-java-test.yamljava-api-examples/OfflineAddDiacritization.javajava-api-examples/run-offline-add-diacritics.shsherpa-onnx/java-api/Makefilesherpa-onnx/java-api/src/main/java/com/k2fsa/sherpa/onnx/OfflineDiacritization.javasherpa-onnx/java-api/src/main/java/com/k2fsa/sherpa/onnx/OfflineDiacritizationConfig.javasherpa-onnx/java-api/src/main/java/com/k2fsa/sherpa/onnx/OfflineDiacritizationModelConfig.javasherpa-onnx/jni/CMakeLists.txtsherpa-onnx/jni/offline-diacritization.ccsherpa-onnx/jni/sherpa-onnx-symbols.exp
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
java-api-examples/run-offline-add-diacritics.sh (1)
35-35:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winQuote
java.library.pathto prevent argument splitting.Line 35 should quote
$PWDso paths with spaces do not break JVM option parsing.🐛 Suggested fix
- -Djava.library.path=$PWD/../build/lib \ + -Djava.library.path="$PWD/../build/lib" \🤖 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 `@java-api-examples/run-offline-add-diacritics.sh` at line 35, The JVM option setting for java.library.path currently uses an unquoted $PWD which can split into multiple arguments if the working directory contains spaces; update the -Djava.library.path assignment (the token starting with -Djava.library.path= and referencing $PWD) to wrap the path in double quotes (e.g., change -Djava.library.path=$PWD/../build/lib to use quotes around the $PWD/../build/lib expression) so the JVM receives the library path as a single argument.Source: Linters/SAST tools
sherpa-onnx/jni/offline-diacritization.cc (1)
114-123:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winValidate native pointer and UTF chars before dereference in
addDiacritics.Line 115 and Line 119 dereference values that can be null (
ptr,ptext). This is a JVM crash path. Also, the missingGetStringUTFCharsnull-check was already reported earlier and is still present.🐛 Suggested fix
JNIEXPORT jstring JNICALL Java_com_k2fsa_sherpa_onnx_OfflineDiacritization_addDiacritics(JNIEnv *env, jobject /*obj*/, jlong ptr, jstring text) { auto diacrt = reinterpret_cast<const sherpa_onnx::OfflineDiacritization *>(ptr); + if (diacrt == nullptr) { + env->ThrowNew(env->FindClass("java/lang/IllegalStateException"), + "OfflineDiacritization has been released or not initialized"); + return nullptr; + } + + if (text == nullptr) { + env->ThrowNew(env->FindClass("java/lang/NullPointerException"), + "text is null"); + return nullptr; + } const char *ptext = env->GetStringUTFChars(text, nullptr); + if (ptext == nullptr) { + return nullptr; // OOM/exception is already pending in JVM + } std::string result = diacrt->AddDiacritics(ptext); env->ReleaseStringUTFChars(text, ptext); return SafeNewStringUTF(env, result); }🤖 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 `@sherpa-onnx/jni/offline-diacritization.cc` around lines 114 - 123, Check for null before dereferencing both the native pointer and the UTF chars: verify the incoming ptr is non-zero before casting to const sherpa_onnx::OfflineDiacritization* (the variable diacrt) and, after calling env->GetStringUTFChars(text, nullptr), ensure ptext is non-null before using it; if either is null, throw an appropriate Java exception (e.g., NullPointerException for a null native pointer, OutOfMemoryError for GetStringUTFChars returning null) via env->ThrowNew and return nullptr, and only call env->ReleaseStringUTFChars(text, ptext) when ptext is non-null; update the addDiacritics native bridge to perform these checks around the reinterpret_cast, env->GetStringUTFChars, and the call to diacrt->AddDiacritics, and continue returning the result via SafeNewStringUTF only when no exception was thrown.
🤖 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 `@sherpa-onnx/jni/offline-diacritization.cc`:
- Around line 18-25: GetOfflineDiacritizationConfig currently calls
env->GetObjectClass(config) and env->GetObjectClass(model_config) without
checking for null; add null guards so a null Java config/model yields a clean
JNI exception instead of crashing: check if config is null before calling
env->GetObjectClass(config) and if so call
env->ThrowNew(env->FindClass("java/lang/IllegalArgumentException"), "config is
null") (or return an appropriate error/null), and after obtaining model_config
via env->GetObjectField(config, fid) check if model_config is null before
calling env->GetObjectClass(model_config) and similarly throw or return; apply
these checks around the env->GetObjectClass(config), env->GetObjectField(config,
fid) and env->GetObjectClass(model_config) usage in
GetOfflineDiacritizationConfig.
---
Duplicate comments:
In `@java-api-examples/run-offline-add-diacritics.sh`:
- Line 35: The JVM option setting for java.library.path currently uses an
unquoted $PWD which can split into multiple arguments if the working directory
contains spaces; update the -Djava.library.path assignment (the token starting
with -Djava.library.path= and referencing $PWD) to wrap the path in double
quotes (e.g., change -Djava.library.path=$PWD/../build/lib to use quotes around
the $PWD/../build/lib expression) so the JVM receives the library path as a
single argument.
In `@sherpa-onnx/jni/offline-diacritization.cc`:
- Around line 114-123: Check for null before dereferencing both the native
pointer and the UTF chars: verify the incoming ptr is non-zero before casting to
const sherpa_onnx::OfflineDiacritization* (the variable diacrt) and, after
calling env->GetStringUTFChars(text, nullptr), ensure ptext is non-null before
using it; if either is null, throw an appropriate Java exception (e.g.,
NullPointerException for a null native pointer, OutOfMemoryError for
GetStringUTFChars returning null) via env->ThrowNew and return nullptr, and only
call env->ReleaseStringUTFChars(text, ptext) when ptext is non-null; update the
addDiacritics native bridge to perform these checks around the reinterpret_cast,
env->GetStringUTFChars, and the call to diacrt->AddDiacritics, and continue
returning the result via SafeNewStringUTF only when no exception was thrown.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0e943169-109e-4f4d-a93e-42c8ccebbd63
📒 Files selected for processing (10)
.github/workflows/run-java-test.yamljava-api-examples/OfflineAddDiacritics.javajava-api-examples/run-offline-add-diacritics.shsherpa-onnx/java-api/Makefilesherpa-onnx/java-api/src/main/java/com/k2fsa/sherpa/onnx/OfflineDiacritization.javasherpa-onnx/java-api/src/main/java/com/k2fsa/sherpa/onnx/OfflineDiacritizationConfig.javasherpa-onnx/java-api/src/main/java/com/k2fsa/sherpa/onnx/OfflineDiacritizationModelConfig.javasherpa-onnx/jni/CMakeLists.txtsherpa-onnx/jni/offline-diacritization.ccsherpa-onnx/jni/sherpa-onnx-symbols.exp
✅ Files skipped from review due to trivial changes (1)
- sherpa-onnx/jni/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (6)
- sherpa-onnx/java-api/src/main/java/com/k2fsa/sherpa/onnx/OfflineDiacritizationConfig.java
- sherpa-onnx/java-api/src/main/java/com/k2fsa/sherpa/onnx/OfflineDiacritizationModelConfig.java
- sherpa-onnx/java-api/Makefile
- sherpa-onnx/jni/sherpa-onnx-symbols.exp
- .github/workflows/run-java-test.yaml
- sherpa-onnx/java-api/src/main/java/com/k2fsa/sherpa/onnx/OfflineDiacritization.java
csukuangfj
approved these changes
Jun 12, 2026
csukuangfj
left a comment
csukuangfj
left a comment
Collaborator
There was a problem hiding this comment.
Thank you for your contribution!
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
2 participants
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.
In commit matiaslin@a2b7ab9,
OfflineDiacritizationwas implemented forC-API,CXX-APIandPython.We provide the
JAVAbindings for this feature.Summary by CodeRabbit
New Features
Tests