feat(drift): add talker_drift_logger package and integrate Drift logg…#426
Open
becandier wants to merge 3 commits intoFrezyx:masterfrom
Open
feat(drift): add talker_drift_logger package and integrate Drift logg…#426becandier wants to merge 3 commits intoFrezyx:masterfrom
becandier wants to merge 3 commits intoFrezyx:masterfrom
Conversation
…ing\n\n- Core: add Drift keys to talker (talker_key.dart), default titles/colors (settings.dart)\n- UI: add TalkerScreen colors for Drift keys (talker_flutter)\n- Package: talker_drift_logger with interceptor, settings, logs; tests; example; README\n- CI: add workflow, align with repo; remove min_coverage override to match other packages\n- Repo docs: update root README with package row and section\n\nBREAKING CHANGE: none
Contributor
Reviewer's GuideThis PR introduces a new Sequence diagram for logging a Drift SELECT query with TalkerDriftLoggersequenceDiagram
participant App
participant Drift
participant TalkerDriftLogger
participant Talker
App->>Drift: runSelect(statement, args)
Drift->>TalkerDriftLogger: runSelect(statement, args)
TalkerDriftLogger->>Talker: logCustom(DriftQueryLog)
TalkerDriftLogger->>Drift: executor.runSelect(statement, args)
Drift-->>TalkerDriftLogger: rows
TalkerDriftLogger->>Talker: logCustom(DriftResultLog)
TalkerDriftLogger-->>Drift: rows
Drift-->>App: rows
Sequence diagram for logging a Drift transaction (begin/commit/rollback)sequenceDiagram
participant App
participant Drift
participant TalkerDriftLogger
participant Talker
App->>Drift: beginTransaction()
Drift->>TalkerDriftLogger: beginTransaction(parent)
TalkerDriftLogger->>Talker: logCustom(DriftTransactionLog)
TalkerDriftLogger-->>Drift: TransactionExecutor
App->>Drift: commitTransaction(inner)
Drift->>TalkerDriftLogger: commitTransaction(inner)
TalkerDriftLogger->>Talker: logCustom(DriftTransactionLog)
TalkerDriftLogger-->>Drift: (commit complete)
App->>Drift: rollbackTransaction(inner)
Drift->>TalkerDriftLogger: rollbackTransaction(inner)
TalkerDriftLogger->>Talker: logCustom(DriftTransactionLog)
TalkerDriftLogger-->>Drift: (rollback complete)
Class diagram for TalkerDriftLogger and related log classesclassDiagram
class TalkerDriftLogger {
- Talker _talker
+ TalkerDriftLoggerSettings settings
+ Future<T> _run<T>(...)
+ bool _accept(...)
+ bool _acceptError(...)
+ TransactionExecutor beginTransaction(...)
+ Future<void> commitTransaction(...)
+ Future<void> rollbackTransaction(...)
+ Future<void> runBatched(...)
+ Future<int> runInsert(...)
+ Future<int> runUpdate(...)
+ Future<int> runDelete(...)
+ Future<void> runCustom(...)
+ Future<List<Map<String, Object?>>> runSelect(...)
}
TalkerDriftLogger --|> QueryInterceptor
TalkerDriftLogger --> TalkerDriftLoggerSettings
TalkerDriftLogger --> Talker
class TalkerDriftLoggerSettings {
+ bool enabled
+ LogLevel logLevel
+ bool printArgs
+ bool printResults
+ int? resultRowLimit
+ int? resultMaxChars
+ bool printTransaction
+ bool printBatch
+ Set<String> obfuscateColumns
+ Set<Pattern> obfuscatePatterns
+ AnsiPen? queryPen
+ AnsiPen? resultPen
+ AnsiPen? errorPen
+ AnsiPen? transactionPen
+ AnsiPen? batchPen
+ bool Function(String, List<Object?>)? statementFilter
+ bool Function(Object)? errorFilter
+ String Function(List<Map<String, Object?>>)? resultPrinter
+ String Function(List<Object?>)? argsPrinter
+ copyWith(...)
}
class DriftQueryLog {
+ List<Object?> args
+ TalkerDriftLoggerSettings settings
+ AnsiPen pen
+ String key
+ LogLevel logLevel
+ String generateTextMessage(...)
}
DriftQueryLog --|> TalkerLog
class DriftResultLog {
+ TalkerDriftLoggerSettings settings
+ int? durationMs
+ int? rowCount
+ List<Map<String, Object?>>? rows
+ int? affected
+ AnsiPen pen
+ String key
+ LogLevel logLevel
+ String generateTextMessage(...)
}
DriftResultLog --|> TalkerLog
class DriftErrorLog {
+ TalkerDriftLoggerSettings settings
+ Object dbError
+ List<Object?> args
+ int? durationMs
+ AnsiPen pen
+ String key
+ LogLevel logLevel
+ String generateTextMessage(...)
}
DriftErrorLog --|> TalkerLog
class DriftTransactionLog {
+ TalkerDriftLoggerSettings settings
+ int? durationMs
+ AnsiPen pen
+ String key
+ LogLevel logLevel
+ String generateTextMessage(...)
}
DriftTransactionLog --|> TalkerLog
class DriftBatchLog {
+ TalkerDriftLoggerSettings settings
+ AnsiPen pen
+ String key
+ LogLevel logLevel
+ String generateTextMessage(...)
}
DriftBatchLog --|> TalkerLog
TalkerDriftLogger --> DriftQueryLog
TalkerDriftLogger --> DriftResultLog
TalkerDriftLogger --> DriftErrorLog
TalkerDriftLogger --> DriftTransactionLog
TalkerDriftLogger --> DriftBatchLog
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In TalkerDriftLogger constructor, consider using the predefined TalkerKey.drift* constants when registering log keys instead of hardcoded strings to avoid typos and ensure consistency.
- The interceptor methods for insert, update, delete, and custom all share similar patterns; extracting the common logic into a reusable private helper would reduce duplication and improve maintainability.
- The cosmetic changes to talker_flutter/pubspec.yaml (changing quote styles) seem unrelated to the drift logger feature; consider reverting these to keep the diff focused.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In TalkerDriftLogger constructor, consider using the predefined TalkerKey.drift* constants when registering log keys instead of hardcoded strings to avoid typos and ensure consistency.
- The interceptor methods for insert, update, delete, and custom all share similar patterns; extracting the common logic into a reusable private helper would reduce duplication and improve maintainability.
- The cosmetic changes to talker_flutter/pubspec.yaml (changing quote styles) seem unrelated to the drift logger feature; consider reverting these to keep the diff focused.
## Individual Comments
### Comment 1
<location> `packages/talker_drift_logger/lib/talker_drift_logger_interceptor.dart:50` </location>
<code_context>
+
+ bool _accept(String statement, List<Object?> args) {
+ if (!settings.enabled) return false;
+ final filter = settings.statementFilter;
+ if (filter == null) return true;
+ try {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider handling exceptions from statementFilter more explicitly.
If statementFilter throws, the error is swallowed and false is returned. Logging or surfacing the error would help with debugging misconfigurations and unexpected behavior.
Suggested implementation:
```
bool _accept(String statement, List<Object?> args) {
if (!settings.enabled) return false;
final filter = settings.statementFilter;
if (filter == null) return true;
try {
```
```
try {
return filter(statement, args);
} catch (e, stackTrace) {
// Log the error for debugging misconfigurations and unexpected behavior
print('Error in statementFilter: $e\n$stackTrace');
return false;
}
```
</issue_to_address>
### Comment 2
<location> `packages/talker_drift_logger/lib/talker_drift_logger_interceptor.dart:268` </location>
<code_context>
+ if (accepted) {
+ _talker.logCustom(DriftQueryLog(statement, args: args, settings: settings));
+ }
+ int? durationMs;
+ return _run<List<Map<String, Object?>>>(
+ statement,
</code_context>
<issue_to_address>
**issue (bug_risk):** Storing durationMs outside the async context may lead to race conditions.
Because durationMs is set in the onDone callback but used in the .then block, its value may be inconsistent if the future resolves before the callback. To prevent this, pass durationMs directly to DriftResultLog within the callback or refactor to eliminate shared mutable state.
</issue_to_address>
### Comment 3
<location> `packages/talker_drift_logger/lib/drift_logs.dart:119` </location>
<code_context>
+ final int? durationMs;
+ final int? rowCount;
+ final List<Map<String, Object?>>? rows;
+ final int? affected;
+
+ @override
</code_context>
<issue_to_address>
**suggestion:** The 'affected' field in DriftResultLog is not consistently set.
Please ensure 'affected' is set for all applicable operations, or remove it if it's not needed to maintain clarity.
Suggested implementation:
```
this.durationMs,
this.rowCount,
this.rows,
}) : super(statement);
final TalkerDriftLoggerSettings settings;
final int? durationMs;
final int? rowCount;
final List<Map<String, Object?>>? rows;
```
```
```
If you want to keep the `affected` field, you must ensure it is set for all applicable operations (e.g., in the constructors or factory methods that create `DriftResultLog` instances for INSERT, UPDATE, DELETE). If you decide to keep it, review all usages and ensure it is always set when relevant.
</issue_to_address>
### Comment 4
<location> `packages/talker_drift_logger/lib/talker_drift_logger_settings.dart:70` </location>
<code_context>
+ final String Function(List<Map<String, Object?>> rows)? resultPrinter;
+ final String Function(List<Object?> args)? argsPrinter;
+
+ TalkerDriftLoggerSettings copyWith({
+ bool? enabled,
+ LogLevel? logLevel,
</code_context>
<issue_to_address>
**suggestion:** copyWith does not allow unsetting nullable fields.
Currently, it's not possible to set a nullable field to null using copyWith. Please update the method to allow explicitly passing null for nullable fields.
Suggested implementation:
```
final bool Function(String statement, List<Object?> args)? statementFilter;
final bool Function(Object error)? errorFilter;
final String Function(List<Map<String, Object?>> rows)? resultPrinter;
final String Function(List<Object?> args)? argsPrinter;
static const _unset = Object();
TalkerDriftLoggerSettings copyWith({
bool? enabled,
LogLevel? logLevel,
bool? printArgs,
bool? printResults,
int? resultRowLimit,
int? resultMaxChars,
bool? printTransaction,
bool? printBatch,
Set<String>? obfuscateColumns,
Set<Pattern>? obfuscatePatterns,
AnsiPen? queryPen,
AnsiPen? resultPen,
Object? statementFilter = _unset,
Object? errorFilter = _unset,
Object? resultPrinter = _unset,
Object? argsPrinter = _unset,
Object? errorPen = _unset,
Object? transactionPen = _unset,
Object? batchPen = _unset,
=======
```
```
Set<String>? obfuscateColumns,
Set<Pattern>? obfuscatePatterns,
AnsiPen? queryPen,
AnsiPen? resultPen,
// Add other nullable fields here as needed
```
```
}) {
return TalkerDriftLoggerSettings(
enabled: enabled ?? this.enabled,
logLevel: logLevel ?? this.logLevel,
printArgs: printArgs ?? this.printArgs,
printResults: printResults ?? this.printResults,
resultRowLimit: resultRowLimit ?? this.resultRowLimit,
resultMaxChars: resultMaxChars ?? this.resultMaxChars,
printTransaction: printTransaction ?? this.printTransaction,
printBatch: printBatch ?? this.printBatch,
obfuscateColumns: obfuscateColumns ?? this.obfuscateColumns,
obfuscatePatterns: obfuscatePatterns ?? this.obfuscatePatterns,
queryPen: queryPen ?? this.queryPen,
resultPen: resultPen ?? this.resultPen,
statementFilter: identical(statementFilter, _unset)
? this.statementFilter
: statementFilter as bool Function(String, List<Object?>)?,
errorFilter: identical(errorFilter, _unset)
? this.errorFilter
: errorFilter as bool Function(Object)?,
resultPrinter: identical(resultPrinter, _unset)
? this.resultPrinter
: resultPrinter as String Function(List<Map<String, Object?>>)?,
argsPrinter: identical(argsPrinter, _unset)
? this.argsPrinter
: argsPrinter as String Function(List<Object?>)?,
errorPen: identical(errorPen, _unset)
? this.errorPen
: errorPen as AnsiPen?,
transactionPen: identical(transactionPen, _unset)
? this.transactionPen
: transactionPen as AnsiPen?,
batchPen: identical(batchPen, _unset)
? this.batchPen
: batchPen as AnsiPen?,
);
}
```
If there are other nullable fields in your class, you should add them to the `copyWith` signature and update the constructor call in the same way.
Update all usages of `copyWith` to use the new API, e.g. `copyWith(resultPrinter: null)` to unset.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| bool _accept(String statement, List<Object?> args) { | ||
| if (!settings.enabled) return false; | ||
| final filter = settings.statementFilter; |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Consider handling exceptions from statementFilter more explicitly.
If statementFilter throws, the error is swallowed and false is returned. Logging or surfacing the error would help with debugging misconfigurations and unexpected behavior.
Suggested implementation:
bool _accept(String statement, List<Object?> args) {
if (!settings.enabled) return false;
final filter = settings.statementFilter;
if (filter == null) return true;
try {
try {
return filter(statement, args);
} catch (e, stackTrace) {
// Log the error for debugging misconfigurations and unexpected behavior
print('Error in statementFilter: $e\n$stackTrace');
return false;
}
| if (accepted) { | ||
| _talker.logCustom(DriftQueryLog(statement, args: args, settings: settings)); | ||
| } | ||
| int? durationMs; |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Storing durationMs outside the async context may lead to race conditions.
Because durationMs is set in the onDone callback but used in the .then block, its value may be inconsistent if the future resolves before the callback. To prevent this, pass durationMs directly to DriftResultLog within the callback or refactor to eliminate shared mutable state.
| final int? durationMs; | ||
| final int? rowCount; | ||
| final List<Map<String, Object?>>? rows; | ||
| final int? affected; |
Contributor
There was a problem hiding this comment.
suggestion: The 'affected' field in DriftResultLog is not consistently set.
Please ensure 'affected' is set for all applicable operations, or remove it if it's not needed to maintain clarity.
Suggested implementation:
this.durationMs,
this.rowCount,
this.rows,
}) : super(statement);
final TalkerDriftLoggerSettings settings;
final int? durationMs;
final int? rowCount;
final List<Map<String, Object?>>? rows;
If you want to keep the affected field, you must ensure it is set for all applicable operations (e.g., in the constructors or factory methods that create DriftResultLog instances for INSERT, UPDATE, DELETE). If you decide to keep it, review all usages and ensure it is always set when relevant.
…rove filters error handling; fix runSelect duration race\n\n- Keys: replace hardcoded strings with TalkerKey.drift*\n- Filters: surface statementFilter/errorFilter exceptions via talker.handle\n- Select: measure duration inside async block to avoid race
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #426 +/- ##
==========================================
- Coverage 98.63% 91.14% -7.49%
==========================================
Files 3 13 +10
Lines 146 271 +125
==========================================
+ Hits 144 247 +103
- Misses 2 24 +22 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Frezyx
added
consideration
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
3 participants
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.
Добавил новый пакет - логгер для Drift (
talker_drift_logger) на базе Talker. В самом talker подвез ключи для drift (query/result/error/transaction/batch) и дефолтные тайтлы/цвета; в talker_flutter прописал эти цвета вTalkerScreen, чтобы логи были не серые.Интерцептор наследуется от
QueryInterceptorи перехватывает все базовые операции (select/insert/update/delete/custom), а также транзакции и батчи. Пишет в Talker: запрос, результат (с временем, количеством строк и срезом данных), и ошибку. Всё настраивается черезTalkerDriftLoggerSettings: можно включать/выключать печать аргументов/результатов, ограничивать объёмы, маскировать поля, фильтровать запросы/ошибки. Подключение - простоNativeDatabase(...).interceptWith(TalkerDriftLogger(...)).Есть базовые тесты и CI; в корневом README добавил пакет и краткую секцию.
Summary by Sourcery
Add talker_drift_logger package to log Drift database operations through Talker, register new drift log keys and colors in Talker core and Flutter UI, and include configuration settings, documentation, examples, tests, and CI setup.
New Features:
Enhancements:
CI:
Documentation:
Tests: