Draft
Conversation
Contributor
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces Flocon SDK 2.0.0, a significant architectural overhaul aimed at enhancing performance, flexibility, and cross-platform compatibility. The core change involves transitioning to a modular, plugin-based system, which streamlines integration and reduces overhead. This release also emphasizes Kotlin Multiplatform support, making Flocon a more versatile tool for modern application development. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request represents a significant and impressive architectural overhaul of the Flocon SDK, moving towards a more modular and extensible design with a Ktor-like plugin system. The introduction of dedicated modules for features like Database, Network, and Deeplinks, along with their -no-op variants, is a major step forward for production safety and binary size reduction. The adoption of Kotlin Multiplatform for key components is also a great direction.
However, the refactoring appears to be incomplete in several areas. Key functionalities, such as the Android and iOS database query implementations and the DataStore integration, are left with TODO() markers or commented-out code. I've also identified some design concerns, particularly around the use of static singletons for plugin management and a fragile JSON-in-JSON protocol for database results. Addressing these points will be crucial for ensuring the stability and maintainability of the new 2.0 architecture.
Comment on lines
+5
to
+46
| actual fun openDbAndExecuteQuery( | ||
| path: String, | ||
| query: String | ||
| ): DatabaseExecuteSqlResponse { | ||
| // var helper: SupportSQLiteOpenHelper? = null | ||
| // return try { | ||
| // val path = context.getDatabasePath(databaseName) | ||
| // val version = getDatabaseVersion(path = path.absolutePath) | ||
| // helper = FrameworkSQLiteOpenHelperFactory().create( | ||
| // SupportSQLiteOpenHelper.Configuration.builder(context) | ||
| // .name(path.absolutePath) | ||
| // .callback(object : SupportSQLiteOpenHelper.Callback(version) { | ||
| // override fun onCreate(db: SupportSQLiteDatabase) { | ||
| // // no op | ||
| // } | ||
| // | ||
| // override fun onUpgrade( | ||
| // db: SupportSQLiteDatabase, | ||
| // oldVersion: Int, | ||
| // newVersion: Int | ||
| // ) { | ||
| // // no op | ||
| // } | ||
| // }) | ||
| // .build() | ||
| // ) | ||
| // val database = helper.writableDatabase | ||
| // | ||
| // executeSQL( | ||
| // database = database, | ||
| // query = query, | ||
| // ) | ||
| // } catch (t: Throwable) { | ||
| // DatabaseExecuteSqlResponse.Error( | ||
| // message = t.message ?: "error on executeSQL", | ||
| // originalSql = query, | ||
| // ) | ||
| // } finally { | ||
| // helper?.close() | ||
| // } | ||
| TODO() | ||
| } No newline at end of file |
Contributor
There was a problem hiding this comment.
The actual implementation of openDbAndExecuteQuery for Android is currently a TODO(), with the previous implementation commented out. This is a critical piece of functionality for the database inspection feature that needs to be implemented for the module to be functional on Android.
Comment on lines
+49
to
+68
| fun DatabaseExecuteSqlResponse.toJson(): String { | ||
| val jsonEncoder = FloconEncoder.json | ||
| val thisAsJson = jsonEncoder.encodeToJsonElement(this) | ||
|
|
||
| val type = when (this) { | ||
| is DatabaseExecuteSqlResponse.Error -> "Error" | ||
| is DatabaseExecuteSqlResponse.Insert -> "Insert" | ||
| DatabaseExecuteSqlResponse.RawSuccess -> "RawSuccess" | ||
| is DatabaseExecuteSqlResponse.Select -> "Select" | ||
| is DatabaseExecuteSqlResponse.UpdateDelete -> "UpdateDelete" | ||
| } | ||
|
|
||
| return buildJsonObject { | ||
| put("type", type) | ||
| put( | ||
| "body", | ||
| thisAsJson.toString() | ||
| ) // warning : the desktop is waiting for a string representation of the json here | ||
| }.toString() | ||
| } |
Contributor
There was a problem hiding this comment.
The toJson() extension function serializes the DatabaseExecuteSqlResponse object into a JSON string, and then embeds that string inside another JSON object under the body key. This JSON-in-JSON string format is fragile, inefficient, and makes parsing on the client side unnecessarily complex. The comment on line 66 indicates this is intentional, but it would be a significant improvement to the protocol to refactor this. The QueryResultDataModel on the desktop side should be updated to expect a proper nested JSON object for the result field, not an escaped string.
Comment on lines
+92
to
+98
| private fun executeUpdateDelete(connection: SQLiteConnection, query: String): DatabaseExecuteSqlResponse { | ||
| connection.prepare(query).use { statement -> | ||
| statement.close() | ||
| } | ||
| // sqlite-kt n'expose pas encore `changes()`, on renvoie 0 | ||
| return DatabaseExecuteSqlResponse.UpdateDelete(affectedCount = 0) | ||
| } |
Contributor
There was a problem hiding this comment.
The executeUpdateDelete function for iOS always returns an affectedCount of 0. The comment indicates this is due to a limitation in sqlite-kt. While this might be a library constraint, it results in incorrect data being displayed to the user. If it's not possible to get the actual count, this limitation should be clearly documented. However, it's worth investigating if there's a way to execute a SELECT changes() query after the statement to get the number of affected rows, similar to how last_insert_rowid() is used for inserts.
Comment on lines
+100
to
+113
| private fun executeInsert(connection: SQLiteConnection, query: String): DatabaseExecuteSqlResponse { | ||
| connection.prepare(query).use { statement -> | ||
| statement.close() | ||
| } | ||
|
|
||
| // Récupération du dernier ID inséré | ||
| var id = -1L | ||
| connection.prepare("SELECT last_insert_rowid()").use { | ||
| // id = if (it.step()) it.getLong(0) else -1L | ||
| // it.close() // maybe remove | ||
| } | ||
|
|
||
| return DatabaseExecuteSqlResponse.Insert(id) | ||
| } |
Contributor
There was a problem hiding this comment.
The iOS implementation for executeInsert is incomplete. The logic to retrieve the last_insert_rowid() is commented out, causing the function to always return the hardcoded value of -1L. This is incorrect and will provide misleading information to the user. The commented-out code should be restored and fixed to correctly report the ID of the inserted row.
Comment on lines
+99
to
+105
| developers { | ||
| developer { | ||
| id = "openflocon" | ||
| name = "Open Flocon" | ||
| url = "https://github.com/openflocon" | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
There is a nested developers block within the pom configuration. This is a typo and will likely lead to an incorrectly generated POM file, which could cause issues during publication to Maven Central.
developer {
id = "openflocon"
name = "Open Flocon"
url = "https://github.com/openflocon"
}
Comment on lines
+22
to
+90
| //class FloconDatastorePreference( | ||
| // override val name: String, | ||
| // private val dataStore: DataStore<Preferences>, | ||
| // private val mapper: FloconDatastoreMapper? = null // if we encrypted the datastore | ||
| //) : FloconPreference { | ||
| // | ||
| // override suspend fun set( | ||
| // columnName: String, | ||
| // FloconPreferenceValue | ||
| // ) { | ||
| // try { | ||
| // val data = dataStore.data.first().asMap() | ||
| // val key = data.keys.find { it.name == columnName } ?: return | ||
| // | ||
| // dataStore.edit { | ||
| // try { | ||
| // when (it[key]) { | ||
| // is String -> it[stringPreferencesKey(columnName)] = mapper?.let { | ||
| // it.toDatastore(value.stringValue!!) | ||
| // } ?: value.stringValue!! | ||
| // is Int -> it[intPreferencesKey(columnName)] = value.intValue!! | ||
| // is Boolean -> it[booleanPreferencesKey(columnName)] = value.booleanValue!! | ||
| // is Float -> it[floatPreferencesKey(columnName)] = value.floatValue!! | ||
| // is Long -> it[longPreferencesKey(columnName)] = value.longValue!! | ||
| // is Double -> it[doublePreferencesKey(columnName)] = | ||
| // value.floatValue!!.toDouble() | ||
| // } | ||
| // } catch (t: Throwable) { | ||
| // FloconLogger.logError("cannot update datastore preference", t) | ||
| // } | ||
| // } | ||
| // } catch (t: Throwable) { | ||
| // FloconLogger.logError(t.message ?: "cannot edit datastore preference columns", t) | ||
| // } | ||
| // } | ||
| // | ||
| // override suspend fun columns(): List<String> { | ||
| // return try { | ||
| // dataStore.data.first().asMap().map { it.key.name } | ||
| // } catch (t: Throwable) { | ||
| // FloconLogger.logError(t.message ?: "cannot get datastore preference columns", t) | ||
| // emptyList() | ||
| // } | ||
| // } | ||
| // | ||
| // override suspend fun get(columnName: String): io.github.openflocon.flocon.pluginsold.sharedprefs.model.FloconPreferenceValue? { | ||
| // return try { | ||
| // val data = dataStore.data.first().asMap() | ||
| // val key = data.keys.find { it.name == columnName } ?: return null | ||
| // val value = data[key] ?: return null | ||
| // | ||
| // return when (value) { | ||
| // is String -> FloconPreferenceValue( | ||
| // stringValue = mapper?.fromDatastore(value) ?: value | ||
| // ) | ||
| // is Int -> FloconPreferenceValue(intValue = value) | ||
| // is Float -> FloconPreferenceValue(floatValue = value) | ||
| // is Double -> FloconPreferenceValue(floatValue = value.toFloat()) | ||
| // is Boolean -> FloconPreferenceValue(booleanValue = value) | ||
| // is Long -> FloconPreferenceValue(longValue = value) | ||
| // else -> null | ||
| // } | ||
| // } catch (t: Throwable) { | ||
| // FloconLogger.logError(t.message ?: "cannot get datastore preference value", t) | ||
| // null | ||
| // } | ||
| // } | ||
| // | ||
| //} No newline at end of file |
Contributor
There was a problem hiding this comment.
The entire FloconDatastorePreference class has been commented out. This suggests the DataStore integration is incomplete. This functionality should be restored for the datastores module to be useful.
| scope = floconConfig.scope, | ||
| providers = pluginConfig.providers | ||
| ) | ||
| .also { FloconDatabasePluginImpl.plugin = it } |
Contributor
There was a problem hiding this comment.
The plugin instance is being stored in a static var on the companion object of FloconDatabasePluginImpl. This singleton-like pattern can introduce tight coupling, make testing more difficult, and potentially lead to state management issues in complex scenarios. Consider managing plugin instances within the core Flocon object and providing access through a registry or dependency injection mechanism rather than static properties. This would align better with the new modular and extensible architecture.
| body = DatabaseQueryLogModel( | ||
| dbName = dbName, | ||
| sqlQuery = sqlQuery, | ||
| bindArgs = bindArgs.map { it.toString() }, |
Contributor
There was a problem hiding this comment.
Converting all bindArgs to strings using it.toString() can lead to a loss of type information and ambiguity. For example, a null value will become the string "null", and numeric types will be indistinguishable from string types on the client side. This can make debugging queries more difficult. It would be more robust to serialize these arguments while preserving their original type, perhaps by converting them to a structure that includes both the value and the type, or by using a more descriptive serialization format.
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
1 participant
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.
Flocon SDK 2.0.0: The Modular Evolution ❄️
Welcome to the future of Android and Multiplatform inspection. Flocon 2.0.0 is not just an update; it's a complete architectural reimagining designed for performance, flexibility, and cross-platform excellence.
Lib
Task:
Desktop
Need to add other
🛠️ Simple & Extensible Initialization
Starting with 2.0.0, Flocon adopts a modern DSL-based initialization. Simply install the plugins you need, and you're ready to go.
🏗️ The New Modular Architecture
Flocon has evolved from a monolithic SDK to a modular ecosystem. This allows you to include only what you need, reducing your app's binary size and improving compile times.
graph TD subgraph "Core Discovery" C[flocon-core] end subgraph "Network Layer" N_CORE[network-core] OK[okhttp-interceptor] KT[ktor-interceptor] GRPC[grpc-interceptor] end subgraph "Database Layer" DB_CORE[database-core] RM[database-room] DS[datastores] end subgraph "Discovery & DeepLinks" DL[deeplinks] end C --> N_CORE C --> DB_CORE C --> DL N_CORE --> OK N_CORE --> KT N_CORE --> GRPC DB_CORE --> RM DB_CORE --> DSImportant
Production Safety first: Every functional module now has a corresponding
-no-opvariant. Use these in your release flavors to ensure no debugging code leaks into production.🌍 Kotlin Multiplatform (KMP) Ready
Version 2.0.0 marks our commitment to the multiplatform ecosystem.
Darwin,CIO, andOkHttpengines.📡 Modern Protocol Support: gRPC
We know modern apps rely on more than just REST. Flocon 2.0.0 introduces specialized interceptors for gRPC, completing our network inspection suite.
🛠️ Migration Path from 1.x to 2.0.0
Making the jump is straightforward but requires updating your dependency declarations to match the new modular structure.
Tip
Check the Mappings:
io.github.openflocon:floconnow serves as the entry point but relies on the underlying modules.:network:okhttp-interceptor).:database:roommodule if you need Room inspection.Flocon 2.0.0: Transparent. Modular. Powerful.