Skip to content

[sqflite] New Sqflite Tizen plugin #276

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

Merged
merged 56 commits into from
Dec 9, 2021

Conversation

rmanganiello
Copy link
Contributor

@rmanganiello rmanganiello commented Nov 15, 2021

Sqflite Tizen plugin

Hey there 👋🏼 ! Here's an implementation of the SQFlite plugin I've been working on for Tizen!

I've been working on this for about ~2 months now and I'm happy to say that I have something functional! (there are some caveats to consider though).

Also, it's important to say that this is my first ever C++ project, I was learning the language while working on this so you'll probably find things to improve in the implementation, any feedback is welcomed!

How it works

I based the implementation on the Sqflite plugin written for Android, all the interfaces are respected which means that this is (almost) fully compatible with SQFlite plugin.

Functionalities

Functionality Supported Notes
Open Database ✔️
Close Database ✔️
Delete Database ✔️ ⚠️ Delete databases functionality is working properly, however, there's no such method in the SQLite3 package, which means we need to delete the file in the storage to accomplish the same behavior, there's no safe check to do this for now, which means the client can provide any file to delete and we will remove that. ⚠️
Get Databases Path ✔️
Options ✔️ Options functionality is working properly but there are some options that we don't support yet like thread priority (we need to check if thread priority makes sense for Tizen whenever we implement background thread functionality), and, we also need to double-check log level usage.
Query ✔️
Insert ✔️
Update ✔️
Batch ✔️
Debug ✔️
Log level usage ✔️
Background thread There's no current implementation to execute this in a background thread for now.

Caveats

  1. I tested this project on Watch and TV emulators.
  2. Open databases in a non-existing directory will fail in this implementation while in the original project succeeds.
  3. Example project was extracted from the original SQFlite project, all credits to the team! @alextekartik please let me know if there's something else I need to include to import your project, I included your LICENSE in the folder.

- Use Namespaces.
- Use correct naming for "#define guard".
- Use correct naming for "typedef"
@bbrto21
Copy link
Contributor

bbrto21 commented Nov 26, 2021

Please put periods at the end of log messages whenever possible.
Please reduce the log message as much as possible under normal state.

@bbrto21
Copy link
Contributor

bbrto21 commented Nov 26, 2021

Other things, looks good to me

@rmanganiello
Copy link
Contributor Author

Thanks for all the reviews! Let me know if you see something else @bbrto21 @swift-kim.

I can also work on adding the log level support, I can do it in this PR or in a different one, it's up to you

Copy link
Member

@swift-kim swift-kim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a quick style review.

