Release v2.0.0 - Major Breaking Changes#3
Conversation
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
|
/gemini review |
![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 an impressive and comprehensive rewrite for v2.0.0, introducing a professional monorepo architecture and a rich feature set. The documentation, including the README and SECURITY guides, is exceptionally detailed and well-structured. My review focuses on several critical and high-severity issues related to security, correctness, and performance that should be addressed before release. These include insecure key generation, non-deterministic hashing, incomplete migration logic, and performance bottlenecks in the caching layer. I've also noted some opportunities for improvement in the backup/restore logic and the query builder.
| 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 key generation method uses DateTime.now().microsecondsSinceEpoch, which is not a source of cryptographic randomness. Keys generated this way are predictable and can compromise the security of the entire encryption system. A cryptographically secure random number generator should be used instead.
import 'dart:math';
// ...
String _generateKey() {
final random = Random.secure();
final values = List<int>.generate(32, (i) => random.nextInt(256));
return base64Url.encode(values);
}| String _generateEncryptionKey() { | ||
| // Generate a 32-byte (256-bit) key | ||
| final random = DateTime.now().millisecondsSinceEpoch.toString(); | ||
| return random.padRight(32, '0').substring(0, 32); |
There was a problem hiding this comment.
The encryption key generation is based on DateTime.now(), which is predictable and not cryptographically secure. This can lead to compromised encryption. It's crucial to use a source of true randomness for generating security keys.
import 'dart:math';
// ...
String _generateEncryptionKey() {
// Generate a 32-byte (256-bit) key
final random = Random.secure();
final values = List<int>.generate(32, (i) => random.nextInt(256));
return base64Url.encode(values);
}|
|
||
| // Check for table rename |
There was a problem hiding this comment.
The schema hash is calculated using schemaMap.toString().hashCode. The toString() representation of a Map does not guarantee a consistent key order across different Dart versions or runs. This can lead to non-deterministic hashes for the same schema, potentially causing unnecessary or incorrect migration triggers. To create a deterministic hash, the map should be converted to a canonical string representation, for example by using json.encode after sorting the map keys recursively.
| // SQLite doesn't support DROP COLUMN directly | ||
| // Need to use table recreation strategy | ||
| operations.add( | ||
| MigrationOperation.customSql( | ||
| sql: | ||
| '-- Field ${change.fieldName} removed from ${change.tableName} (requires table recreation)', | ||
| description: | ||
| 'Remove field ${change.fieldName} from ${change.tableName}', | ||
| ), | ||
| ); | ||
|
|
||
| case SchemaChangeType.fieldTypeChanged: | ||
| case SchemaChangeType.fieldConstraintChanged: | ||
| // SQLite doesn't support ALTER COLUMN | ||
| // Need to use table recreation strategy | ||
| operations.add( | ||
| MigrationOperation.customSql( | ||
| sql: | ||
| '-- Field ${change.fieldName} modified in ${change.tableName} (requires table recreation)', | ||
| description: | ||
| 'Modify field ${change.fieldName} in ${change.tableName}', | ||
| ), | ||
| ); |
There was a problem hiding this comment.
The migration generation for fieldRemoved, fieldTypeChanged, and fieldConstraintChanged is incomplete. It only adds a SQL comment indicating that table recreation is needed. For the feature "Automatic schema migration" to be fully realized, the system should handle this by implementing the shadow table migration pattern (create new table, copy data, drop old table, rename new table). The method migrateWithZeroDowntime seems to implement this pattern, and it should be integrated here to automate these complex migrations.
| String _buildConditionSQL(QueryCondition condition) { | ||
| // Simplified implementation - would need full condition parsing | ||
| return condition.toString(); | ||
| } |
There was a problem hiding this comment.
The _buildConditionSQL method, which is supposed to handle nested query conditions, currently just returns condition.toString(). This is a placeholder and will not generate the correct SQL for nested WHERE clauses, breaking the functionality for complex queries. This method needs to be implemented to recursively build the SQL string for the nested QueryCondition.
| # Android Studio will place build artifacts here | ||
| /android/app/debug | ||
| /android/app/profile | ||
| /android/app/release |
There was a problem hiding this comment.
The test coverage report directory (coverage/) is not ignored. Generated files like this should not be committed to version control as they can cause merge conflicts and unnecessarily increase repository size.
/android/app/release
/coverage/
| .flutter-plugins-dependencies | ||
|
|
||
| .kiro/ | ||
| AGENTS.md No newline at end of file |
There was a problem hiding this comment.
The pubspec.lock file for the example app is included in version control. For library packages, it's a best practice to not commit the pubspec.lock file (even for examples) to avoid forcing specific dependency versions on consumers and contributors.
AGENTS.md
**/pubspec.lock
| 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 implements a First-In, First-Out (FIFO) eviction policy by sorting entries based on createdAt. However, the CacheConfig allows specifying other policies like LRU or LFU. The DiskCache should respect the configured evictionPolicy instead of having a hardcoded behavior. The method should be updated to handle different eviction strategies, for example by sorting on lastAccessedAt for LRU or accessCount for LFU.
| Future<void> _checkExpirations() async { | ||
| if (!_initialized) return; | ||
|
|
||
| // Check memory cache | ||
| for (final entry in _memoryCache.entries) { | ||
| if (entry.isExpired) { | ||
| _expirationController.add( | ||
| CacheExpirationEvent( | ||
| key: entry.key, | ||
| expiredAt: DateTime.now(), | ||
| ), | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| // Check disk cache | ||
| for (final entry in await _diskCache.entries) { | ||
| if (entry.isExpired) { | ||
| _expirationController.add( | ||
| CacheExpirationEvent( | ||
| key: entry.key, | ||
| expiredAt: DateTime.now(), | ||
| ), | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| // Clear expired entries | ||
| await clearExpired(); | ||
| } |
There was a problem hiding this comment.
The _checkExpirations method iterates through all cache entries to find expired items and emit events. Immediately after, clearExpired is called, which iterates through all entries again to remove them. This double iteration can be inefficient, especially for large caches. The logic could be combined into a single pass to both identify expired items for event emission and remove them.
| 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 use placeholder implementations (toString() and returning an empty map). The comment indicates that json.encode/decode should be used. This should be fixed to correctly serialize and deserialize space metadata to and from JSON.
String _encodeMetadata(Map<String, dynamic> metadata) {
return json.encode(metadata);
}
/// Decodes metadata from JSON string.
Map<String, dynamic> _decodeMetadata(String json) {
return json.decode(json) as Map<String, dynamic>;
}1 participant
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: