RNMT-4515 Fixes an issue with recent devices running Android 11 #1

luissilvaos · 2020-11-25T17:49:27Z

Description

This PR introduces the changes that fixes an issue with the recent devices running Android 11, by introducing a complex type for the representation of the SQL Handle and the SQLite operation result, instead of mixing both values into a long value.

Context

Fixes https://outsystemsrd.atlassian.net/browse/RNMT-4515

The previous approach was using a simple type to return the SQL handle. The returned handle value was a mix between the result value of SQLite operation and the SQL handle, where negative number indicates an error, causing the issues in the recent devices running Android 11, where the memory addresses of the handle were valid but they were also negative numbers when converted to the `long long` type. With this new complex type for the SQL Handle, we are now able to return both the result of the SQLite operation and the SQL handle. References https://outsystemsrd.atlassian.net/browse/RNMT-4515

References https://outsystemsrd.atlassian.net/browse/RNMT-4515

Chuckytuh · 2020-11-25T20:49:46Z

jni/Application.mk

 # For future consideration (needs testing):
 #APP_ABI += mips mips64
-
+APP_PLATFORM := android-23


Are we gaining anything from this bump ? Also, reading the docs https://developer.android.com/ndk/guides/application_mk#app_platform it states that this should match the minSdkVersion , 23 is Android 6 but we support Android 5.1 , right ? If there's no gain from this being set as we, I don't see a reason for this to be break change by bumping this to 23

No, we only support Android 6+ in MABS 7. Both 5 and 5.1 were dropped.

So we're just introducing a breaking change without actually needing it on this package, right? What if tomorrow we need to fix something here that's needs to be applied also on MABS 5, we'll have to deal with release branches and whatnot for no reason

I understand your point, but to be honest, I don't see it as a problem. Here are my thoughts regarding this topic:

We already have breaking changes in the cordova-sqlite-storage plugin for MABS 7.

The current version used by MABS 6 has been a stable version.

If we somehow have to deal with some fixes in the lib versions that support cordova-sqlite-storage for MABS 6 we can just create a branch for that.

Also, the changes introduced in this PR aren't a simple fix that we can simply port to MABS 6. IMHO, they should only be applied to the version that will be released with MABS 7, due to the eventual impacts that they may cause.

Chuckytuh · 2020-11-25T20:54:06Z

native/sqlc.c

+#define HANDLE_FROM_VP(p) (( (unsigned char *)(p) - (unsigned char *)NULL ) )
+#define HANDLE_TO_VP(h) (void *)( (unsigned char *)NULL + (ptrdiff_t)((h)) )


I guess this no longer is required. Or is there something to it ?

I removed the BASE_OFFSET and left it there to be an helper for the conversions pointer -> long and long -> pointer, and to minimize the changes in the code as well.

It's no longer required because it's no longer +number = pointer -number = error.

EiyuuZack

LGTM. What a headache this must have been considering upstream has not be updated in years...

oscsx · 2020-11-26T09:48:11Z

java/io/liteglue/SQLiteResponse.java

+        return this.handle;
+    }
+
+}


Missing newline at the end

oscsx · 2020-11-26T10:00:57Z

native/SQLiteNative_JNI.c

-  return _res;
+
+  jclass class = (*env)->FindClass(env,"io/liteglue/SQLiteResponse");
+  jmethodID constructor = (*env)->GetMethodID(env, class, "<init>", "(IJ)V");


"(IJ)V"? What is this? Is it really a static thing?

It's the convention for the method signature. Check this.

I will never again complain about JavaScript 😂

oscsx · 2020-11-26T11:02:36Z

native/SQLiteNative_JNI.c

