GO-3153 sqlite spacestore #1092
Conversation
Coverage provided by https://github.com/seriousben/go-patch-cover-action |
cheggaaa
force-pushed
the
GO-3153-sqlite-spacestore
branch
from
6397658 to
519313b
Compare
…t into GO-3153-sqlite-spacestore # Conflicts: # go.mod # go.sum
WalkthroughThe update encompasses a significant overhaul in storage management within the system. Key changes include the renaming of the Changes
Recent Review DetailsConfiguration used: CodeRabbit UI Files ignored due to path filters (2)
Files selected for processing (2)
Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| func createSpaceStorage(s *storageService, payload spacestorage.SpaceStorageCreatePayload) (spacestorage.SpaceStorage, error) { | ||
| tx, err := s.writeDb.Begin() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if _, err = tx.Stmt(s.stmt.createSpace).Exec( | ||
| // space(id, header, settingsId, aclId) | ||
| payload.SpaceHeaderWithId.Id, | ||
| payload.SpaceHeaderWithId.RawHeader, | ||
| payload.SpaceSettingsWithId.Id, | ||
| payload.AclWithId.Id, | ||
| ); err != nil { | ||
| _ = tx.Rollback() | ||
| if isUniqueConstraint(err) { | ||
| return nil, spacestorage.ErrSpaceStorageExists | ||
| } | ||
| return nil, err | ||
| } | ||
|
|
||
| if _, err = tx.Stmt(s.stmt.createTree).Exec( | ||
| // settings tree (id, spaceId, heads, type) | ||
| payload.SpaceSettingsWithId.Id, | ||
| payload.SpaceHeaderWithId.Id, | ||
| treestorage.CreateHeadsPayload([]string{payload.SpaceSettingsWithId.Id}), | ||
| treeTypeSettings, | ||
| ); err != nil { | ||
| _ = tx.Rollback() | ||
| return nil, err | ||
| } | ||
|
|
||
| if _, err = tx.Stmt(s.stmt.createTree).Exec( | ||
| // acl list tree (id, spaceId, heads, type) | ||
| payload.AclWithId.Id, | ||
| payload.SpaceHeaderWithId.Id, | ||
| payload.AclWithId.Id, | ||
| treeTypeList, | ||
| ); err != nil { | ||
| _ = tx.Rollback() | ||
| return nil, err | ||
| } | ||
|
|
||
| if _, err = tx.Stmt(s.stmt.createChange).Exec( | ||
| // settings change (id, spaceId, treeId, data) | ||
| payload.SpaceSettingsWithId.Id, | ||
| payload.SpaceHeaderWithId.Id, | ||
| payload.SpaceSettingsWithId.Id, | ||
| payload.SpaceSettingsWithId.RawChange, | ||
| ); err != nil { | ||
| _ = tx.Rollback() | ||
| return nil, err | ||
| } | ||
|
|
||
| if _, err = tx.Stmt(s.stmt.createChange).Exec( | ||
| // acl change (id, spaceId, treeId, data) | ||
| payload.AclWithId.Id, | ||
| payload.SpaceHeaderWithId.Id, | ||
| payload.AclWithId.Id, | ||
| payload.AclWithId.Payload, | ||
| ); err != nil { | ||
| _ = tx.Rollback() | ||
| return nil, err | ||
| } | ||
|
|
||
| if err = tx.Commit(); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| ss := &spaceStorage{ | ||
| spaceId: payload.SpaceHeaderWithId.Id, | ||
| service: s, | ||
| spaceSettingsId: payload.SpaceSettingsWithId.Id, | ||
| header: payload.SpaceHeaderWithId, | ||
| aclId: payload.AclWithId.Id, | ||
| } | ||
|
|
||
| if ss.aclStorage, err = newListStorage(ss, ss.aclId); err != nil { | ||
| _ = tx.Rollback() | ||
| return nil, err | ||
| } | ||
|
|
||
| return ss, nil |
There was a problem hiding this comment.
Optimize error handling in createSpaceStorage by consolidating the repeated tx.Rollback() calls into a deferred function. This will ensure that rollback is attempted in all error scenarios without repeating the code.
- if _, err = tx.Stmt(s.stmt.createSpace).Exec(
+ defer func() {
+ if err != nil {
+ _ = tx.Rollback()
+ }
+ }()
+ if _, err = tx.Stmt(s.stmt.createSpace).Exec(Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| func createSpaceStorage(s *storageService, payload spacestorage.SpaceStorageCreatePayload) (spacestorage.SpaceStorage, error) { | |
| tx, err := s.writeDb.Begin() | |
| if err != nil { | |
| return nil, err | |
| } | |
| if _, err = tx.Stmt(s.stmt.createSpace).Exec( | |
| // space(id, header, settingsId, aclId) | |
| payload.SpaceHeaderWithId.Id, | |
| payload.SpaceHeaderWithId.RawHeader, | |
| payload.SpaceSettingsWithId.Id, | |
| payload.AclWithId.Id, | |
| ); err != nil { | |
| _ = tx.Rollback() | |
| if isUniqueConstraint(err) { | |
| return nil, spacestorage.ErrSpaceStorageExists | |
| } | |
| return nil, err | |
| } | |
| if _, err = tx.Stmt(s.stmt.createTree).Exec( | |
| // settings tree (id, spaceId, heads, type) | |
| payload.SpaceSettingsWithId.Id, | |
| payload.SpaceHeaderWithId.Id, | |
| treestorage.CreateHeadsPayload([]string{payload.SpaceSettingsWithId.Id}), | |
| treeTypeSettings, | |
| ); err != nil { | |
| _ = tx.Rollback() | |
| return nil, err | |
| } | |
| if _, err = tx.Stmt(s.stmt.createTree).Exec( | |
| // acl list tree (id, spaceId, heads, type) | |
| payload.AclWithId.Id, | |
| payload.SpaceHeaderWithId.Id, | |
| payload.AclWithId.Id, | |
| treeTypeList, | |
| ); err != nil { | |
| _ = tx.Rollback() | |
| return nil, err | |
| } | |
| if _, err = tx.Stmt(s.stmt.createChange).Exec( | |
| // settings change (id, spaceId, treeId, data) | |
| payload.SpaceSettingsWithId.Id, | |
| payload.SpaceHeaderWithId.Id, | |
| payload.SpaceSettingsWithId.Id, | |
| payload.SpaceSettingsWithId.RawChange, | |
| ); err != nil { | |
| _ = tx.Rollback() | |
| return nil, err | |
| } | |
| if _, err = tx.Stmt(s.stmt.createChange).Exec( | |
| // acl change (id, spaceId, treeId, data) | |
| payload.AclWithId.Id, | |
| payload.SpaceHeaderWithId.Id, | |
| payload.AclWithId.Id, | |
| payload.AclWithId.Payload, | |
| ); err != nil { | |
| _ = tx.Rollback() | |
| return nil, err | |
| } | |
| if err = tx.Commit(); err != nil { | |
| return nil, err | |
| } | |
| ss := &spaceStorage{ | |
| spaceId: payload.SpaceHeaderWithId.Id, | |
| service: s, | |
| spaceSettingsId: payload.SpaceSettingsWithId.Id, | |
| header: payload.SpaceHeaderWithId, | |
| aclId: payload.AclWithId.Id, | |
| } | |
| if ss.aclStorage, err = newListStorage(ss, ss.aclId); err != nil { | |
| _ = tx.Rollback() | |
| return nil, err | |
| } | |
| return ss, nil | |
| func createSpaceStorage(s *storageService, payload spacestorage.SpaceStorageCreatePayload) (spacestorage.SpaceStorage, error) { | |
| tx, err := s.writeDb.Begin() | |
| if err != nil { | |
| return nil, err | |
| } | |
| defer func() { | |
| if err != nil { | |
| _ = tx.Rollback() | |
| } | |
| }() | |
| if _, err = tx.Stmt(s.stmt.createSpace).Exec( | |
| // space(id, header, settingsId, aclId) | |
| payload.SpaceHeaderWithId.Id, | |
| payload.SpaceHeaderWithId.RawHeader, | |
| payload.SpaceSettingsWithId.Id, | |
| payload.AclWithId.Id, | |
| ); err != nil { | |
| if isUniqueConstraint(err) { | |
| return nil, spacestorage.ErrSpaceStorageExists | |
| } | |
| return nil, err | |
| } | |
| if _, err = tx.Stmt(s.stmt.createTree).Exec( | |
| // settings tree (id, spaceId, heads, type) | |
| payload.SpaceSettingsWithId.Id, | |
| payload.SpaceHeaderWithId.Id, | |
| treestorage.CreateHeadsPayload([]string{payload.SpaceSettingsWithId.Id}), | |
| treeTypeSettings, | |
| ); err != nil { | |
| return nil, err | |
| } | |
| if _, err = tx.Stmt(s.stmt.createTree).Exec( | |
| // acl list tree (id, spaceId, heads, type) | |
| payload.AclWithId.Id, | |
| payload.SpaceHeaderWithId.Id, | |
| payload.AclWithId.Id, | |
| treeTypeList, | |
| ); err != nil { | |
| return nil, err | |
| } | |
| if _, err = tx.Stmt(s.stmt.createChange).Exec( | |
| // settings change (id, spaceId, treeId, data) | |
| payload.SpaceSettingsWithId.Id, | |
| payload.SpaceHeaderWithId.Id, | |
| payload.SpaceSettingsWithId.Id, | |
| payload.SpaceSettingsWithId.RawChange, | |
| ); err != nil { | |
| return nil, err | |
| } | |
| if _, err = tx.Stmt(s.stmt.createChange).Exec( | |
| // acl change (id, spaceId, treeId, data) | |
| payload.AclWithId.Id, | |
| payload.SpaceHeaderWithId.Id, | |
| payload.AclWithId.Id, | |
| payload.AclWithId.Payload, | |
| ); err != nil { | |
| return nil, err | |
| } | |
| if err = tx.Commit(); err != nil { | |
| return nil, err | |
| } | |
| ss := &spaceStorage{ | |
| spaceId: payload.SpaceHeaderWithId.Id, | |
| service: s, | |
| spaceSettingsId: payload.SpaceSettingsWithId.Id, | |
| header: payload.SpaceHeaderWithId, | |
| aclId: payload.AclWithId.Id, | |
| } | |
| if ss.aclStorage, err = newListStorage(ss, ss.aclId); err != nil { | |
| return nil, err | |
| } | |
| return ss, nil | |
| } |
| func TestListStorage_AddRawRecord(t *testing.T) { | ||
| fx := newFixture(t) | ||
| defer fx.finish(t) | ||
|
|
||
| payload := spaceTestPayload() | ||
| ss, err := fx.CreateSpaceStorage(payload) | ||
| require.NoError(t, err) | ||
| defer func() { _ = ss.Close(ctx) }() | ||
|
|
||
| ls, err := ss.AclStorage() | ||
| require.NoError(t, err) | ||
|
|
||
| var rec = &consensusproto.RawRecordWithId{ | ||
| Id: "id", | ||
| Payload: []byte("data"), | ||
| } | ||
|
|
||
| require.NoError(t, ls.AddRawRecord(ctx, rec)) | ||
|
|
||
| res, err := ls.GetRawRecord(ctx, rec.Id) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, rec, res) | ||
| } |
There was a problem hiding this comment.
Ensure proper context management in deferred function calls.
- defer func() { _ = ss.Close(ctx) }()
+ defer func() { _ = ss.Close() }()The ctx variable is not defined within the scope of the deferred function, which might lead to a runtime error. It seems like the Close method does not require a context parameter based on typical Go context usage in similar functions.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| func TestListStorage_AddRawRecord(t *testing.T) { | |
| fx := newFixture(t) | |
| defer fx.finish(t) | |
| payload := spaceTestPayload() | |
| ss, err := fx.CreateSpaceStorage(payload) | |
| require.NoError(t, err) | |
| defer func() { _ = ss.Close(ctx) }() | |
| ls, err := ss.AclStorage() | |
| require.NoError(t, err) | |
| var rec = &consensusproto.RawRecordWithId{ | |
| Id: "id", | |
| Payload: []byte("data"), | |
| } | |
| require.NoError(t, ls.AddRawRecord(ctx, rec)) | |
| res, err := ls.GetRawRecord(ctx, rec.Id) | |
| require.NoError(t, err) | |
| assert.Equal(t, rec, res) | |
| } | |
| func TestListStorage_AddRawRecord(t *testing.T) { | |
| fx := newFixture(t) | |
| defer fx.finish(t) | |
| payload := spaceTestPayload() | |
| ss, err := fx.CreateSpaceStorage(payload) | |
| require.NoError(t, err) | |
| defer func() { _ = ss.Close() }() | |
| ls, err := ss.AclStorage() | |
| require.NoError(t, err) | |
| var rec = &consensusproto.RawRecordWithId{ | |
| Id: "id", | |
| Payload: []byte("data"), | |
| } | |
| require.NoError(t, ls.AddRawRecord(ctx, rec)) | |
| res, err := ls.GetRawRecord(ctx, rec.Id) | |
| require.NoError(t, err) | |
| assert.Equal(t, rec, res) | |
| } |
| func newListStorage(ss *spaceStorage, treeId string) (liststorage.ListStorage, error) { | ||
| ts := &listStorage{ | ||
| listId: treeId, | ||
| spaceStorage: ss, | ||
| service: ss.service, | ||
| } | ||
| if err := ss.service.stmt.loadTreeHeads.QueryRow(treeId).Scan(&ts.head); err != nil { | ||
| if errors.Is(err, sql.ErrNoRows) { | ||
| return nil, treestorage.ErrUnknownTreeId | ||
| } | ||
| return nil, err | ||
| } | ||
| return ts, nil | ||
| } |
There was a problem hiding this comment.
Handle potential SQL injection vulnerabilities.
- if err := ss.service.stmt.loadTreeHeads.QueryRow(treeId).Scan(&ts.head); err != nil {
+ if err := ss.service.stmt.loadTreeHeads.QueryRow(sql.EscapeString(treeId)).Scan(&ts.head); err != nil {The treeId parameter is directly used in a SQL query, which could lead to SQL injection if not properly sanitized. Consider using parameterized queries or properly escaping inputs to mitigate this risk.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| func newListStorage(ss *spaceStorage, treeId string) (liststorage.ListStorage, error) { | |
| ts := &listStorage{ | |
| listId: treeId, | |
| spaceStorage: ss, | |
| service: ss.service, | |
| } | |
| if err := ss.service.stmt.loadTreeHeads.QueryRow(treeId).Scan(&ts.head); err != nil { | |
| if errors.Is(err, sql.ErrNoRows) { | |
| return nil, treestorage.ErrUnknownTreeId | |
| } | |
| return nil, err | |
| } | |
| return ts, nil | |
| } | |
| func newListStorage(ss *spaceStorage, treeId string) (liststorage.ListStorage, error) { | |
| ts := &listStorage{ | |
| listId: treeId, | |
| spaceStorage: ss, | |
| service: ss.service, | |
| } | |
| if err := ss.service.stmt.loadTreeHeads.QueryRow(sql.EscapeString(treeId)).Scan(&ts.head); err != nil { | |
| if errors.Is(err, sql.ErrNoRows) { | |
| return nil, treestorage.ErrUnknownTreeId | |
| } | |
| return nil, err | |
| } | |
| return ts, nil | |
| } |
There was a problem hiding this comment.
Escape is not needed because there is a prepared statement. No way to do SQL injection
There was a problem hiding this comment.
@cheggaaa, thank you for your clarification. You are correct that the use of prepared statements effectively mitigates the risk of SQL injection. The QueryRow method in this context is indeed safe as it operates on a prepared statement, ensuring that treeId is handled securely. I appreciate your attention to detail and ensuring the security of the code.
New Learnings Added
User: cheggaaa"
URL: https://github.com/anyproto/anytype-heart/pull/1092
Timestamp: 2024-04-15T11:56:47.495Z
Learning: The `QueryRow` method used with `treeId` in the `newListStorage` function of the `sqlitestorage` package is safe from SQL injection due to the use of prepared statements.
Note: Learnings are effective only in the context of specific code segments. To apply general coding guidelines, please configure review instructions.
| func initStmts(s *storageService) (err error) { | ||
| if s.stmt.createSpace, err = s.writeDb.Prepare(`INSERT INTO spaces(id, header, settingsId, aclId) VALUES (?, ?, ?, ?)`); err != nil { | ||
| return | ||
| } | ||
| if s.stmt.createTree, err = s.writeDb.Prepare(`INSERT INTO trees(id, spaceId, heads, type) VALUES(?, ?, ?, ?)`); err != nil { | ||
| return | ||
| } | ||
| if s.stmt.createChange, err = s.writeDb.Prepare(`INSERT INTO changes(id, spaceId, treeId, data) VALUES(?, ?, ?, ?)`); err != nil { | ||
| return | ||
| } | ||
| if s.stmt.updateSpaceHash, err = s.writeDb.Prepare(`UPDATE spaces SET hash = ? WHERE id = ?`); err != nil { | ||
| return | ||
| } | ||
| if s.stmt.updateSpaceOldHash, err = s.writeDb.Prepare(`UPDATE spaces SET oldHash = ? WHERE id = ?`); err != nil { | ||
| return | ||
| } | ||
| if s.stmt.updateSpaceIsCreated, err = s.writeDb.Prepare(`UPDATE spaces SET isCreated = ? WHERE id = ?`); err != nil { | ||
| return | ||
| } | ||
| if s.stmt.updateSpaceIsDeleted, err = s.writeDb.Prepare(`UPDATE spaces SET isDeleted = ? WHERE id = ?`); err != nil { | ||
| return | ||
| } | ||
| if s.stmt.treeIdsBySpace, err = s.readDb.Prepare(`SELECT id FROM trees WHERE spaceId = ? AND type != 1`); err != nil { | ||
| return | ||
| } | ||
| if s.stmt.updateTreeDelStatus, err = s.writeDb.Prepare(`UPDATE trees SET deleteStatus = ? WHERE id = ?`); err != nil { | ||
| return | ||
| } | ||
| if s.stmt.treeDelStatus, err = s.readDb.Prepare(`SELECT deleteStatus FROM trees WHERE id = ?`); err != nil { | ||
| return | ||
| } | ||
| if s.stmt.change, err = s.readDb.Prepare(`SELECT data FROM changes WHERE id = ? AND spaceId = ?`); err != nil { | ||
| return | ||
| } | ||
| if s.stmt.hasTree, err = s.readDb.Prepare(`SELECT COUNT(*) FROM trees WHERE id = ? AND spaceId = ?`); err != nil { | ||
| return | ||
| } | ||
| if s.stmt.hasChange, err = s.readDb.Prepare(`SELECT COUNT(*) FROM changes WHERE id = ? AND treeId = ?`); err != nil { | ||
| return | ||
| } | ||
| if s.stmt.updateTreeHeads, err = s.writeDb.Prepare(`UPDATE trees SET heads = ? WHERE id = ?`); err != nil { | ||
| return | ||
| } | ||
| if s.stmt.deleteTree, err = s.writeDb.Prepare(`DELETE FROM trees WHERE id = ?`); err != nil { | ||
| return | ||
| } | ||
| if s.stmt.deleteChangesByTree, err = s.writeDb.Prepare(`DELETE FROM changes WHERE treeId = ?`); err != nil { | ||
| return | ||
| } | ||
| if s.stmt.loadTreeHeads, err = s.readDb.Prepare(`SELECT heads FROM trees WHERE id = ?`); err != nil { | ||
| return | ||
| } | ||
| if s.stmt.loadSpace, err = s.readDb.Prepare(`SELECT header, settingsId, aclId, hash, oldHash, isDeleted FROM spaces WHERE id = ?`); err != nil { | ||
| return | ||
| } | ||
| if s.stmt.spaceIds, err = s.readDb.Prepare(`SELECT id FROM spaces`); err != nil { | ||
| return | ||
| } | ||
| if s.stmt.spaceIsCreated, err = s.readDb.Prepare(`SELECT isCreated FROM spaces WHERE id = ?`); err != nil { | ||
| return | ||
| } | ||
| if s.stmt.getBind, err = s.readDb.Prepare(`SELECT spaceId FROM binds WHERE objectId = ?`); err != nil { | ||
| return | ||
| } | ||
| if s.stmt.upsertBind, err = s.writeDb.Prepare(`INSERT INTO binds (objectId, spaceId) VALUES (?, ?) ON CONFLICT (objectId) DO UPDATE SET spaceId = ?`); err != nil { | ||
| return | ||
| } | ||
| if s.stmt.deleteSpace, err = s.writeDb.Prepare(`DELETE FROM spaces WHERE id = ?`); err != nil { | ||
| return | ||
| } | ||
| if s.stmt.deleteTreesBySpace, err = s.writeDb.Prepare(`DELETE FROM trees WHERE spaceId = ?`); err != nil { | ||
| return | ||
| } | ||
| if s.stmt.deleteChangesBySpace, err = s.writeDb.Prepare(`DELETE FROM changes WHERE spaceId = ?`); err != nil { | ||
| return | ||
| } | ||
| if s.stmt.deleteBindsBySpace, err = s.writeDb.Prepare(`DELETE FROM binds WHERE spaceId = ?`); err != nil { | ||
| return | ||
| } | ||
| return |
There was a problem hiding this comment.
Ensure robust error handling in initStmts.
The function initStmts initializes various SQL statements and returns immediately upon encountering an error. This approach can lead to resource leaks if some statements are prepared successfully before an error occurs. Consider deferring the closure of all successfully prepared statements upon an error to ensure that no database connections are leaked.
| func newTreeStorage(ss *spaceStorage, treeId string) (treestorage.TreeStorage, error) { | ||
| ts := &treeStorage{ | ||
| treeId: treeId, | ||
| spaceStorage: ss, | ||
| service: ss.service, | ||
| } | ||
| var headsPayload []byte | ||
| if err := ss.service.stmt.loadTreeHeads.QueryRow(treeId).Scan(&headsPayload); err != nil { | ||
| if errors.Is(err, sql.ErrNoRows) { | ||
| return nil, treestorage.ErrUnknownTreeId | ||
| } | ||
| return nil, err | ||
| } | ||
| ts.heads = treestorage.ParseHeads(headsPayload) | ||
| return ts, nil | ||
| } |
There was a problem hiding this comment.
Ensure proper error handling and SQL injection prevention in newTreeStorage.
- if err := ss.service.stmt.loadTreeHeads.QueryRow(treeId).Scan(&headsPayload); err != nil {
+ if err := ss.service.stmt.loadTreeHeads.QueryRow(sql.EscapeString(treeId)).Scan(&headsPayload); err != nil {Similar to the list storage, the treeId parameter is directly used in a SQL query in newTreeStorage. This could lead to SQL injection if not properly sanitized. Use parameterized queries or proper escaping to mitigate this risk.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| func newTreeStorage(ss *spaceStorage, treeId string) (treestorage.TreeStorage, error) { | |
| ts := &treeStorage{ | |
| treeId: treeId, | |
| spaceStorage: ss, | |
| service: ss.service, | |
| } | |
| var headsPayload []byte | |
| if err := ss.service.stmt.loadTreeHeads.QueryRow(treeId).Scan(&headsPayload); err != nil { | |
| if errors.Is(err, sql.ErrNoRows) { | |
| return nil, treestorage.ErrUnknownTreeId | |
| } | |
| return nil, err | |
| } | |
| ts.heads = treestorage.ParseHeads(headsPayload) | |
| return ts, nil | |
| } | |
| func newTreeStorage(ss *spaceStorage, treeId string) (treestorage.TreeStorage, error) { | |
| ts := &treeStorage{ | |
| treeId: treeId, | |
| spaceStorage: ss, | |
| service: ss.service, | |
| } | |
| var headsPayload []byte | |
| if err := ss.service.stmt.loadTreeHeads.QueryRow(sql.EscapeString(treeId)).Scan(&headsPayload); err != nil { | |
| if errors.Is(err, sql.ErrNoRows) { | |
| return nil, treestorage.ErrUnknownTreeId | |
| } | |
| return nil, err | |
| } | |
| ts.heads = treestorage.ParseHeads(headsPayload) | |
| return ts, nil | |
| } |
| func createTreeStorage(ss *spaceStorage, payload treestorage.TreeStorageCreatePayload) (ts treestorage.TreeStorage, err error) { | ||
| ts = &treeStorage{ | ||
| treeId: payload.RootRawChange.Id, | ||
| spaceStorage: ss, | ||
| service: ss.service, | ||
| heads: []string{payload.RootRawChange.Id}, | ||
| } | ||
|
|
||
| tx, err := ss.service.writeDb.Begin() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if _, err := tx.Stmt(ss.service.stmt.createTree).Exec( | ||
| payload.RootRawChange.Id, | ||
| ss.spaceId, | ||
| treestorage.CreateHeadsPayload(payload.Heads), | ||
| treeTypeTree, | ||
| ); err != nil { | ||
| _ = tx.Rollback() | ||
| if isUniqueConstraint(err) { | ||
| return nil, treestorage.ErrTreeExists | ||
| } | ||
| return nil, err | ||
| } | ||
| if _, err = tx.Stmt(ss.service.stmt.createChange).Exec( | ||
| // root change (id, spaceId, treeId, data) | ||
| payload.RootRawChange.Id, | ||
| ss.spaceId, | ||
| payload.RootRawChange.Id, | ||
| payload.RootRawChange.RawChange, | ||
| ); err != nil { | ||
| _ = tx.Rollback() | ||
| return nil, err | ||
| } | ||
|
|
||
| for _, change := range payload.Changes { | ||
| _, chErr := tx.Stmt(ss.service.stmt.createChange).Exec(change.Id, ss.spaceId, ts.Id(), change.RawChange) | ||
| if chErr != nil { | ||
| if isUniqueConstraint(chErr) { | ||
| continue | ||
| } else { | ||
| _ = tx.Rollback() | ||
| return nil, chErr | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if err = tx.Commit(); err != nil { | ||
| return nil, err | ||
| } | ||
| return ts, nil | ||
| } |
There was a problem hiding this comment.
Review transaction handling in createTreeStorage.
The function createTreeStorage uses transactions to ensure atomicity of multiple database operations. However, the rollback mechanism is manually handled in multiple places. Consider using a deferred rollback check that only triggers if the commit hasn't been successful. This would simplify the code and reduce the possibility of errors in transaction management.
| func New() ClientStorage { | ||
| return &storageService{} | ||
| } | ||
|
|
||
| func (s *storageService) Init(a *app.App) (err error) { | ||
| s.dbPath = a.MustComponent("config").(configGetter).GetSpaceStorePath() | ||
| s.lockedSpaces = map[string]*lockSpace{} | ||
| return | ||
| } | ||
|
|
||
| func (s *storageService) Run(ctx context.Context) (err error) { | ||
| connectionUrlParams := make(url.Values) | ||
| connectionUrlParams.Add("_txlock", "immediate") | ||
| connectionUrlParams.Add("_journal_mode", "WAL") | ||
| connectionUrlParams.Add("_busy_timeout", "5000") | ||
| connectionUrlParams.Add("_synchronous", "NORMAL") | ||
| connectionUrlParams.Add("_cache_size", "10000000") | ||
| connectionUrlParams.Add("_foreign_keys", "true") | ||
| connectionUri := s.dbPath + "?" + connectionUrlParams.Encode() | ||
| if s.writeDb, err = sql.Open("sqlite3", connectionUri); err != nil { | ||
| return | ||
| } | ||
| s.writeDb.SetMaxOpenConns(1) | ||
| if _, err = s.writeDb.Exec(sqlCreateTables); err != nil { | ||
| return | ||
| } | ||
|
|
||
| if s.readDb, err = sql.Open("sqlite3", connectionUri); err != nil { | ||
| return | ||
| } | ||
| s.readDb.SetMaxOpenConns(10) | ||
|
|
||
| return initStmts(s) | ||
| } |
There was a problem hiding this comment.
Review the initialization and running of the storage service.
The methods Init and Run of storageService set up the database connections and prepare SQL statements. It is crucial to ensure that resources are properly managed and errors are handled appropriately. Consider using a more robust error handling and resource management strategy, such as using defer to clean up resources in case of initialization failures.
core/anytype/config/config.go
Outdated
| // check if sqlite db exists | ||
| if _, err2 := os.Stat(filepath.Join(repoPath, SpaceStoreSqlitePath)); err2 == nil { | ||
| // already have sqlite db | ||
| c.SpaceStorageMode = storage.SpaceStorageModeSqlite | ||
| } else if _, err2 = os.Stat(filepath.Join(repoPath, SpaceStoreBadgerPath)); err2 == nil { | ||
| // old account repos | ||
| c.SpaceStorageMode = storage.SpaceStorageModeBadger | ||
| } else { | ||
| // new account repos | ||
| c.SpaceStorageMode = storage.SpaceStorageModeSqlite | ||
| } |
There was a problem hiding this comment.
Ensure proper error handling when checking file existence.
Currently, the existence checks for the SQLite and Badger database files do not handle possible errors other than the file not being found (e.g., permission issues). Consider refining the error handling to cover these cases to avoid potential runtime issues.
- if _, err2 := os.Stat(filepath.Join(repoPath, SpaceStoreSqlitePath)); err2 == nil {
+ if _, err2 := os.Stat(filepath.Join(repoPath, SpaceStoreSqlitePath)); err2 != nil {
+ if !os.IsNotExist(err2) {
+ return fmt.Errorf("failed to check SQLite DB file: %w", err2)
+ }
+ } else {
c.SpaceStorageMode = storage.SpaceStorageModeSqlite
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| // check if sqlite db exists | |
| if _, err2 := os.Stat(filepath.Join(repoPath, SpaceStoreSqlitePath)); err2 == nil { | |
| // already have sqlite db | |
| c.SpaceStorageMode = storage.SpaceStorageModeSqlite | |
| } else if _, err2 = os.Stat(filepath.Join(repoPath, SpaceStoreBadgerPath)); err2 == nil { | |
| // old account repos | |
| c.SpaceStorageMode = storage.SpaceStorageModeBadger | |
| } else { | |
| // new account repos | |
| c.SpaceStorageMode = storage.SpaceStorageModeSqlite | |
| } | |
| // check if sqlite db exists | |
| if _, err2 := os.Stat(filepath.Join(repoPath, SpaceStoreSqlitePath)); err2 != nil { | |
| if !os.IsNotExist(err2) { | |
| return fmt.Errorf("failed to check SQLite DB file: %w", err2) | |
| } | |
| } else { | |
| c.SpaceStorageMode = storage.SpaceStorageModeSqlite | |
| } | |
| if _, err2 = os.Stat(filepath.Join(repoPath, SpaceStoreBadgerPath)); err2 == nil { | |
| // old account repos | |
| c.SpaceStorageMode = storage.SpaceStorageModeBadger | |
| } else { | |
| // new account repos | |
| c.SpaceStorageMode = storage.SpaceStorageModeSqlite | |
| } |
There was a problem hiding this comment.
we need this if statements only in case db exists. So don't need to check other errors
There was a problem hiding this comment.
@requilence, I understand your point about focusing on the existence checks for the database files and not handling other errors in this specific context. This approach can indeed simplify the code if we are confident that other error types (e.g., permission issues) are unlikely or managed elsewhere in the application.
Just a reminder, while this approach is streamlined, it's important to be cautious about scenarios where other errors might occur and ensure they won't negatively impact the application's stability or data integrity. If you're confident these cases are adequately handled or are very unlikely due to the environment setup, then your approach is sound.
If there's anything else you'd like to discuss or need further assistance with, feel free to let me know!
| @@ -104,7 +113,7 @@ func (s *clientServer) prepareListener(port int) (net.Listener, error) { | |||
| } | |||
| } | |||
| // otherwise listening to new port | |||
| //nolint: gosec | |||
| // nolint: gosec | |||
There was a problem hiding this comment.
Consider specifying a more restrictive IP address instead of 0.0.0.0 to reduce the risk of exposing the server publicly on all interfaces.
- return net.Listen("tcp", ":")
+ return net.Listen("tcp", "127.0.0.1:")Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| // nolint: gosec | |
| // nolint: gosec |
| s.stmt.deleteTreesBySpace, | ||
| } { | ||
| if _, err = tx.Stmt(stmt).Exec(spaceId); err != nil { | ||
| _ = tx.Rollback() |
There was a problem hiding this comment.
Check rollback error handling in deleteSpace.
The error from tx.Rollback() is not checked, which could lead to unnoticed failures in rolling back the transaction.
- _ = tx.Rollback()
+ if err := tx.Rollback(); err != nil {
+ return fmt.Errorf("failed to rollback: %w", err)
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| _ = tx.Rollback() | |
| if err := tx.Rollback(); err != nil { | |
| return fmt.Errorf("failed to rollback: %w", err) | |
| } |
| r.spaceStorageMode = cfgGetter.GetSpaceStorageMode() | ||
| } else { | ||
| return fmt.Errorf("ds config is missing") | ||
| return fmt.Errorf("" + | ||
| "ds config is missing") |
There was a problem hiding this comment.
Handling of missing datastore configuration.
The error handling for missing datastore configuration could be improved by providing more specific error messages or suggestions for resolution.
- return fmt.Errorf("" +
- "ds config is missing")
+ return fmt.Errorf("datastore configuration is missing, please ensure it is correctly provided")Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| r.spaceStorageMode = cfgGetter.GetSpaceStorageMode() | |
| } else { | |
| return fmt.Errorf("ds config is missing") | |
| return fmt.Errorf("" + | |
| "ds config is missing") | |
| r.spaceStorageMode = cfgGetter.GetSpaceStorageMode() | |
| } else { | |
| return fmt.Errorf("datastore configuration is missing, please ensure it is correctly provided") |
space/spacecore/storage/storage.go
Outdated
| if mode == SpaceStorageModeBadger { | ||
| // for already existing account repos | ||
| b := badgerstorage.New() | ||
| fmt.Println(b.Name()) |
There was a problem hiding this comment.
I think we shouldn't use fmt.Println
| if chErr != nil { | ||
| if isUniqueConstraint(chErr) { | ||
| continue | ||
| } else { |
There was a problem hiding this comment.
I suppose else is redundant
| CREATE TABLE IF NOT EXISTS trees ( | ||
| id text not null primary key, | ||
| spaceId text not null, | ||
| type int not null default 0, |
There was a problem hiding this comment.
Maybe it should be some kind of enum or constraints?
There was a problem hiding this comment.
Sqlite doesn't have enums. Constraint I didn't add because we don't have some migration mechanism and will not be easy to change this constraint in the future.
| aclId text not null, | ||
| hash text not null default '', | ||
| oldHash text not null default '', | ||
| isCreated boolean not null default false, |
There was a problem hiding this comment.
Could you please explain why we need this field? I don't understand, because from my point of view, if space exist in this table and field IsDeleted=false, then it's considered as existing 😅
There was a problem hiding this comment.
I don't know, I've implemented the existing interface :) As @mcrakhman said, it's hard to explain :)
| &ss.oldHash, | ||
| &ss.isDeleted, | ||
| ) | ||
| if err != nil { |
There was a problem hiding this comment.
Could be 1 if statement
if err != nil && errors.Is(err, sql.ErrNoRows)
There was a problem hiding this comment.
Thanks! There was another bug, I ignored an err in case it was not sql.NoRows
|
|
||
| func isUniqueConstraint(err error) bool { | ||
| var sqliteErr sqlite3.Error | ||
| if errors.As(err, &sqliteErr) { |
There was a problem hiding this comment.
Could be 1 if statement
| func (s *storageService) Init(a *app.App) (err error) { | ||
| s.dbPath = a.MustComponent("config").(configGetter).GetSpaceStorePath() | ||
| s.lockedSpaces = map[string]*lockSpace{} | ||
| return | ||
| } |
There was a problem hiding this comment.
Ensure proper error handling during initialization.
Consider adding error handling for the initialization process to ensure that any issues with setting the database path or initializing the locked spaces map are properly managed. This could involve adding error checks and potentially logging or handling the errors appropriately.
| if _, err = tx.Stmt(s.stmt.createSpace).Exec( | ||
| // space(id, header, settingsId, aclId) | ||
| payload.SpaceHeaderWithId.Id, | ||
| payload.SpaceHeaderWithId.RawHeader, | ||
| payload.SpaceSettingsWithId.Id, | ||
| payload.AclWithId.Id, | ||
| ); err != nil { | ||
| _ = tx.Rollback() | ||
| if isUniqueConstraint(err) { | ||
| return nil, spacestorage.ErrSpaceStorageExists | ||
| } | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Optimize error handling in createSpaceStorage.
Consolidate the repeated tx.Rollback() calls into a deferred function to ensure that rollback is attempted in all error scenarios without repeating the code. This will make the code cleaner and less error-prone.
- if _, err = tx.Stmt(s.stmt.createSpace).Exec(
+ defer func() {
+ if err != nil {
+ _ = tx.Rollback()
+ }
+ }()
+ if _, err = tx.Stmt(s.stmt.createSpace).Exec(Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if _, err = tx.Stmt(s.stmt.createSpace).Exec( | |
| // space(id, header, settingsId, aclId) | |
| payload.SpaceHeaderWithId.Id, | |
| payload.SpaceHeaderWithId.RawHeader, | |
| payload.SpaceSettingsWithId.Id, | |
| payload.AclWithId.Id, | |
| ); err != nil { | |
| _ = tx.Rollback() | |
| if isUniqueConstraint(err) { | |
| return nil, spacestorage.ErrSpaceStorageExists | |
| } | |
| return nil, err | |
| } | |
| defer func() { | |
| if err != nil { | |
| _ = tx.Rollback() | |
| } | |
| }() | |
| if _, err = tx.Stmt(s.stmt.createSpace).Exec( | |
| // space(id, header, settingsId, aclId) | |
| payload.SpaceHeaderWithId.Id, | |
| payload.SpaceHeaderWithId.RawHeader, | |
| payload.SpaceSettingsWithId.Id, | |
| payload.AclWithId.Id, | |
| ); err != nil { | |
| if isUniqueConstraint(err) { | |
| return nil, spacestorage.ErrSpaceStorageExists | |
| } | |
| return nil, err | |
| } |
| if _, err = tx.Stmt(s.stmt.createTree).Exec( | ||
| // settings tree (id, spaceId, heads, type) | ||
| payload.SpaceSettingsWithId.Id, | ||
| payload.SpaceHeaderWithId.Id, | ||
| treestorage.CreateHeadsPayload([]string{payload.SpaceSettingsWithId.Id}), | ||
| treeTypeSettings, | ||
| ); err != nil { | ||
| _ = tx.Rollback() | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Ensure rollback is checked for errors in createSpaceStorage.
The error return value of tx.Rollback is not checked after a failed execution of createTree. This could lead to unnoticed failures in rolling back the transaction.
- _ = tx.Rollback()
+ if err := tx.Rollback(); err != nil {
+ return fmt.Errorf("failed to rollback: %w", err)
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if _, err = tx.Stmt(s.stmt.createTree).Exec( | |
| // settings tree (id, spaceId, heads, type) | |
| payload.SpaceSettingsWithId.Id, | |
| payload.SpaceHeaderWithId.Id, | |
| treestorage.CreateHeadsPayload([]string{payload.SpaceSettingsWithId.Id}), | |
| treeTypeSettings, | |
| ); err != nil { | |
| _ = tx.Rollback() | |
| return nil, err | |
| } | |
| if _, err = tx.Stmt(s.stmt.createTree).Exec( | |
| // settings tree (id, spaceId, heads, type) | |
| payload.SpaceSettingsWithId.Id, | |
| payload.SpaceHeaderWithId.Id, | |
| treestorage.CreateHeadsPayload([]string{payload.SpaceSettingsWithId.Id}), | |
| treeTypeSettings, | |
| ); err != nil { | |
| if err := tx.Rollback(); err != nil { | |
| return fmt.Errorf("failed to rollback: %w", err) | |
| } | |
| return nil, err | |
| } |
| func (s *storageService) Init(a *app.App) (err error) { | ||
| s.dbPath = a.MustComponent("config").(configGetter).GetSpaceStorePath() | ||
| s.lockedSpaces = map[string]*lockSpace{} | ||
| if s.checkpointAfterWrite == 0 { | ||
| s.checkpointAfterWrite = time.Second | ||
| } | ||
| if s.checkpointForce == 0 { | ||
| s.checkpointForce = time.Minute | ||
| } | ||
| return |
There was a problem hiding this comment.
Initialization method lacks robust error handling.
Consider enhancing the error handling in the Init method to manage potential issues during the initialization phase, such as problems with setting the database path or initializing the locked spaces map. This could involve adding error checks and potentially logging or handling the errors appropriately.
There was a problem hiding this comment.
it's okay because in case of err on Init or Run Close will be called
There was a problem hiding this comment.
@cheggaaa, thank you for clarifying the error handling strategy involving the Close method. It's good to know that there's a mechanism in place to manage any issues that might arise during the initialization or runtime phases.
Just to ensure robustness, it might be beneficial to double-check that the Close method comprehensively addresses all potential error conditions and properly cleans up resources. This is crucial especially in scenarios where initialization fails partway, to prevent any resource leaks or inconsistent states.
If you need further assistance in reviewing the Close method or any other part of the implementation, feel free to ask!
| func (s *storageService) Run(ctx context.Context) (err error) { | ||
| driverName := bson.NewObjectId().Hex() | ||
| sql.Register(driverName, | ||
| &sqlite3.SQLiteDriver{ | ||
| ConnectHook: func(conn *sqlite3.SQLiteConn) error { | ||
| conn.RegisterUpdateHook(func(op int, db string, table string, rowid int64) { | ||
| s.lastWrite.Store(time.Now()) | ||
| }) | ||
| return nil | ||
| }, | ||
| }) | ||
|
|
||
| connectionUrlParams := make(url.Values) | ||
| connectionUrlParams.Add("_txlock", "immediate") | ||
| connectionUrlParams.Add("_journal_mode", "WAL") | ||
| connectionUrlParams.Add("_busy_timeout", "5000") | ||
| connectionUrlParams.Add("_synchronous", "NORMAL") | ||
| connectionUrlParams.Add("_cache_size", "10000000") | ||
| connectionUrlParams.Add("_foreign_keys", "true") | ||
| connectionUri := s.dbPath + "?" + connectionUrlParams.Encode() | ||
| if s.writeDb, err = sql.Open(driverName, connectionUri); err != nil { | ||
| return | ||
| } | ||
| s.writeDb.SetMaxOpenConns(1) | ||
| if _, err = s.writeDb.Exec(sqlCreateTables); err != nil { | ||
| return | ||
| } | ||
|
|
||
| if s.readDb, err = sql.Open(driverName, connectionUri); err != nil { | ||
| return | ||
| } | ||
| s.readDb.SetMaxOpenConns(10) | ||
|
|
||
| if err = initStmts(s); err != nil { | ||
| return | ||
| } | ||
| s.ctx, s.ctxCancel = context.WithCancel(context.Background()) | ||
| go s.checkpointLoop() | ||
| return | ||
| } |
There was a problem hiding this comment.
Tip
Codebase Verification
The review of the Run method in the storageService class reveals that while database connections are established and checked for errors, there is a potential issue with resource management. Specifically, the method does not use defer to ensure that resources are cleaned up in case of initialization failures. This could lead to resource leaks if an error occurs after some resources have been allocated but before the method completes successfully.
The database connections (writeDb and readDb) are established using sql.Open and are immediately checked for errors, which aligns with the review comment's requirement to verify that the connections are correctly established. However, the preparation of SQL statements and their error handling could not be verified from the provided script output and would require further investigation or access to additional parts of the code.
Given these observations, it is recommended to introduce defer statements for proper resource cleanup and to further verify the preparation of SQL statements.
Analysis chain
Review the setup and execution of the storage service.
Ensure that resources are properly managed and errors are handled appropriately in the Run method. Consider using defer to clean up resources in case of initialization failures. Additionally, verify that the database connections are correctly established and the SQL statements are prepared without errors.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that the database connections are correctly established and the SQL statements are prepared without errors.
rg --type go "sql.Open"
Length of output: 262
…t into GO-3153-sqlite-spacestore # Conflicts: # core/filestorage/filesync/filesync.go
| func (s *storageService) Init(a *app.App) (err error) { | ||
| s.dbPath = a.MustComponent("config").(configGetter).GetSpaceStorePath() | ||
| s.lockedSpaces = map[string]*lockSpace{} | ||
| if s.checkpointAfterWrite == 0 { | ||
| s.checkpointAfterWrite = time.Second | ||
| } | ||
| if s.checkpointForce == 0 { | ||
| s.checkpointForce = time.Minute | ||
| } | ||
| return |
There was a problem hiding this comment.
Initialization lacks robust error handling for database path retrieval.
- s.dbPath = a.MustComponent("config").(configGetter).GetSpaceStorePath()
+ s.dbPath, ok = a.MustComponent("config").(configGetter).GetSpaceStorePath()
+ if !ok {
+ return fmt.Errorf("failed to get space store path")
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| func (s *storageService) Init(a *app.App) (err error) { | |
| s.dbPath = a.MustComponent("config").(configGetter).GetSpaceStorePath() | |
| s.lockedSpaces = map[string]*lockSpace{} | |
| if s.checkpointAfterWrite == 0 { | |
| s.checkpointAfterWrite = time.Second | |
| } | |
| if s.checkpointForce == 0 { | |
| s.checkpointForce = time.Minute | |
| } | |
| return | |
| func (s *storageService) Init(a *app.App) (err error) { | |
| s.dbPath, ok = a.MustComponent("config").(configGetter).GetSpaceStorePath() | |
| if !ok { | |
| return fmt.Errorf("failed to get space store path") | |
| } | |
| s.lockedSpaces = map[string]*lockSpace{} | |
| if s.checkpointAfterWrite == 0 { | |
| s.checkpointAfterWrite = time.Second | |
| } | |
| if s.checkpointForce == 0 { | |
| s.checkpointForce = time.Minute | |
| } | |
| return |
| func (s *storageService) Run(ctx context.Context) (err error) { | ||
| driverName := bson.NewObjectId().Hex() | ||
| sql.Register(driverName, | ||
| &sqlite3.SQLiteDriver{ | ||
| ConnectHook: func(conn *sqlite3.SQLiteConn) error { | ||
| conn.RegisterUpdateHook(func(op int, db string, table string, rowid int64) { | ||
| s.lastWrite.Store(time.Now()) | ||
| }) | ||
| return nil | ||
| }, | ||
| }) | ||
|
|
||
| connectionUrlParams := make(url.Values) | ||
| connectionUrlParams.Add("_txlock", "immediate") | ||
| connectionUrlParams.Add("_journal_mode", "WAL") | ||
| connectionUrlParams.Add("_busy_timeout", "5000") | ||
| connectionUrlParams.Add("_synchronous", "NORMAL") | ||
| connectionUrlParams.Add("_cache_size", "10000000") | ||
| connectionUrlParams.Add("_foreign_keys", "true") | ||
| connectionUri := s.dbPath + "?" + connectionUrlParams.Encode() | ||
| if s.writeDb, err = sql.Open(driverName, connectionUri); err != nil { | ||
| log.With(zap.String("db", "spacestore_sqlite"), zap.String("type", "write"), zap.Error(err)).Error("failed to open db") | ||
| return | ||
| } | ||
| s.writeDb.SetMaxOpenConns(1) | ||
| if _, err = s.writeDb.Exec(sqlCreateTables); err != nil { | ||
| log.With(zap.String("db", "spacestore_sqlite"), zap.String("type", "createtable"), zap.Error(err)).Error("failed to open db") | ||
| return | ||
| } | ||
|
|
||
| if s.readDb, err = sql.Open(driverName, connectionUri); err != nil { | ||
| log.With(zap.String("db", "spacestore_sqlite"), zap.String("type", "read"), zap.Error(err)).Error("failed to open db") | ||
| return | ||
| } | ||
| s.readDb.SetMaxOpenConns(10) | ||
|
|
||
| if err = initStmts(s); err != nil { | ||
| log.With(zap.String("db", "spacestore_sqlite"), zap.String("type", "init"), zap.Error(err)).Error("failed to open db") | ||
| return | ||
| } | ||
| s.ctx, s.ctxCancel = context.WithCancel(context.Background()) | ||
| go s.checkpointLoop() | ||
| return | ||
| } |
There was a problem hiding this comment.
Ensure proper resource cleanup in the Run method using defer.
Consider using defer statements to ensure that resources such as database connections are properly cleaned up in case of initialization failures. This is crucial to prevent resource leaks.
| _ = s.stmt.spaceIsCreated.QueryRow(id).Scan(&created) | ||
| return |
There was a problem hiding this comment.
Error handling in IsSpaceCreated needs improvement.
- _ = s.stmt.spaceIsCreated.QueryRow(id).Scan(&created)
+ err = s.stmt.spaceIsCreated.QueryRow(id).Scan(&created)
+ if err != nil {
+ return false
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| _ = s.stmt.spaceIsCreated.QueryRow(id).Scan(&created) | |
| return | |
| err = s.stmt.spaceIsCreated.QueryRow(id).Scan(&created) | |
| if err != nil { | |
| return false | |
| } | |
| return |
| _ = tx.Rollback() | ||
| return err |
There was a problem hiding this comment.
Check rollback error handling in deleteSpace.
- _ = tx.Rollback()
+ if err := tx.Rollback(); err != nil {
+ return fmt.Errorf("failed to rollback: %w", err)
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| _ = tx.Rollback() | |
| return err | |
| if err := tx.Rollback(); err != nil { | |
| return fmt.Errorf("failed to rollback: %w", err) | |
| } | |
| return err |
Description
What type of PR is this? (check all applicable)
Related Tickets & Documents
Mobile & Desktop Screenshots/Recordings
Added tests?
Added to documentation?
[optional] Are there any post-deployment tasks we need to perform?
Summary by CodeRabbit
SpaceStorage()method in favor of more specific storage implementations.