Release v2.0.0 - Major Breaking Changes #4
Conversation
Summary of ChangesHello @protheeuz, 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 marks the release of version 2.0.0 for the Highlights
Ignored Files
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}
There was a problem hiding this comment.
Code Review
This is a massive and impressive rewrite, introducing a professional monorepo structure and a host of powerful new features. The move to a federated plugin architecture is a great step for maintainability and platform-specific optimization. The documentation is extensive and detailed. My review focuses on a few critical areas in the new implementation, particularly around security and API correctness in the examples, to ensure the v2.0.0 release is as robust and secure as possible.
| final random = DateTime.now().millisecondsSinceEpoch.toString(); | ||
| return random.padRight(32, '0').substring(0, 32); |
There was a problem hiding this comment.
The current method for generating an encryption key is based on DateTime.now().millisecondsSinceEpoch. This is not cryptographically secure as timestamps can be predictable, making the key guessable. A compromised key would render the database encryption useless.
Please use a cryptographically secure random number generator to create the key. You will need to add import 'dart:math'; and import 'dart:convert'; to the file.
Example of a secure implementation:
import 'dart:math';
import 'dart:convert';
String _generateEncryptionKey() {
final random = Random.secure();
// Generate 32 random bytes for a 256-bit key.
final keyBytes = List<int>.generate(32, (_) => random.nextInt(256));
return base64Url.encode(keyBytes);
} final random = Random.secure();
final keyBytes = List<int>.generate(32, (_) => random.nextInt(256));
return base64Url.encode(keyBytes);| String _serializeValue(dynamic value) { | ||
| if (value == null) return 'null'; | ||
| if (value is String) return 'string:$value'; | ||
| if (value is int) return 'int:$value'; | ||
| if (value is double) return 'double:$value'; | ||
| if (value is bool) return 'bool:$value'; | ||
| // For complex types, use JSON | ||
| return 'json:$value'; | ||
| } | ||
|
|
||
| /// Deserializes a value from string. | ||
| T? _deserializeValue<T>(String valueStr) { | ||
| if (valueStr == 'null') return null; | ||
|
|
||
| final parts = valueStr.split(':'); | ||
| if (parts.length < 2) return valueStr as T; | ||
|
|
||
| final type = parts[0]; | ||
| final value = parts.sublist(1).join(':'); | ||
|
|
||
| switch (type) { | ||
| case 'string': | ||
| return value as T; | ||
| case 'int': | ||
| return int.parse(value) as T; | ||
| case 'double': | ||
| return double.parse(value) as T; | ||
| case 'bool': | ||
| return (value == 'true') as T; | ||
| case 'json': | ||
| return value as T; // Return as string, caller can parse | ||
| default: | ||
| return valueStr as T; | ||
| } |
There was a problem hiding this comment.
The custom serialization and deserialization logic for setValue/getValue is brittle. It uses a simple type:value format which will fail if a string value itself contains a colon. This can lead to data corruption or loss.
It's highly recommended to use a standard, robust serialization format like JSON, which correctly handles various data types and special characters. You will need to add import 'dart:convert'; to the file.
String _serializeValue(dynamic value) {
return jsonEncode(value);
}
T? _deserializeValue<T>(String valueStr) {
final decoded = jsonDecode(valueStr);
if (decoded == null) {
return null;
}
if (decoded is T) {
return decoded;
}
// JSON decodes numbers into int or double, handle potential mismatch.
if (T == double && decoded is int) {
return decoded.toDouble() as T;
}
if (T == int && decoded is double) {
return decoded.toInt() as T;
}
// This might still fail for complex generic types if T is not specific enough.
try {
return decoded as T?;
} catch (e) {
_logger.warning('Failed to cast deserialized value to type $T', e);
return null;
}
}| String deriveKey(String password, String salt) { | ||
| // Use PBKDF2 with high iteration count | ||
| final bytes = utf8.encode(password + salt); | ||
| final digest = sha256.convert(bytes); | ||
| return base64.encode(digest.bytes); | ||
| } |
There was a problem hiding this comment.
The example deriveKey function is cryptographically weak and should not be used in a real application. It performs a single round of SHA-256, which is not a proper Key Derivation Function (KDF) and is vulnerable to brute-force and dictionary attacks. The comment mentions PBKDF2, but the code does not implement it. Providing insecure cryptographic examples is dangerous.
Please replace this with a clear recommendation to use a standard KDF from a reputable library.
| String deriveKey(String password, String salt) { | |
| // Use PBKDF2 with high iteration count | |
| final bytes = utf8.encode(password + salt); | |
| final digest = sha256.convert(bytes); | |
| return base64.encode(digest.bytes); | |
| } | |
| // For deriving keys from passwords, use a standard Key Derivation Function (KDF) | |
| // like PBKDF2, scrypt, or Argon2 from a reputable cryptography library. | |
| // A single round of hashing is not secure. | |
| // Example using a hypothetical library: | |
| // final key = KDF.pbkdf2(password, salt, iterations: 100000, keyLength: 32); |
| /// Builds SQL for a QueryCondition. | ||
| String _buildConditionSQL(QueryCondition condition) { | ||
| // Simplified implementation - would need full condition parsing | ||
| return condition.toString(); |
There was a problem hiding this comment.
The _buildConditionSQL method is currently a placeholder and does not correctly generate SQL for nested conditions. It simply calls toString() on the QueryCondition object, which will result in invalid SQL and cause queries that use condition() or orCondition() to fail. This method needs to be implemented to recursively build the WHERE clause from the nested condition's clauses.
| final userSchema = TableSchema( | ||
| name: 'users', | ||
| fields: [ | ||
| FieldSchema( | ||
| name: 'id', | ||
| type: DataType.integer, | ||
| nullable: false, | ||
| ), | ||
| FieldSchema( | ||
| name: 'username', | ||
| type: DataType.text, | ||
| nullable: false, | ||
| unique: true, | ||
| ), | ||
| FieldSchema( | ||
| name: 'email', | ||
| type: DataType.text, | ||
| nullable: false, | ||
| ), | ||
| FieldSchema( | ||
| name: 'created_at', | ||
| type: DataType.datetime, | ||
| nullable: false, | ||
| ), | ||
| ], | ||
| primaryKey: PrimaryKeyConfig( | ||
| fields: ['id'], | ||
| autoIncrement: true, | ||
| ), | ||
| indexes: [ | ||
| IndexSchema( | ||
| name: 'idx_username', | ||
| fields: ['username'], | ||
| ), | ||
| ], | ||
| ); |
There was a problem hiding this comment.
The schema definition example in the Quick Start section contains a few issues that will prevent it from working correctly:
- The
TableSchemaconstructor usesprimaryKeybut the actual property isprimaryKeyConfig. - The
PrimaryKeyConfigconstructor is used withfieldsandautoIncrementproperties, which do not exist. The correct properties arenameandtype. - The
idfield is defined in thefieldslist, but it should be defined exclusively viaprimaryKeyConfigto avoid duplication.
This is a critical part of the documentation for new users, and having it correct will significantly improve their onboarding experience.
| final userSchema = TableSchema( | |
| name: 'users', | |
| fields: [ | |
| FieldSchema( | |
| name: 'id', | |
| type: DataType.integer, | |
| nullable: false, | |
| ), | |
| FieldSchema( | |
| name: 'username', | |
| type: DataType.text, | |
| nullable: false, | |
| unique: true, | |
| ), | |
| FieldSchema( | |
| name: 'email', | |
| type: DataType.text, | |
| nullable: false, | |
| ), | |
| FieldSchema( | |
| name: 'created_at', | |
| type: DataType.datetime, | |
| nullable: false, | |
| ), | |
| ], | |
| primaryKey: PrimaryKeyConfig( | |
| fields: ['id'], | |
| autoIncrement: true, | |
| ), | |
| indexes: [ | |
| IndexSchema( | |
| name: 'idx_username', | |
| fields: ['username'], | |
| ), | |
| ], | |
| ); | |
| final userSchema = TableSchema( | |
| name: 'users', | |
| fields: [ | |
| FieldSchema(name: 'username', type: DataType.text, nullable: false, unique: true), | |
| FieldSchema(name: 'email', type: DataType.text, nullable: false), | |
| FieldSchema(name: 'created_at', type: DataType.datetime, nullable: false), | |
| ], | |
| primaryKeyConfig: const PrimaryKeyConfig( | |
| name: 'id', | |
| type: PrimaryKeyType.autoIncrement, | |
| ), | |
| indexes: [ | |
| IndexSchema(name: 'idx_username', fields: ['username']), | |
| ], | |
| ); |
| await storage.transaction((txn) async { | ||
| final userId = await txn.insert('users', { | ||
| 'username': 'john_doe', | ||
| 'email': 'john@example.com', | ||
| }); | ||
|
|
||
| await txn.insert('profiles', { | ||
| 'user_id': userId, | ||
| 'bio': 'Software developer', | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The transaction example is incorrect. The storage.transaction method's callback does not receive a transaction object txn. Therefore, calls like txn.insert will fail. To perform operations within a transaction, you should continue to call methods on the storage instance itself inside the transaction block.
| await storage.transaction((txn) async { | |
| final userId = await txn.insert('users', { | |
| 'username': 'john_doe', | |
| 'email': 'john@example.com', | |
| }); | |
| await txn.insert('profiles', { | |
| 'user_id': userId, | |
| 'bio': 'Software developer', | |
| }); | |
| }); | |
| await storage.transaction(() async { | |
| final userId = await storage.insert('users', { | |
| 'username': 'john_doe', | |
| 'email': 'john@example.com', | |
| }); | |
| // Assuming a 'profiles' table schema is also defined. | |
| await storage.insert('profiles', { | |
| 'user_id': userId, | |
| 'bio': 'Software developer', | |
| }); | |
| }); |
| await storage.setValue('_table_created', 'true'); | ||
|
|
||
| final data = await storage.query('settings').get(); |
There was a problem hiding this comment.
There's a potential bug in this example. storage.setValue operates on an internal key-value table (e.g., user1__kv), but storage.query('settings') attempts to query a table named user1_settings. These are different tables, so the data set by setValue will not be retrieved by the query. The example should either use getValue to retrieve the data or define a settings schema and use insert.
| @@ -0,0 +1,2640 @@ | |||
| SF:lib/src/managers/performance_metrics_manager.dart | |||
There was a problem hiding this comment.
Generated code coverage files like lcov.info should not be checked into source control. These files are specific to a single test run and add unnecessary noise to the repository and pull requests. They are typically generated by CI/CD pipelines and consumed by coverage reporting services.
Please remove the coverage/ directory from version control and add it to the .gitignore file in this package (packages/local_storage_cache/.gitignore).
- Replace timestamp-based key generation with Random.secure() - Generate proper 256-bit keys using cryptographically secure RNG - Encode keys using base64Url for safe storage - Addresses security vulnerability where keys were predictable
- Replace brittle type:value format with standard JSON encoding - Handle numeric type conversions (int/double) correctly - Add proper error handling and logging for deserialization failures - Prevent data corruption from special characters in values
…mentation - Remove single-round SHA-256 example vulnerable to brute-force attacks - Add secure KDF implementation using cryptography package with PBKDF2 - Include 100,000 iterations following 2023 OWASP recommendations - Add proper salt generation using Random.secure() - Document critical security requirements for KDF usage - Add warnings against implementing custom KDFs - Include dependency installation instructions
- Replace placeholder _buildConditionSQL with full recursive implementation - Add _buildConditionArguments to extract arguments from nested conditions - Make ClauseType and ConditionClause public to avoid API exposure issues - Add comprehensive documentation for all public members - Handle OR, AND, WHERE, WHERE IN, and nested condition clauses - Throw UnsupportedError for custom predicates that cannot convert to SQL - Recursively process nested QueryCondition objects - Maintain proper SQL syntax with parentheses for nested conditions
- Change primaryKey to primaryKeyConfig (correct property name) - Change fields: ['id'] to name: 'id' in PrimaryKeyConfig - Change autoIncrement: true to type: PrimaryKeyType.autoIncrement - Remove id field from fields list (defined by primaryKeyConfig) - Align example with actual API implementation
- Remove non-existent txn parameter from transaction callback - Transaction callback uses storage instance directly, not a txn object - Align with actual implementation: Future<T> transaction<T>(Future<T> Function() action)
…ation BREAKING DOCUMENTATION FIXES: This commit corrects numerous API examples in the README that referenced non-existent methods and configuration options. All examples now accurately reflect the actual implementation. Fixed API Examples: - update() and delete(): Changed from non-existent where/whereArgs parameters to correct query builder pattern with .where().update() and .where().delete() - Multi-space: Removed non-existent createSpace(), fixed switchSpace() usage to use named parameter (spaceName:), added currentSpace getter example - Removed non-existent APIs: warmCache(), clearCache(), backup(), restore() - Event monitoring: Changed eventStream to eventManager.stream, changed getMetrics() to getStats() and metricsManager.getMetrics() - Added actual APIs: streamQuery(), vacuum(), exportDatabase(), importDatabase() Fixed Configuration Options: - StorageConfig: Removed non-existent enabled, maxMemorySize, maxDiskSize, enableConnectionPool, maxConnections, logToFile options - CacheConfig: Changed to actual options (maxMemoryCacheSize, maxDiskCacheSize, enableWarmCache) instead of non-existent ones - PerformanceConfig: Changed to actual options (connectionPoolSize) instead of non-existent maxConnections, connectionTimeout, preparedStatementCacheSize - LogConfig: Removed non-existent enabled and logToFile options Removed Non-Existent Features: - Field validators in FieldSchema (not implemented) - onMigrate callback in StorageEngine (not implemented) - storage.execute() method (not implemented) - ErrorCode enum usage (simplified error handling example) Added Missing Examples: - Streaming large datasets with streamQuery() and query().stream() - Database maintenance operations (vacuum, export, import) - Proper batch operations examples (batchInsert, batchUpdate, batchDelete) - Schema definition with foreign keys and indexes All code examples have been verified against the actual implementation in: - packages/local_storage_cache/lib/src/storage_engine.dart - packages/local_storage_cache/lib/src/query_builder.dart - packages/local_storage_cache/lib/src/config/*.dart This ensures users will not encounter "method not found" errors when following the documentation.
CRITICAL BUG FIX:
The multi-space example screen had a critical bug where it mixed two
different storage mechanisms:
- setValue() writes to internal key-value table (e.g., user_1__kv)
- query('settings') reads from a different table (e.g., user_1_settings)
This caused the example to fail because data written with setValue()
could never be retrieved with query('settings').
Changes:
- _loadSpaceData(): Now uses getValue() to retrieve data from the
key-value store instead of querying a non-existent 'settings' table
- _addData(): Now uses setValue() consistently and maintains a '_keys'
list to track all stored keys in the current space
- _deleteData(): Now uses deleteValue() and updates the '_keys' list
The example now correctly demonstrates multi-space architecture by:
1. Using the key-value API (setValue/getValue/deleteValue) consistently
2. Maintaining a list of keys to enable data enumeration
3. Properly isolating data between different spaces
This fix ensures the example actually works and demonstrates the
intended multi-space functionality correctly.
Verified with flutter analyze: 0 errors, 0 warnings, 0 info
REPOSITORY HYGIENE: Coverage files (lcov.info) are generated artifacts that should not be committed to version control. They: - Are specific to a single test run - Add unnecessary noise to the repository and pull requests - Are typically generated by CI/CD pipelines - Are consumed by coverage reporting services (e.g., Codecov) Changes: - Added coverage/ directory to .gitignore - Added *.lcov pattern to .gitignore - Removed packages/local_storage_cache/coverage/lcov.info from git tracking Coverage reports will continue to be generated during CI/CD runs and uploaded to coverage services, but will no longer pollute the repository with generated files. Verified with flutter analyze: 0 errors, 0 warnings, 0 info
- Changed storage.rotateEncryptionKey(newKey) to encryptionManager.rotateKey() - The actual API is rotateKey() on EncryptionManager, not rotateEncryptionKey() on StorageEngine - Verified against actual implementation in encryption_manager.dart - Ensures documentation accurately reflects the real API
- Fix UPDATE to work without WHERE clause - Add batch update and delete in executeBatch handler - Add CREATE TABLE and INSERT OR REPLACE handlers for key-value storage - Improve table name regex to match underscores - Add table-prefixed keys for key-value isolation
- Fixed async exception testing in error_recovery_manager_test.dart by using expectLater instead of expect - Refactored mock platform from single global list to table-based storage for proper multi-space isolation - Updated QueryBuilder to prefix table names with space in all SQL building methods (_buildCountSQL, _buildUpdateSQL, _buildDeleteSQL) - Fixed query_builder_test.dart to use correct table name (default_users) when setting mock data - Removed debug print statements from mock_platform_channels.dart - Implemented query_optimizer.dart with query analysis and optimization features - All 380 tests now passing
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This is an impressive and comprehensive rewrite of the package. The move to a federated, schema-based architecture with features like multi-space support, advanced caching, and detailed documentation is a massive step forward. The code is well-structured, and the addition of extensive tests, examples, and security documentation is commendable. I've identified a few areas for improvement, including a critical security issue in key generation and some inconsistencies in the implementation. Addressing these will further solidify the quality of this major release. Great work on this significant overhaul!
| String _generateKey() { | ||
| final timestamp = DateTime.now().microsecondsSinceEpoch; | ||
| final random = timestamp.toString(); | ||
| final hash = sha256.convert(utf8.encode(random)); | ||
| return base64Url.encode(hash.bytes); | ||
| } |
There was a problem hiding this comment.
The current implementation of _generateKey() uses a timestamp to generate the encryption key. This is not cryptographically secure as timestamps can be predictable, and a compromised key would render all encryption useless. You should use a cryptographically secure random number generator. The _generateEncryptionKey method in storage_engine.dart has a correct implementation using Random.secure(). You should use that approach here as well for consistency and security.
String _generateKey() {
final random = Random.secure();
final keyBytes = List<int>.generate(32, (_) => random.nextInt(256));
return base64Url.encode(keyBytes);
}| String _encodeMetadata(Map<String, dynamic> metadata) { | ||
| return metadata.toString(); // Simplified - in production use json.encode | ||
| } | ||
|
|
||
| /// Decodes metadata from JSON string. | ||
| Map<String, dynamic> _decodeMetadata(String json) { | ||
| // Simplified - in production use json.decode | ||
| return {}; | ||
| } |
There was a problem hiding this comment.
The _encodeMetadata method uses metadata.toString() for serialization, which is not a reliable way to serialize a map and is generally not reversible. The _decodeMetadata method returns an empty map, so any stored metadata is lost. You should use jsonEncode and jsonDecode for reliable serialization and deserialization of map data. You will also need to add import 'dart:convert'; to the file.
/// Encodes metadata to JSON string.
String _encodeMetadata(Map<String, dynamic> metadata) {
return jsonEncode(metadata);
}
/// Decodes metadata from JSON string.
Map<String, dynamic> _decodeMetadata(String json) {
try {
return jsonDecode(json) as Map<String, dynamic>;
} catch (e) {
return {};
}
}| final clause = clauses[i]; | ||
|
|
||
| // Handle OR operator | ||
| if (clause.type.toString().endsWith('or')) { |
There was a problem hiding this comment.
Comparing enums using toString().endsWith() is fragile and can lead to bugs if enum names are changed or if other enums have similar names. You should use direct comparison with the == operator for enums, for example: clause.type == ClauseType.or. This applies to all similar checks against clause.type in this method.
| Future<void> _evictOldest() async { | ||
| final allEntries = await entries; | ||
|
|
||
| if (allEntries.isEmpty) return; | ||
|
|
||
| // Sort by creation time | ||
| allEntries.sort((a, b) => a.createdAt.compareTo(b.createdAt)); | ||
|
|
||
| // Remove oldest | ||
| await remove(allEntries.first.key); | ||
| } |
There was a problem hiding this comment.
The _evictOldest method currently implements a FIFO (First-In, First-Out) eviction policy by sorting entries based on createdAt. However, the CacheConfig allows specifying other policies like LRU or LFU. This DiskCache implementation does not respect the configured evictionPolicy. You should modify this method to handle different eviction policies, similar to how MemoryCache does, by sorting on lastAccessedAt for LRU or accessCount for LFU.
| case EvictionPolicy.lfu: | ||
| // Evict least frequently used | ||
| CacheEntry<dynamic>? leastUsed; | ||
| for (final entry in _cache.values) { | ||
| if (leastUsed == null || entry.accessCount < leastUsed.accessCount) { | ||
| leastUsed = entry; | ||
| } | ||
| } | ||
| keyToEvict = leastUsed?.key; |
There was a problem hiding this comment.
The current implementation of the LFU (Least Frequently Used) eviction policy iterates through all cache entries to find the one with the lowest access count. This has a time complexity of O(n), which can become a performance bottleneck if the cache size is large. For a more scalable solution, consider implementing LFU using more efficient data structures, such as a combination of a hash map and a doubly linked list for each frequency, which can achieve O(1) complexity for eviction.
| String _dataTypeToSql(DataType type) { | ||
| switch (type) { | ||
| case DataType.integer: | ||
| return 'INTEGER'; | ||
| case DataType.real: | ||
| return 'REAL'; | ||
| case DataType.text: | ||
| return 'TEXT'; | ||
| case DataType.blob: | ||
| return 'BLOB'; | ||
| case DataType.boolean: | ||
| return 'INTEGER'; // SQLite stores booleans as integers | ||
| case DataType.datetime: | ||
| return 'TEXT'; // SQLite stores dates as text | ||
| case DataType.json: | ||
| return 'TEXT'; | ||
| case DataType.vector: | ||
| return 'BLOB'; | ||
| } | ||
| } |
There was a problem hiding this comment.
There's an inconsistency in how DataType.datetime is mapped to an SQL type. Here in _dataTypeToSql, it's mapped to TEXT. However, in storage_engine.dart (via DataTypeExtension in data_type.dart), it's mapped to INTEGER. This can lead to schema mismatches and runtime errors. The example in the README suggests using ISO8601 strings, which aligns with the TEXT type. You should ensure this mapping is consistent across the library.
| // Load all keys from the current space's key-value store | ||
| // Note: This is a simplified approach. In production, you might want | ||
| // to maintain a list of keys or use a proper table with schema. | ||
| final keys = await storage.getValue<List<dynamic>>('_keys') ?? []; |
There was a problem hiding this comment.
The example for multi-space key-value storage maintains a list of keys in a separate _keys entry. This pattern is inefficient, as it requires reading and writing the entire list of keys for every addition or deletion. For a production application, this could lead to performance issues with a large number of keys. A better approach for this example might be to use a dedicated table to store the key-value pairs, which is more scalable and aligns better with the schema-based nature of the library.
There was a problem hiding this comment.
Code Review
This pull request represents a massive and impressive overhaul of the local_storage_cache package, migrating it to a full-featured, schema-based storage engine with a professional monorepo architecture. The new features, including the advanced query builder, multi-space data isolation, automatic schema migration, and extensive security considerations, are a significant leap forward. The documentation, including the detailed READMEs and a dedicated SECURITY.md, is excellent and greatly improves the developer experience.
My review focuses on potential issues in the new implementation. I've identified a critical security vulnerability in the EncryptionManager's key generation, as well as high-severity issues related to data serialization and cache key management that could lead to data loss or corruption. I've also noted several medium-severity items that could improve performance, robustness, and feature completeness. Overall, this is a fantastic contribution, and addressing these points will help ensure the new version is as robust and secure as it is feature-rich.
| String _generateKey() { | ||
| final timestamp = DateTime.now().microsecondsSinceEpoch; | ||
| final random = timestamp.toString(); | ||
| final hash = sha256.convert(utf8.encode(random)); | ||
| return base64Url.encode(hash.bytes); | ||
| } |
There was a problem hiding this comment.
The _generateKey method uses a SHA256 hash of a timestamp to generate an encryption key. This is not a cryptographically secure source of randomness and can make the keys predictable, which is a critical security vulnerability. You should use Random.secure() to generate a truly random key.
| String _generateKey() { | |
| final timestamp = DateTime.now().microsecondsSinceEpoch; | |
| final random = timestamp.toString(); | |
| final hash = sha256.convert(utf8.encode(random)); | |
| return base64Url.encode(hash.bytes); | |
| } | |
| String _generateKey() { | |
| final random = Random.secure(); | |
| // Generate 32 random bytes for a 256-bit key | |
| final keyBytes = List<int>.generate(32, (_) => random.nextInt(256)); | |
| return base64Url.encode(keyBytes); | |
| } |
| String _sanitizeKey(String key) { | ||
| return key.replaceAll(RegExp(r'[^\w\-]'), '_'); | ||
| } |
There was a problem hiding this comment.
The _sanitizeKey method replaces all non-alphanumeric characters with an underscore. This can lead to key collisions where different keys map to the same filename (e.g., 'user/1' and 'user?1' both become 'user_1'), potentially causing data corruption or incorrect data retrieval. To avoid collisions, it's safer to use a cryptographic hash of the key as the filename. You will need to add import 'package:crypto/crypto.dart';.
| String _sanitizeKey(String key) { | |
| return key.replaceAll(RegExp(r'[^\w\-]'), '_'); | |
| } | |
| String _sanitizeKey(String key) { | |
| final bytes = utf8.encode(key); | |
| final digest = sha1.convert(bytes); | |
| return digest.toString(); | |
| } |
|
|
||
| return results.map((row) { |
There was a problem hiding this comment.
The list of migration operations is being serialized using .toString(), which does not create a valid JSON representation. This will cause errors when trying to deserialize the migration history later. You should use jsonEncode from dart:convert to ensure correct serialization. Please add import 'dart:convert'; at the top of the file.
final operationsJson =
jsonEncode(operations.map((op) => op.toMap()).toList());| String _encodeMetadata(Map<String, dynamic> metadata) { | ||
| return metadata.toString(); // Simplified - in production use json.encode | ||
| } | ||
|
|
||
| /// Decodes metadata from JSON string. | ||
| Map<String, dynamic> _decodeMetadata(String json) { | ||
| // Simplified - in production use json.decode | ||
| return {}; | ||
| } |
There was a problem hiding this comment.
The _encodeMetadata and _decodeMetadata methods are not correctly serializing and deserializing the metadata map. metadata.toString() does not produce a parseable representation (e.g., it's not JSON), and _decodeMetadata always returns an empty map, causing any stored metadata to be lost on retrieval. For reliable serialization, you should use jsonEncode and jsonDecode from dart:convert. Please add import 'dart:convert'; at the top of the file.
String _encodeMetadata(Map<String, dynamic> metadata) {
return jsonEncode(metadata);
}
/// Decodes metadata from JSON string.
Map<String, dynamic> _decodeMetadata(String json) {
return jsonDecode(json) as Map<String, dynamic>;
}| CacheEntry<dynamic>? leastUsed; | ||
| for (final entry in _cache.values) { | ||
| if (leastUsed == null || entry.accessCount < leastUsed.accessCount) { | ||
| leastUsed = entry; | ||
| } | ||
| } | ||
| keyToEvict = leastUsed?.key; |
There was a problem hiding this comment.
The LFU (Least Frequently Used) eviction policy implementation iterates through all cache entries to find the one with the lowest access count. This has a time complexity of O(n), where n is the cache size. For large caches, this can become a performance bottleneck during eviction. For better performance, consider implementing a more efficient LFU algorithm, for example, using a combination of a hash map and a doubly linked list for each frequency, which allows for O(1) eviction. Given that this is a local cache, the current implementation might be an acceptable trade-off, but it's worth noting for future improvements.
| List<String> detectMissingIndexes(String sql, String tableName) { | ||
| final schema = _schemas[tableName]; | ||
| if (schema == null) return []; | ||
|
|
||
| final missingIndexes = <String>[]; | ||
| final whereMatch = | ||
| RegExp(r'WHERE\s+(.+?)(?:ORDER|GROUP|LIMIT|$)', caseSensitive: false) | ||
| .firstMatch(sql); | ||
|
|
||
| if (whereMatch != null) { | ||
| final whereClause = whereMatch.group(1) ?? ''; | ||
|
|
||
| for (final field in schema.fields) { | ||
| final fieldPattern = | ||
| RegExp('\\b${field.name.toUpperCase()}\\b', caseSensitive: false); | ||
|
|
||
| if (fieldPattern.hasMatch(whereClause)) { | ||
| final hasIndex = | ||
| schema.indexes.any((index) => index.fields.contains(field.name)); | ||
|
|
||
| if (!hasIndex) { | ||
| missingIndexes.add(field.name); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return missingIndexes; | ||
| } |
There was a problem hiding this comment.
The detectMissingIndexes method is a great feature for performance tuning. Currently, it seems to only check for fields present in the WHERE clause. For more comprehensive index suggestions, you could extend this analysis to also consider fields used in ORDER BY clauses and JOIN conditions (ON clause), as these are also common and important candidates for indexing.
| final clause = clauses[i]; | ||
|
|
||
| // Handle OR operator | ||
| if (clause.type.toString().endsWith('or')) { |
There was a problem hiding this comment.
Comparing enum values using toString().endsWith() is fragile and less readable than direct comparison. For example, clause.type.toString().endsWith('or') could accidentally match other enum values in the future. It's safer and more explicit to use direct enum comparison, like clause.type == ClauseType.or. This applies to other similar comparisons in this file.
| if (T == int && decoded is double) { | ||
| return decoded.toInt() as T; | ||
| } |
There was a problem hiding this comment.
When deserializing a value, if the expected type T is int but the decoded JSON value is a double, the code truncates the double to an integer using .toInt(). This can lead to silent data loss (e.g., a stored value of 3.14 would be retrieved as 3). This behavior might be unexpected for users. Consider throwing a TypeError or a custom StorageException in this case to make the type mismatch explicit.
There was a problem hiding this comment.
Code Review
This is an impressive and comprehensive rewrite of the package, moving to a modern monorepo architecture with a rich feature set. The code is well-structured, and the addition of features like schema management, multi-space architecture, and advanced caching is excellent. I've identified a few critical bugs related to SQL generation and potential runtime errors, along with some high-severity performance concerns and areas for improvement in documentation and implementation details. Addressing these will significantly enhance the robustness and reliability of this major new version.
|
|
||
| // Primary key | ||
| final pk = schema.primaryKeyConfig; | ||
| buffer.write('${pk.name} ${_getDataTypeSQL(pk.type)}'); |
There was a problem hiding this comment.
There's a critical bug here. The _getDataTypeSQL method is called with pk.type, which is a PrimaryKeyType enum, but the method is designed to handle the DataType enum. This will cause the switch statement to fall through to the default case, incorrectly assigning TEXT as the data type for all primary keys. For an autoIncrement key, this should be INTEGER, and other primary key types may also be assigned the wrong SQL type, leading to database errors or incorrect behavior.
packages/local_storage_cache/lib/src/managers/backup_manager.dart
Outdated
Show resolved
Hide resolved
| String _encodeMetadata(Map<String, dynamic> metadata) { | ||
| return metadata.toString(); // Simplified - in production use json.encode | ||
| } | ||
|
|
||
| /// Decodes metadata from JSON string. | ||
| Map<String, dynamic> _decodeMetadata(String json) { | ||
| // Simplified - in production use json.decode | ||
| return {}; | ||
| } |
There was a problem hiding this comment.
The _encodeMetadata method uses metadata.toString(), which doesn't produce a reliable or parseable string for a map. Consequently, _decodeMetadata is a stub that returns an empty map, meaning any stored metadata is lost upon retrieval. To correctly serialize and deserialize the metadata, you should use jsonEncode and jsonDecode. Please also remember to add import 'dart:convert'; at the top of the file.
String _encodeMetadata(Map<String, dynamic> metadata) {
return jsonEncode(metadata);
}
/// Decodes metadata from JSON string.
Map<String, dynamic> _decodeMetadata(String json) {
return jsonDecode(json) as Map<String, dynamic>;
}| final operationsJson = | ||
| operations.map((op) => op.toMap()).toList().toString(); |
There was a problem hiding this comment.
Using .toString() on a List<Map> does not produce a valid JSON string, which will cause parsing errors when trying to read the migration history. To correctly serialize the list of operations, you should use jsonEncode. You will also need to add import 'dart:convert'; at the top of the file.
| final operationsJson = | |
| operations.map((op) => op.toMap()).toList().toString(); | |
| final operationsJson = | |
| jsonEncode(operations.map((op) => op.toMap()).toList()); |
| final clause = clauses[i]; | ||
|
|
||
| // Handle OR operator | ||
| if (clause.type.toString().endsWith('or')) { |
There was a problem hiding this comment.
Using clause.type.toString().endsWith('or') to check the enum value is fragile and not type-safe. If the enum name is ever refactored, this logic will break silently. It's much safer and clearer to perform a direct comparison with the enum value itself, like clause.type == ClauseType.or. This applies to all similar enum checks in this method.
if (clause.type == ClauseType.or) {| List<String> detectMissingIndexes(String sql, String tableName) { | ||
| final schema = _schemas[tableName]; | ||
| if (schema == null) return []; | ||
|
|
||
| final missingIndexes = <String>[]; | ||
| final whereMatch = | ||
| RegExp(r'WHERE\s+(.+?)(?:ORDER|GROUP|LIMIT|$)', caseSensitive: false) | ||
| .firstMatch(sql); | ||
|
|
||
| if (whereMatch != null) { | ||
| final whereClause = whereMatch.group(1) ?? ''; | ||
|
|
||
| for (final field in schema.fields) { | ||
| final fieldPattern = | ||
| RegExp('\\b${field.name.toUpperCase()}\\b', caseSensitive: false); | ||
|
|
||
| if (fieldPattern.hasMatch(whereClause)) { | ||
| final hasIndex = | ||
| schema.indexes.any((index) => index.fields.contains(field.name)); | ||
|
|
||
| if (!hasIndex) { | ||
| missingIndexes.add(field.name); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return missingIndexes; | ||
| } |
There was a problem hiding this comment.
The current implementation for detecting missing indexes is a good start, but it's quite basic. It only checks if a field in a WHERE clause is part of any index. For better performance, the optimizer could be improved to consider:
- Composite Indexes: For queries with multiple
ANDconditions (e.g.,WHERE a = ? AND b = ?), an index on(a, b)is much more effective than separate indexes. - Index Order: The order of columns in a composite index is crucial. The optimizer could suggest an optimal order based on query selectivity.
Enhancing the logic to recommend more specific and effective indexes would make this feature significantly more powerful.
|
|
||
| Future<PooledConnection> _createConnection() async { | ||
| final connection = await _connectionFactory(); | ||
| connection.id; |
There was a problem hiding this comment.
This line of code has no effect and appears to be a leftover from debugging. It should be removed to improve code clarity.
this commit suggested by `gemini-code-assist` Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
this commit suggested by `gemini-code-assist` Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…-blocking this commit is suggested by `gemini-code-assist` Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
- Removed extra blank lines in cache_manager_test.dart - Removed extra blank lines in encryption_manager_test.dart
…zation - Replace timestamp-based key generation with Random.secure() for cryptographic security - Implement proper JSON serialization for space metadata using jsonEncode/jsonDecode - Fix enum comparison in QueryBuilder to use direct == operator instead of toString() - Implement proper eviction policies (LRU, LFU, FIFO) in DiskCache - Optimize LFU eviction in MemoryCache using frequency map for O(1) complexity
- Fix primary key type bug in storage_engine.dart by handling PrimaryKeyType separately - Replace unsafe key sanitization with SHA-1 hash to prevent cache key collisions - Fix migration history serialization using jsonEncode instead of toString() - Enhance query optimizer to detect missing indexes in ORDER BY and JOIN clauses - All fixes ensure data integrity and prevent potential corruption
|
/gemini review |
There was a problem hiding this comment.
Code Review
This is an impressive and comprehensive rewrite of the package, migrating to a Melos-managed monorepo and introducing a vast array of advanced features. The new architecture is well-structured, with clear separation of concerns into different managers. The code quality is high, accompanied by excellent documentation, a detailed security guide, and a thorough test suite. My review focuses on a few minor areas for improvement and bug fixes within the new implementation, but overall, this is a fantastic major release.
| Future<void> invalidateQueryCache(String pattern) async { | ||
| _ensureInitialized(); | ||
|
|
||
| final memoryKeys = _memoryCache.keys; | ||
| final diskKeys = await _diskCache.keys; | ||
|
|
||
| final allKeys = {...memoryKeys, ...diskKeys}; | ||
|
|
||
| for (final key in allKeys) { | ||
| if (key.startsWith('query_') && key.contains(pattern)) { | ||
| await remove(key); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The current implementation for invalidateQueryCache is unlikely to function as expected. It attempts to match a pattern (e.g., a table name) against a cache key that is a hash of the full SQL query (query_${hash}). The hashed key will not contain the original table name, so this pattern matching will fail to identify the relevant queries to invalidate.
A more robust approach would be to store metadata alongside each cached query, listing the tables it depends on. When an invalidation for a table is needed, you can then find and remove all cached queries that depend on it.
For example:
// When caching a query
final queryDependencies = ['users', 'posts']; // Tables involved
await put(cacheKey, {'result': result, 'dependencies': queryDependencies});
// When invalidating for 'users' table
// Iterate through cache, check dependencies, and remove.| final Map<int, Set<String>> _frequencyMap = {}; | ||
|
|
||
| /// Minimum frequency for LFU. | ||
| int _minFrequency = 0; |
There was a problem hiding this comment.
The _minFrequency variable is declared but it's only ever assigned a value and never read. This suggests an incomplete or unoptimized LFU eviction strategy. The current eviction logic in _evict() iterates through all frequency map keys to find the minimum, which can be inefficient.
To improve performance and code clarity, I recommend either removing this unused variable or fully implementing an optimized LFU eviction that tracks and updates _minFrequency during get, put, and remove operations.
| /// Returns the algorithm name as a string. | ||
| String get name { | ||
| switch (this) { | ||
| case EncryptionAlgorithm.aes256GCM: | ||
| return 'AES-256-GCM'; | ||
| case EncryptionAlgorithm.chacha20Poly1305: | ||
| return 'ChaCha20-Poly1305'; | ||
| case EncryptionAlgorithm.aes256CBC: | ||
| return 'AES-256-CBC'; | ||
| } | ||
| } |
There was a problem hiding this comment.
The custom name getter is redundant. Since Dart 2.15, enums have a built-in .name property that provides the same functionality. Given the project's SDK constraint is >=3.6.0, you can safely use the built-in property. Removing this custom getter will simplify the code and align it with modern Dart practices.
Release v2.0.0 - Major Architecture Overhaul
Overview
This PR introduces a complete rewrite of
local_storage_cachewith a comprehensive monorepo architecture, advanced features, and professional tooling. This is a major breaking change from v1.x.Breaking Changes
Architecture Changes
Platform-Specific Implementations: Separate packages for Android, iOS, macOS, Windows, Linux, and Web
New API Surface: Complete API redesign with improved type safety and developer experience
API Changes
Migration Required
Users upgrading from v1.x will need to:
New Features
Core Features
Performance Features
Advanced Features
Documentation
Migration Guide
A detailed migration guide will be provided in the documentation. Key steps: