chore: bump jni from 0.21.1 to 0.22.4 - #6274
Conversation
Bumps [jni](https://github.com/jni-rs/jni-rs) from 0.21.1 to 0.22.4. - [Release notes](https://github.com/jni-rs/jni-rs/releases) - [Changelog](https://github.com/jni-rs/jni-rs/blob/master/CHANGELOG.md) - [Commits](jni-rs/jni-rs@v0.21.1...v0.22.4) --- updated-dependencies: - dependency-name: jni dependency-version: 0.22.4 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
prql-bot
left a comment
prql-bot
left a comment
There was a problem hiding this comment.
The version bump on its own doesn't compile — jni 0.22 is a breaking API redesign, and prqlc/bindings/java/src/lib.rs still uses the 0.21 shapes. cargo check -p prql-java on this branch fails with 9 errors; the same command on the merge base (b3fbb00e) is clean, so this is the bump rather than something pre-existing.
Two independent breaks:
JNIEnvis nowEnvUnowned, an FFI-only wrapper with none of the JNI methods on it.get_string,new_string,find_classandthrow_newmoved toEnv, which you can only reach inside anEnvUnowned::with_env(...)closure whose outcome is mapped through anErrorPolicy.jni_sys::jbooleanchanged fromu8tobool(jni-sys #23), soformat != 0andsignature != 0no longer type-check.
I'm pushing the migration to this branch rather than just describing it — Dependabot won't act on a review. Java-visible behavior is unchanged: an unknown target dialect still throws IllegalArgumentException, a compile error still throws java.lang.Exception, and the native methods still return a null reference in both cases (JString::default() is JObject::null()). One incidental improvement falls out of with_env: a panic inside prqlc::compile is now caught and converted to a Java exception instead of unwinding out of an extern "system" function and aborting the process.
Verified with cargo check, cargo clippy -p prql-java --all-targets -- -D warnings, and ./mvnw test in prqlc/bindings/java/java/ — 3 tests pass, including a new one asserting the IllegalArgumentException path, since that's the exception type the migration re-implements by hand.
Holding approval until CI is green on the pushed commit.
Notes and alternatives
Alternative worth a maintainer's call: the Java bindings are publish = false and the publish-prql-java release job is commented out, so staying on jni 0.21 and telling Dependabot to ignore this crate is a defensible option too. I went with migrating because the rest of the tree tracks dependencies forward, but closing this in favour of an ignore entry is a reasonable call.
jboolean as bool. This is an upstream soundness tightening rather than something the bindings control: JNI's jboolean is an 8-bit value where the spec only guarantees JNI_FALSE == 0, so a JVM passing something other than 0 or 1 for a native-method argument would now be constructing an invalid bool. HotSpot normalises native-method jboolean arguments to 0/1, and there is no way to opt out short of pinning jni 0.21, so this is noted rather than actionable.
MSRV. jni 0.22.4 declares rust-version = "1.85.0", matching the workspace's rust-version, so test-msrv shouldn't be affected.
Why CI didn't say this first. Every job on the tests run for this commit is CANCELLED or SKIPPED — the run was replaced while the Dependabot batch was opening. test-java is gated on the prqlc/bindings/java/** path filter, which prqlc/bindings/java/Cargo.toml matches, so a run that isn't cancelled would have caught this.
jni 0.22 replaces the `JNIEnv` native-method argument with `EnvUnowned`, which exposes no JNI methods; the methods now live on `Env`, reachable only inside an `EnvUnowned::with_env` closure whose outcome is mapped through an `ErrorPolicy`. `jni_sys::jboolean` also became `bool` rather than `u8`, so the `!= 0` conversions no longer type-check. Java-visible behavior is unchanged: an unknown target dialect still throws IllegalArgumentException, a compile error still throws java.lang.Exception, and both paths still return a null reference (`JString::default()` is `JObject::null()`). Panics inside the compiler are now caught by `with_env` and converted to a Java exception rather than unwinding out of an `extern "system"` function. Adds a test for the IllegalArgumentException path, which the migration re-implements by hand.
prql-bot
left a comment
prql-bot
left a comment
There was a problem hiding this comment.
test-java is green on both runners at 4fedae9, and the workspace-wide clippy step in test-rust (--all-targets ... -D warnings) covers prql-java as a workspace member — so the migration is lint-clean under CI, not just locally. That clears the hold I put on this earlier.
One cleanup left, inline. java_string_with_exception resolves java/lang/Exception with find_class before throwing it, but Env::throw_new takes a class descriptor directly — throw_illegal_argument, fourteen lines above, already throws that way. The find_class call and the local reference it creates are redundant, and the Ok(())/Err(e) mapping around it is a second copy of that helper's body. I verified the replacement locally: cargo fmt --check and cargo clippy -p prql-java --all-targets -- -D warnings are clean, and ./mvnw test still passes 3/3 — including compileWithError, which is the test that exercises this exact throw path. I'm not pushing it: CI is already green, so a cleanup commit would cost a full review round for a cosmetic gain.
Not approving, and this one is my judgment rather than a repo rule. The substantive code here is a commit I pushed, so an approval from this account would restate my own confidence rather than add an independent one — and the migrate-here-or-pin-jni question from my earlier review is still an open maintainer call. Leaving the verdict to a human.
Error paths I traced against the jni 0.22.4 source
Java-visible behaviour is preserved on every path I could construct:
- null
Stringargument —JString::try_to_stringreturnsError::NullPtrfor a null reference (documented onmutf8_chars), same as 0.21'sget_string, sojstring_to_stringstill throwsIllegalArgumentException. - unknown target —
IllegalArgumentException, now covered by the newcompileWithUnknownTargettest. - compile error —
java.lang.Exception, unchanged. - panic in
prqlc::compile—with_envwraps the closure incatch_unwind, andThrowRuntimeExAndDefault::on_panicconverts it to aRuntimeException. Previously this unwound out of anextern "system"fn and aborted the JVM. - return value on error —
JString'sDefaultisJObject::null(), so the native methods still hand back a null reference alongside the pending exception.
ThrowRuntimeExAndDefault::on_error calls env.exception_check() first and returns the default without throwing when an exception is already pending, so the specific exception thrown inside the closure is the one Java sees. The doc comment on throw_illegal_argument is accurate about that.
One note on the Ok(()) => arms: throw_new delegates to throw_new_optional, which returns Err(Error::JavaException) on success and Err(Error::ThrowFailed(res)) on failure — it has no Ok path at all in 0.22.4. So those arms are unreachable. They're harmless defensive code and the comment above them already states the real contract, so I've left them alone rather than suggesting a change that only removes a branch the type system still requires.
| Err(err) => { | ||
| let message = err.to_string(); | ||
| match env.find_class("java/lang/Exception") { | ||
| Ok(exception) => { | ||
| if let Err(e) = env.throw_new(exception, message) { | ||
| eprintln!("Error throwing exception: {e:?}"); | ||
| } | ||
| } | ||
| Err(e) => { | ||
| eprintln!("Error finding java/lang/Exception: {e:?}"); | ||
| } | ||
| } | ||
| std::ptr::null_mut() | ||
| let exception = env.find_class(jni_str!("java/lang/Exception"))?; | ||
| Err(match env.throw_new(exception, JNIString::from(message)) { | ||
| Ok(()) => JniError::JavaException, | ||
| Err(e) => e, | ||
| }) | ||
| } |
There was a problem hiding this comment.
throw_new accepts anything that implements Desc<JClass>, which includes the &JNIStr that jni_str! produces — that's how throw_illegal_argument throws java/lang/IllegalArgumentException a few lines up. So the find_class lookup and the JClass local reference it returns aren't needed here.
| Err(err) => { | |
| let message = err.to_string(); | |
| match env.find_class("java/lang/Exception") { | |
| Ok(exception) => { | |
| if let Err(e) = env.throw_new(exception, message) { | |
| eprintln!("Error throwing exception: {e:?}"); | |
| } | |
| } | |
| Err(e) => { | |
| eprintln!("Error finding java/lang/Exception: {e:?}"); | |
| } | |
| } | |
| std::ptr::null_mut() | |
| let exception = env.find_class(jni_str!("java/lang/Exception"))?; | |
| Err(match env.throw_new(exception, JNIString::from(message)) { | |
| Ok(()) => JniError::JavaException, | |
| Err(e) => e, | |
| }) | |
| } | |
| Err(err) => { | |
| let class = jni_str!("java/lang/Exception"); | |
| let message = JNIString::from(err.to_string()); | |
| Err(match env.throw_new(class, message) { | |
| Ok(()) => JniError::JavaException, | |
| Err(e) => e, | |
| }) | |
| } |
1 participant
Bumps jni from 0.21.1 to 0.22.4.
Release notes
Sourced from jni's releases.
... (truncated)
Changelog
Sourced from jni's changelog.
... (truncated)
Commits
5ae9458Release jni 0.22.42f954cdFix copy&paste error s/JString::collection/JString::as_char_sequence/33045a1Release jni-macros 0.22.4527703eNo longer recommend passing&mut Envas the last argumentce7130bImport docs/macros/jni_mangle.md docs for jni_mangle macrod80bf23Add more-ergonomic JValueOwned accessors5ffd96abind_java_type: Support #[cfg()] guarded methods/fieldsb498e9fbind_java_type: support non_null methods/fields1f74e4bAddobjects::JCharSequencebinding25f810dRelease jni 0.22.3Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting
@dependabot rebase.Dependabot commands and options
You can trigger Dependabot actions by commenting on this PR:
@dependabot rebasewill rebase this PR@dependabot recreatewill recreate this PR, overwriting any edits that have been made to it@dependabot show <dependency name> ignore conditionswill show all of the ignore conditions of the specified dependency@dependabot ignore this major versionwill close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)@dependabot ignore this minor versionwill close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)@dependabot ignore this dependencywill close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)