Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect conversion in JNI for non-nullable built-in Blob type #929

Closed
Blastbit513 opened this issue May 31, 2021 · 4 comments · Fixed by #935
Closed

Incorrect conversion in JNI for non-nullable built-in Blob type #929

Blastbit513 opened this issue May 31, 2021 · 4 comments · Fixed by #935
Assignees
Labels
bug Something isn't working java

Comments

@Blastbit513
Copy link

The built-in Blob type represents std::shared_ptr< std::vector<uint8_t> > in Cpp and byte array in Java.
So Blob can't be nullable in java and must translate to empty array unless it marked @ Nullable (Blob?) type which is std::optional< std::shared_ptr< std::vector<uint8_t> > > in Cpp.

JNI converter converts an empty shared_ptr to null that looks wrong and leads to a runtime exception.
gluecodium/src/main/resources/templates/jni/utils/JniCppConversionUtilsImplementation.mustache

JniReference<jbyteArray>
convert_to_jni( JNIEnv* env, const std::shared_ptr< std::vector< uint8_t > >& nvalue )
{
    if ( !nvalue )
    {
        return {};
    }

    jsize size = static_cast< jsize >( nvalue->size( ) );
    auto jresult = make_local_ref(env, env->NewByteArray( size ));
    const jbyte* jbuffer = reinterpret_cast< const jbyte* >( nvalue->data( ) );
    env->SetByteArrayRegion( jresult.get(), 0, size, jbuffer );

    return jresult;
}

I would suggest converting an empty pointer to an empty java bytearray like in the patch below

diff --git a/gluecodium/src/main/resources/templates/jni/utils/JniCppConversionUtilsImplementation.mustache b/gluecodium/src/main/resources/templates/jni/utils/JniCppConversionUtilsImplementation.mustache
index a6fedb659..f33eb19d8 100644
--- a/gluecodium/src/main/resources/templates/jni/utils/JniCppConversionUtilsImplementation.mustache
+++ b/gluecodium/src/main/resources/templates/jni/utils/JniCppConversionUtilsImplementation.mustache
@@ -194,7 +194,7 @@ convert_to_jni( JNIEnv* env, const std::shared_ptr< std::vector< uint8_t > >& nv
 {
     if ( !nvalue )
     {
-        return {};
+        return make_local_ref(env, env->NewByteArray( 0 ));
     }

     jsize size = static_cast< jsize >( nvalue->size( ) );

@Blastbit513 Blastbit513 changed the title Bug in Blob conversion in JNI part Incorrect conversion in JNI for non-nullable built-in Blob type May 31, 2021
@DanielKamkha DanielKamkha added bug Something isn't working java good-first-issue Good first issue for starting with the project labels May 31, 2021
@DanielKamkha
Copy link
Contributor

Nullable Blobs also need to be tested for Swift and Dart generators, in case some similar bug is also present there.

@DanielKamkha DanielKamkha removed the good-first-issue Good first issue for starting with the project label May 31, 2021
@DanielKamkha DanielKamkha self-assigned this May 31, 2021
@DanielKamkha
Copy link
Contributor

DanielKamkha commented May 31, 2021

Having a null shared pointer coming from C++ when the type is specified as non-nullable in the IDL is a violation of contract. This specific issue with Blobs only happens because there is no compiler-level mechanism to enforce this contract in C++. The contract is expresses with a @NotNull documentation comment in the generated C++ code. But then the C++ implementer is free to ignore it.

The real question is how to handle such contract violations in the conversion code. Current "approach" in JNI is to silently ignore it. If we continue to use this approach, then the proposed fix for the Blobs issue is correct. But do we? What if we throw instead (for Blobs and for classes/interfaces)? @Hsilgos, any preferences?

@DanielKamkha
Copy link
Contributor

The whole Blob type looks to be controversial, on all levels (C++, JNI, Java). There's also no consensus on how to improve it. Therefore, implementing the proposed fix as a non-breaking stop-gap measure for now.

DanielKamkha added a commit that referenced this issue Jun 1, 2021
Updated JNI conversion function for non-nullable Blobs to avoid returning `null` when the C++ function returns a null
shared pointer.

Having a null shared pointer coming from C++ when the type is specified as non-nullable in the IDL is a violation of
contract. The correct fix for this issue would be to change `Blob` representation from `shared_ptr<vector>` to something
that cannot be `null`. However, this would be a breaking change, so it has to be done later (#934). For now it's just
the small JNI workaround.

Resolves: #929
Signed-off-by: Daniel Kamkha <[email protected]>
@DanielKamkha
Copy link
Contributor

Created #934 for future Blob improvements.

DanielKamkha added a commit that referenced this issue Jun 1, 2021
Updated JNI conversion function for non-nullable Blobs to avoid returning `null` when the C++ function returns a null
shared pointer.

Having a null shared pointer coming from C++ when the type is specified as non-nullable in the IDL is a violation of
contract. The correct fix for this issue would be to change `Blob` representation from `shared_ptr<vector>` to something
that cannot be `null`. However, this would be a breaking change, so it has to be done later (#934). For now it's just
the small JNI workaround.

Resolves: #929
Signed-off-by: Daniel Kamkha <[email protected]>
DanielKamkha added a commit that referenced this issue Jun 1, 2021
Updated JNI conversion function for non-nullable Blobs to avoid returning `null` when the C++ function returns a null
shared pointer.

Having a null shared pointer coming from C++ when the type is specified as non-nullable in the IDL is a violation of
contract. The correct fix for this issue would be to change `Blob` representation from `shared_ptr<vector>` to something
that cannot be `null`. However, this would be a breaking change, so it has to be done later (#934). For now it's just
the small JNI workaround.

Resolves: #929
Signed-off-by: Daniel Kamkha <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working java
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants