Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Since the SQLite driver has been swapped out, double-check that any driver-specific behavior (DSN format, pragmas, busy timeout / WAL settings, and default transaction behavior) provided by the previous fork is either preserved or explicitly reconfigured with github.com/libtnb/sqlite to avoid subtle runtime differences.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Since the SQLite driver has been swapped out, double-check that any driver-specific behavior (DSN format, pragmas, busy timeout / WAL settings, and default transaction behavior) provided by the previous fork is either preserved or explicitly reconfigured with github.com/libtnb/sqlite to avoid subtle runtime differences.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@naltatis this fails |
|
It also fails backup/restore where we check that a saved title is restored from backup. Does not look like brittle expectation, more like a real issue. |
|
Can we find out why? Looks like a proper library, I'd expect it to actually work :O |
|
Looks like two issue. The driver uses another string time format: The integration test fails because of the addition of |
|
Regarding time string format: I'm unsure if the new format would create issues or if GORM will simply convert both (old and new) to proper time. However, this mix would lead to inconsistency in raw data. |
Ok. I always assumed WAL mode is the default. We should re-add this and do a SQL export instead. There's also a dedicated backup api (https://sqlite.org/backup.html), let's see if that's supported here. |
Just did a functional check. Both time formats seem to be readable, but I'm really confused by this new length date format. Maybe it's time to also switch to unix timestamp here to get rid of this topic :D |
+1, but sounds like a topic for a dedicate PR |
|
@naltatis added draft backup. Agreed this is only needed for WAL mode. Might move to dedicated function and keep nonetheless. |
|
Time should be fixed- let's see whats missing now. |
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="server/db/db.go" line_range="118-124" />
<code_context>
return db.Close()
}
+
+func Backup(ctx context.Context, target string) error {
+ live, err := Instance.DB()
+ if err != nil {
+ return err
+ }
+
+ conn, err := live.Conn(ctx)
+ if err != nil {
+ return err
</code_context>
<issue_to_address>
**issue (bug_risk):** The *sql.Conn acquired in Backup is never closed, which can leak connections.
After `live.Conn(ctx)` succeeds, ensure the returned `*sql.Conn` is closed (e.g., `defer conn.Close()`) after `conn.Raw(...)` completes. Leaving it open for the process lifetime can exhaust the connection pool and underlying resources over time.
</issue_to_address>
### Comment 2
<location path="server/db/db.go" line_range="129-133" />
<code_context>
+ return err
+ }
+
+ return conn.Raw(func(driverConn any) error {
+ type backuper interface {
+ NewBackup(string) (*sqlite3.Backup, error)
+ NewRestore(string) (*sqlite3.Backup, error)
+ }
+
</code_context>
<issue_to_address>
**suggestion:** The backuper interface includes NewRestore but it is never used.
You can narrow the `backuper` interface to only `NewBackup`. Leaving `NewRestore` unused makes the assertion more restrictive than needed and more fragile if the driver changes.
```suggestion
return conn.Raw(func(driverConn any) error {
type backuper interface {
NewBackup(string) (*sqlite3.Backup, error)
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
2 participants
Follow-up to #29301
TODO
Out of scope