@@ -143,19 +150,26 @@ Java_io_liteglue_SQLiteNative_sqlc_1db_1open__Ljava_lang_String_2I(JNIEnv *env,
  if ( NULL != filename ) {
    (*env)->ReleaseStringUTFChars(env, filename, _strchars_filename);
  }
-  return _res;
+
+  jclass class = (*env)->FindClass(env,"io/liteglue/SQLiteResponse");


Maybe we can refactor this code to avoid duplication?

Sure, that will be a good improvement.

Chuckytuh · 2020-11-26T11:18:39Z

Just adding context that this relates with storesafe/cordova-sqlite-storage#954

Merge commit 'a69fa8fea29d77b781b1250554932174328ff337' of https://github.com/OutSystems/Android-sqlite-native-driver into main include some updates from this PR on the OutSystems fork: - OutSystems/Android-sqlite-native-driver#1

This contains changes to the native C code.

…ned structs This was done manually in OutSystems/Android-sqlite-native-driver#1 and is done with Gluegen in this commit.

See OutSystems/Android-sqlite-native-driver#1

luissilvaos added 5 commits November 25, 2020 17:30

feat: add a java class to match the complex type for the SQL handle

5d4f4a5

References https://outsystemsrd.atlassian.net/browse/RNMT-4515

feat: update JNI to use the complex type for the SQL handle

a69fa8f

References https://outsystemsrd.atlassian.net/browse/RNMT-4515

chore: update sqlite-amalgamation to version 3.26.0

e52eca7

References https://outsystemsrd.atlassian.net/browse/RNMT-4515

build: update target platform and architectures

3cf482d

References https://outsystemsrd.atlassian.net/browse/RNMT-4515

luissilvaos self-assigned this Nov 25, 2020

luissilvaos requested review from andredestro, Chuckytuh, cmfsotelo, EiyuuZack, pevidex, tiagoappereira, usernuno and vmfo22 November 25, 2020 17:50

luissilvaos mentioned this pull request Nov 25, 2020

RNMT-4515 Fixes an issue with recent devices running Android 11 OutSystems/Android-sqlite-connector#1

Merged

Chuckytuh reviewed Nov 25, 2020

View reviewed changes

luissilvaos requested a review from oscsx November 26, 2020 09:41

Chuckytuh approved these changes Nov 26, 2020

View reviewed changes

EiyuuZack approved these changes Nov 26, 2020

View reviewed changes

luissilvaos merged commit f8d156b into outsystems Nov 26, 2020

luissilvaos deleted the feat/RNMT-4515/support-android11 branch November 26, 2020 10:08

luissilvaos mentioned this pull request Nov 26, 2020

RNMT-4515 Revert #7 and update dependency libs to fully support Android 11 OutSystems/Cordova-sqlite-storage#9

Merged

oscsx reviewed Nov 26, 2020

View reviewed changes

laurentperez mentioned this pull request Feb 8, 2021

Android SDK Target 30 + OPEN database: x FAILED storesafe/cordova-sqlite-storage#954

Closed

gauthierm added a commit to silverorange/Android-sqlite-ext-native-driver that referenced this pull request Sep 8, 2021

Backport fix from OutSystems/Android-sqlite-native-driver#1

dd59749

This contains changes to the native C code.

gauthierm added a commit to silverorange/react-native-sqlite-storage that referenced this pull request Sep 8, 2021

Add new libs and connector to fix bug in Android 11 SDK 30

b4ab830

See OutSystems/Android-sqlite-native-driver#1

octavezero mentioned this pull request Nov 11, 2021

Patch and update the android native libraries for API 30 andpor/react-native-sqlite-storage#494

Open

ArGeoph pushed a commit to ArGeoph/react-native-sqlite-storage that referenced this pull request Feb 14, 2023

Add new libs and connector to fix bug in Android 11 SDK 30

2de55a1

See OutSystems/Android-sqlite-native-driver#1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RNMT-4515 Fixes an issue with recent devices running Android 11 #1

RNMT-4515 Fixes an issue with recent devices running Android 11 #1

luissilvaos commented Nov 25, 2020

Chuckytuh Nov 25, 2020

usernuno Nov 25, 2020

Chuckytuh Nov 25, 2020 •

edited

luissilvaos Nov 26, 2020 •

edited

Chuckytuh Nov 25, 2020

luissilvaos Nov 26, 2020

gauthierm Sep 8, 2021

EiyuuZack left a comment

oscsx Nov 26, 2020

oscsx Nov 26, 2020

luissilvaos Nov 26, 2020

oscsx Nov 26, 2020

oscsx Nov 26, 2020

luissilvaos Nov 26, 2020

Chuckytuh commented Nov 26, 2020

		#define HANDLE_FROM_VP(p) (( (unsigned char )(p) - (unsigned char )NULL ) )
		#define HANDLE_TO_VP(h) (void )( (unsigned char )NULL + (ptrdiff_t)((h)) )

RNMT-4515 Fixes an issue with recent devices running Android 11 #1

RNMT-4515 Fixes an issue with recent devices running Android 11 #1

Conversation

luissilvaos commented Nov 25, 2020

Description

Context

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chuckytuh Nov 25, 2020 • edited

Choose a reason for hiding this comment

luissilvaos Nov 26, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EiyuuZack left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chuckytuh commented Nov 26, 2020

Chuckytuh Nov 25, 2020 •

edited

luissilvaos Nov 26, 2020 •

edited