std::unique_ptr<flutter::MethodResult<flutter::EncodableValue>> result) {
LOG_DEBUG("HandleMethodCall: %s", method_call.method_name().c_str());
const std::string method_name = method_call.method_name();
if (method_name == sqflite_constants::kMethodOpenDatabase) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's up to your preference but I personally prefer

Suggested change
if (method_name == sqflite_constants::kMethodOpenDatabase) {
if (method_name == "openDatabase") {

in terms of readability.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll keep the constants since I think is a good practice to preserve fixed strings in constant variables in case we need to use them somewhere else.

@bbrto21
Copy link
Contributor

bbrto21 commented Nov 29, 2021

I can also work on adding the log level support, I can do it in this PR or in a different one, it's up to you

I think it would be better to create a separate PR.

@rmanganiello
Copy link
Contributor Author

I just added Log Level support and apply all style guide changes suggested, thank you for the suggestions!

I just figured out there's an issue when opening the Open tests twice, it seems there's some bad memory access.
I couldn't find the issue yet... If you have some tips on how to debug it that would be really helpful.

@swift-kim
Copy link
Member

I just figured out there's an issue when opening the Open tests twice, it seems there's some bad memory access.

I managed to reproduce the issue on a mobile 6.0 emulator and here's the stack trace. I haven't looked into the details.

(lldb) Process 4780 stopped
* thread #1, name = 'Runner.dll', stop reason = signal SIGSEGV: invalid address (fault address: 0x2c)
  * frame #0: 0xb67b29de libsqlite3.so.0`sqlite3PcacheSetPageSize [inlined] numberOfCachePages(p=<unavailable>) at sqlite3.c:49120
    frame #1: 0xb67b29de libsqlite3.so.0`sqlite3PcacheSetPageSize(pCache=0x80b36218, szPage=-1233469155) at sqlite3.c:49207
    frame #2: 0xb67b2a88 libsqlite3.so.0`sqlite3PcacheMakeClean(p=<unavailable>) at sqlite3.c:49452
    frame #3: 0xb681ca71 libsqlite3.so.0`sqlite3PagerCommitPhaseOne at sqlite3.c:58268
    frame #4: 0xb681c630 libsqlite3.so.0`sqlite3PagerCommitPhaseOne(pPager=<unavailable>, zSuper="", noSync=-2135727592) at sqlite3.c:58075
    frame #5: 0xb681e509 libsqlite3.so.0`sqlite3WalCheckpoint at sqlite3.c:61017
    frame #6: 0xb681e4ff libsqlite3.so.0`sqlite3WalCheckpoint at sqlite3.c:61143
    frame #7: 0xb681e4ff libsqlite3.so.0`sqlite3WalCheckpoint at sqlite3.c:61397
    frame #8: 0xb681e408 libsqlite3.so.0`sqlite3WalCheckpoint(pWal=<unavailable>, db=<unavailable>, eMode=<unavailable>, xBusy=<unavailable>, pBusyArg=<unavailable>, sync_flags=<unavailable>, nBuf=<unavailable>, zBuf=<unavailable>, pnLog=<unavailable>, pnCkpt=<unavailable>) at sqlite3.c:63229
    frame #9: 0xb681e673 libsqlite3.so.0`sqlite3WalCheckpoint at sqlite3.c:61352
    frame #10: 0xb681e408 libsqlite3.so.0`sqlite3WalCheckpoint(pWal=<unavailable>, db=<unavailable>, eMode=<unavailable>, xBusy=<unavailable>, pBusyArg=<unavailable>, sync_flags=<unavailable>, nBuf=<unavailable>, zBuf=<unavailable>, pnLog=<unavailable>, pnCkpt=<unavailable>) at sqlite3.c:63229
    frame #11: 0xb681ebdd libsqlite3.so.0`sqlite3BtreeOpen [inlined] sqlite3MutexAlloc(id=2) at sqlite3.c:25942
    frame #12: 0xb681ebc8 libsqlite3.so.0`sqlite3BtreeOpen(pVfs=<unavailable>, zFilename=<unavailable>, db=<unavailable>, ppBtree=<unavailable>, flags=<unavailable>, vfsFlags=<unavailable>) at sqlite3.c:66912
    frame #13: 0x9c72cf00 libflutter_plugins.so`sqflite_database::DatabaseManager::Close(this=0x80cc4c0c) at database_manager.cc:80
    frame #14: 0x9c72ce27 libflutter_plugins.so`sqflite_database::DatabaseManager::~DatabaseManager(this=0x80cc4c0c) at database_manager.cc:21
    frame #15: 0x9c70931d libflutter_plugins.so`void __gnu_cxx::new_allocator<sqflite_database::DatabaseManager>::destroy<sqflite_database::DatabaseManager>(this=0x80cc4c0c, __p=0x80cc4c0c) at new_allocator.h:153
    frame #16: 0x9c709295 libflutter_plugins.so`void std::allocator_traits<std::allocator<sqflite_database::DatabaseManager> >::destroy<sqflite_database::DatabaseManager>(__a=0x80cc4c0c, __p=0x80cc4c0c) at alloc_traits.h:497
    frame #17: 0x9c7087e5 libflutter_plugins.so`std::_Sp_counted_ptr_inplace<sqflite_database::DatabaseManager, std::allocator<sqflite_database::DatabaseManager>, (__gnu_cxx::_Lock_policy)2>::_M_dispose(this=0x80cc4c00) at shared_ptr_base.h:557
    frame #18: 0xb65ec6a4 libsecurity-manager-commons.so.1`std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release() + 84
    frame #19: 0x9c6fc328 libflutter_plugins.so`std::__shared_count<(__gnu_cxx::_Lock_policy)2>::~__shared_count(this=0xbff352b4) at shared_ptr_base.h:730
    frame #20: 0x9c6fc2e9 libflutter_plugins.so`std::__shared_ptr<sqflite_database::DatabaseManager, (__gnu_cxx::_Lock_policy)2>::~__shared_ptr(this=0xbff352b0) at shared_ptr_base.h:1169
    frame #21: 0x9c6fc2b6 libflutter_plugins.so`std::shared_ptr<sqflite_database::DatabaseManager>::~shared_ptr(this=0xbff352b0) at shared_ptr.h:103
    frame #22: 0x9c6ff7d2 libflutter_plugins.so`SqflitePlugin::OnDeleteDatabase(this=0x80beda90, method_call=0x80cc5f80, result=<unavailable>) at sqflite_plugin.cc:498
    frame #23: 0x9c6fd4b8 libflutter_plugins.so`SqflitePlugin::HandleMethodCall(this=0x80beda90, method_call=0x80cc5f80, result=<unavailable>) at sqflite_plugin.cc:92
    frame #24: 0x9c6fd0ed libflutter_plugins.so`auto SqflitePlugin::RegisterWithRegistrar(this=0x80cc8500, call=0x80cc5f80, result=<unavailable>)::'lambda'(flutter::MethodCall<flutter::EncodableValue> const&, std::unique_ptr<flutter::MethodResult<flutter::EncodableValue>, std::default_delete<flutter::MethodResult<flutter::EncodableValue> > >)::operator()<flutter::MethodCall<flutter::EncodableValue>, std::unique_ptr<flutter::MethodResult<flutter::EncodableValue>, std::default_delete<flutter::MethodResult<flutter::EncodableValue> > > >(flutter::MethodCall<flutter::EncodableValue> const&, std::unique_ptr<flutter::MethodResult<flutter::EncodableValue>, std::default_delete<flutter::MethodResult<flutter::EncodableValue> > >) const at sqflite_plugin.cc:73
    frame #25: 0x9c6fceb6 libflutter_plugins.so`std::_Function_handler<void (flutter::MethodCall<flutter::EncodableValue> const&, std::unique_ptr<flutter::MethodResult<flutter::EncodableValue>, std::default_delete<flutter::MethodResult<flutter::EncodableValue> > >), SqflitePlugin::RegisterWithRegistrar(flutter::PluginRegistrar*)::'lambda'(void  const(&)(flutter::MethodCall<flutter::EncodableValue> const&, std::unique_ptr<flutter::MethodResult<flutter::EncodableValue>, std::default_delete<flutter::MethodResult<flutter::EncodableValue> > >), SqflitePlugin::RegisterWithRegistrar(flutter::PluginRegistrar*)::'lambda'(T_ const&, T0_))>::_M_invoke(__functor=0x80cc8500, __args=0x80cc5f80, __args=0xbff355a0) at std_function.h:300
    frame #26: 0x9c6f4645 libflutter_plugins.so`std::function<void (flutter::MethodCall<flutter::EncodableValue> const&, std::unique_ptr<flutter::MethodResult<flutter::EncodableValue>, std::default_delete<flutter::MethodResult<flutter::EncodableValue> > >)>::operator(this=0x80cc8500, __args=0x80cc5f80, __args=<unavailable>)(flutter::MethodCall<flutter::EncodableValue> const&, std::unique_ptr<flutter::MethodResult<flutter::EncodableValue>, std::default_delete<flutter::MethodResult<flutter::EncodableValue> > >) const at std_function.h:690
    frame #27: 0x9c6f421d libflutter_plugins.so`flutter::MethodChannel<flutter::EncodableValue>::SetMethodCallHandler(this=0x80cc8500, message="\a\x0edeleteDatabase\r\x01\a\x04path\aU/opt/usr/home/owner/apps_rw/org.tizen.sqflite_example/data/open_close_open_no_wait.dbsub_sub/open.1", message_size=111, reply=<unavailable>)>) const::'lambda'(unsigned char const*, unsigned int, std::function<void (unsigned char const*, unsigned int)>)::operator()(unsigned char const*, unsigned int, std::function<void (unsigned char const*, unsigned int)>) const at method_channel.h:119
    frame #28: 0x9c6f3c69 libflutter_plugins.so`std::_Function_handler<void (unsigned char const*, unsigned int, std::function<void (unsigned char const*, unsigned int)>), flutter::MethodChannel<flutter::EncodableValue>::SetMethodCallHandler(std::function<void (flutter::MethodCall<flutter::EncodableValue> const&, std::unique_ptr<flutter::MethodResult<flutter::EncodableValue>, std::default_delete<flutter::MethodResult<flutter::EncodableValue> > >)>) const::'lambda'(unsigned char const*, unsigned int, std::function<void (unsigned char const*, unsigned int)>)>::_M_invoke(__functor=0x80cc8238, __args=0xbff35694, __args=0xbff35698, __args=0xbff356e0)>&&) at std_function.h:300
    frame #29: 0x9c6b2249 libflutter_plugins.so`std::function<void (unsigned char const*, unsigned int, std::function<void (unsigned char const*, unsigned int)>)>::operator(this=0x80cc8238, __args="\a\x0edeleteDatabase\r\x01\a\x04path\aU/opt/usr/home/owner/apps_rw/org.tizen.sqflite_example/data/open_close_open_no_wait.dbsub_sub/open.1", __args=111, __args=<unavailable>)(unsigned char const*, unsigned int, std::function<void (unsigned char const*, unsigned int)>) const at std_function.h:690
    frame #30: 0x9c6b0247 libflutter_plugins.so`flutter::(anonymous namespace)::ForwardToHandler(messenger=0x80b83f50, message=0xbff35790, user_data=0x80cc8238) at core_implementations.cc:58

I can tell you how to get this stack trace if you really want.

Copy link
Member

@swift-kim swift-kim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick fix.

@rmanganiello
Copy link
Contributor Author

I can tell you how to get this stack trace if you really want.

@swift-kim I was thinking to create an Issue asking for best practices to debug Tizen plugins. I'll create it tomorrow, it'll be useful for other contributors as well.

@swift-kim
Copy link
Member

@rmanganiello The current best practice is just to use logs :D

I'm currently writing https://github.com/flutter-tizen/flutter-tizen/wiki/Debugging-and-profiling-the-engine#debugging-with-lldb and it will give you some useful information. You can use either a wearable or mobile emulator, and should download the libsqlite-debuginfo debug symbol for i686 in step 2. You can skip steps 3 and 4 and just attach lldb to a running process after launching the sqflite example app.

@rmanganiello
Copy link
Contributor Author

@rmanganiello The current best practice is just to use logs :D

I'm currently writing https://github.com/flutter-tizen/flutter-tizen/wiki/Debugging-and-profiling-the-engine#debugging-with-lldb and it will give you some useful information. You can use either a wearable or mobile emulator, and should download the libsqlite-debuginfo debug symbol for i686 in step 2. You can skip steps 3 and 4 and just attach lldb to a running process after launching the sqflite example app.

Thanks, this is really useful! I'll give it a try and see if I can fix this issue, it seems related with a bad pointer while trying to destroy the Database instance.

- Add mutex protection to static variables in SqflitePlugin such as
  `database_id_`, `database_map_` and `single_instances_by_path_`.

- Remove usages of `DatabaseManager::Init` since it shuts down the
database and invalidate stored Databases between executions.
@rmanganiello
Copy link
Contributor Author

Just fixed the issue @bbrto21 and @swift-kim, I was shutting down sqlite each time a new Database instance was created and it seems that corrupted previous stored connections.

I also added a mutex to protect static variables in the plugin as the original library is doing.

Please take a look and thanks for the patience

Copy link
Member

@swift-kim swift-kim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you for your hard work!

@swift-kim swift-kim requested a review from a team December 7, 2021 02:56
Copy link
Contributor

@bbrto21 bbrto21 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@bbrto21 bbrto21 merged commit a3894d4 into flutter-tizen:master Dec 9, 2021
@bbrto21
Copy link
Contributor

bbrto21 commented Dec 9, 2021

published

bwikbs pushed a commit to bwikbs/plugins that referenced this pull request Mar 26, 2022
* WIP

* Remove unused folder

* Rename test

* Add update/insert/query functionalities

* Fix static variables and onDelete

* Start throwing exceptions in DatabaseManager

* Make batch operations work

* Fix batch call

* Fix OnOpenDatabaseCall and open readOnly database

* RAII for db, support blobs/lists and simplify code

* Handle query exceptions correctly

* Handle empty resultset response while querying

* Fix no-rowid tests for tizen

* Remove usages of deprecated `debugModeOn` in raw_test_page

* Use correct error for open failed exception

* Add assets to test from asset db

* Fix more tests and change open sub folder test to fail

* Fix exp tests

* Simplify code

* Create constants

* Fix manual tests and add OnDebugCall support

* Check for allowed permissions properly

* Remove `using`

* Apply `expect` implementation fix from sqflite

* Update README

* Update README

* Update README.md

* Remove unneded tests

* Remove unneded example project dependencies

* Remove test_driver folder

* Remove unneded files and variables

* Add new line EOF

* Fix manifest and remove useless folder

* Add new lines at EOF

* Add new line at EOF

* Cleanup code

* Use constant

* Add SQFlite LICENSE for example project

* Move to MINOR version

* Add integration tests

* Fix Dart issues

* Add sqflite to recipe.yaml for integration tests

* Remove permission check for mediastorage

This permission is not needed to run the application since we store
databases in the internal app directory and not in the mediastorage

* Set sqlite3_busy_timeout and add tv target

* Apply Style guide

* Convert to snake_case

* Move static members to end of class

* Remove unneded methods

* Remove deprecated constant and move struct out of class

* Apply more style guide rules

- Use Namespaces.
- Use correct naming for "#define guard".
- Use correct naming for "typedef"

* Remove logs, fix licenses and apply suggested comments

* Add log_level support and style guide changes

* Fix variables naming database manager header

* Fix database_manager header private variables order

* Remove unused imports

* Include `mutex` support and fix DatabaseManager

- Add mutex protection to static variables in SqflitePlugin such as
  `database_id_`, `database_map_` and `single_instances_by_path_`.

- Remove usages of `DatabaseManager::Init` since it shuts down the
database and invalidate stored Databases between executions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-publishing The package should be published after merge tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants