implement transactions: Add Enhanced Authorization and Protocol Improvements #45
Conversation
WalkthroughThis pull request introduces several modifications across various components of the project, focusing on enhancing authorization, socket handling, and database command protocols. Key changes include the removal of specific workflow triggers, the addition of new fields to structs for tracking table actions and enforcing read-only behavior, and updates to the SQL schema. The modifications aim to improve transaction management, clarify command handling, and enhance the overall structure and functionality of the Atlas system. Changes
Possibly related PRs
Poem
✨ Finishing Touches
🪧 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
coderabbitai
bot
changed the title
coderabbitai
bot
added
bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
atlas/socket/handler.go (1)
483-578: RefactorSanitizeBeginmethod for improved readabilityThe new
SanitizeBeginmethod introduces complex parsing logic which could be difficult to maintain. Consider the following improvements:
- Simplify Parsing Logic: Break down the function into smaller helper functions to manage different parts of the parsing process.
- Enhance Error Messages: Provide more descriptive error messages to aid in debugging when parsing fails.
- Add Comments: Include comments explaining the purpose of key code blocks for better understandability.
Apply this diff to refactor the parsing loop:
func (s *Socket) SanitizeBegin(cmd commands.Command) (tables, views, triggers []string, err error) { + // Check if already in a transaction if s.inTransaction { err = errors.New("the transaction is already in progress") return } + // Extract list of objects (tables, views, triggers) extractList := func() (list []string, err error) { // Implementation of extractList... } + // Parse command arguments if t, ok := cmd.SelectNormalizedCommand(1); ok && t == "IMMEDIATE" { if err = cmd.CheckMinLen(3); err != nil { return } + // Loop to handle different object types for { switch t, _ = cmd.SelectNormalizedCommand(2); t { case "TABLE": + // Extract tables // ... case "VIEW": + // Extract views // ... case "TRIGGER": + // Extract triggers // ... default: return } } } return }This refactor improves readability and maintainability by structuring the code more clearly.
atlas/socket/query.go (1)
52-70: Ensure robust error handling inHandlemethodIn the
Handlemethod, resetting the authorizer withs.authorizer.Reset()is appropriate to clear previous authorization states. However, consider the following:
- Error Handling: When calling
operations.CreateTable(q.query), ensure that any errors returned are properly handled to prevent silent failures.- Handling Other Statements: Currently, only
CREATEstatements are handled. Consider extending support to other statements likeALTER, as indicated by the TODO comment.Consider updating the error handling logic:
if first == "CREATE" { q.tables, err = operations.CreateTable(q.query) if err != nil { - return nil + return makeFatal(err) } }This ensures that serious errors are not overlooked and are handled appropriately.
atlas/socket/prepare_test.go (2)
Line range hint
28-42: Consider adding more test cases.The test cases cover basic scenarios, but consider adding more edge cases such as:
- Empty command string
- Invalid statement name (e.g., containing special characters)
- Multiple spaces between arguments
- Complex SQL queries with subqueries, joins, etc.
Line range hint
44-77: Enhance error scenario coverage.Consider adding test cases for:
- Invalid SQL syntax
- Database connection errors
- Resource cleanup after errors
- Statement finalization errors
docs/reference/commands.md (1)
189-189: Fix grammar in command description.Change "Rollback" to "Rolls back" for consistency with other command descriptions.
-Rollback the current transaction. +Rolls back the current transaction.🧰 Tools
🪛 LanguageTool
[grammar] ~189-~189: This sentence should probably be started with a verb instead of the noun ‘Rollback’. If not, consider inserting a comma for better clarity.
Context: ...BACK**ROLLBACK\r\nRollback the current transaction. Returns: O...(SENT_START_NN_DT)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/go.yml(0 hunks)atlas/authorization.go(3 hunks)atlas/consensus/server.go(3 hunks)atlas/pool.go(0 hunks)atlas/socket/handler.go(4 hunks)atlas/socket/prepare_test.go(1 hunks)atlas/socket/query.go(1 hunks)docs/reference/commands.md(1 hunks)
💤 Files with no reviewable changes (2)
- .github/workflows/go.yml
- atlas/pool.go
🧰 Additional context used
🪛 GitHub Check: lint
atlas/socket/handler.go
[failure] 261-261:
Error return value of s.sql.SetAuthorizer is not checked (errcheck)
🪛 GitHub Actions: Go
atlas/socket/handler.go
[error] 261-261: Error return value of s.sql.SetAuthorizer is not checked
🪛 LanguageTool
docs/reference/commands.md
[grammar] ~189-~189: This sentence should probably be started with a verb instead of the noun ‘Rollback’. If not, consider inserting a comma for better clarity.
Context: ...BACK** ROLLBACK\r\n Rollback the current transaction. Returns: O...
(SENT_START_NN_DT)
🪛 Markdownlint (0.37.0)
docs/reference/commands.md
128-128: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
136-136: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
144-144: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
152-152: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
161-161: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
169-169: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
178-178: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
186-186: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
194-194: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
202-202: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
208-208: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
215-215: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
225-225: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
232-232: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
240-240: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (13)
atlas/socket/handler.go (1)
164-166: Correct handling of zero timeout insetTimeoutmethodThe logic for setting an infinite timeout when
t == 0by callings.conn.SetDeadline(time.Time{})is appropriate. This ensures that no timeout is applied when a zero duration is specified.atlas/socket/query.go (2)
31-33: Addition oftablesfield toQuerystructIntroducing the
tables []*consensus.Tablefield allows theQuerystruct to store references to tables involved in the query. This enhancement is beneficial for managing table metadata and transaction contexts.
36-48: Consistent use of named return values inParseQueryChanging the function signature to use named return values improves clarity. Initializing the
queryvariable before returning enhances readability and aligns with Go best practices.atlas/authorization.go (3)
31-33: Addition ofLastTablesfield toAuthorizerstructThe introduction of
LastTables map[string]struct{}provides a mechanism to track the tables involved in database actions. This is a valuable enhancement for auditing and enforcing fine-grained access control.
79-79: InitializeLastTablesinResetmethodProperly initializing the
LastTablesmap in theResetmethod ensures that table tracking starts afresh with each new authorization context. This prevents residual data from previous authorizations affecting current operations.
97-101: Capture accessed tables inAuthorizemethodBy recording tables into
LastTablesduring authorization, you enhance the ability to perform detailed auditing and to enforce security policies based on table access patterns.atlas/socket/prepare_test.go (1)
80-137: Well-structured and comprehensive test cases!The test cases effectively validate the
SanitizeBeginmethod by covering:
- Invalid command structures
- Valid commands with multiple tables
- Valid commands with views
- Invalid command lengths
atlas/consensus/server.go (2)
Line range hint
391-402: LGTM! Function visibility change is appropriate.The function
ConstructCurrentNodeis now exported, which is appropriate as it's used across multiple packages. The implementation remains unchanged and follows good practices by:
- Using consistent field initialization
- Properly handling all required node attributes
- Following the single responsibility principle
518-518: Consistent usage of the exported function.The function is consistently used in
WriteMigrationRequestandGossipMigrationstructures.Also applies to: 638-638
docs/reference/commands.md (4)
125-126: LGTM! Clear return value specifications.The return values for commands are now explicitly defined, improving clarity for implementers.
Also applies to: 133-134, 141-142, 149-150, 157-158
159-176: Well-documented transaction commands.The
BEGINandBEGIN IMMEDIATEcommands are clearly documented with their return values and usage.🧰 Tools
🪛 Markdownlint (0.37.0)
161-161: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
169-169: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
214-222: Clear documentation of principle command.The principle command documentation effectively explains:
- The command syntax
- Its purpose for row-level security
- Usage restrictions within transactions
🧰 Tools
🪛 Markdownlint (0.37.0)
215-215: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
224-231: LGTM! Clear documentation of stream management commands.The
SCROLL,RESET, andCLEAR BINDINGScommands are well-documented with their:
- Purpose
- Syntax
- Return values
Also applies to: 232-245
🧰 Tools
🪛 Markdownlint (0.37.0)
225-225: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
withinboredom
force-pushed
the
withinboredom/bot-86-create-operations-layer
branch
from
da040f4 to
4ea35df
Compare
withinboredom
force-pushed
the
withinboredom/bot-86-create-operations-layer-2
branch
from
4214d5b to
95b7491
Compare
withinboredom
force-pushed
the
withinboredom/bot-86-create-operations-layer-2
branch
from
95b7491 to
a26c356
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
atlas/authorization.go (1)
102-105: Consider using strings.Builder for table name construction.When constructing the table name, using strings.Builder would be more efficient, especially when dealing with multiple concatenations.
Apply this diff to improve string concatenation:
if action.Table() != "" && action.Database() != "" && action.Table() != "sqlite_master" { - name := strings.ToUpper(action.Database()) + "." + strings.ToUpper(action.Table()) + var b strings.Builder + b.WriteString(strings.ToUpper(action.Database())) + b.WriteByte('.') + b.WriteString(strings.ToUpper(action.Table())) + a.LastTables[b.String()] = struct{}{} - a.LastTables[name] = struct{}{} }docs/reference/commands.md (2)
159-192: Add examples for transaction commands.While the documentation is clear, it would be helpful to include examples demonstrating common transaction patterns, especially for
BEGIN IMMEDIATEwhich requires object declarations.Add examples like:
-- Example 1: Read-only transaction BEGIN QUERY SELECT * FROM users; COMMIT -- Example 2: Write transaction with table declaration BEGIN IMMEDIATE WITH TABLE (users, orders) QUERY INSERT INTO users (id, name) VALUES (1, 'John'); QUERY INSERT INTO orders (user_id, amount) VALUES (1, 100); COMMIT -- Example 3: Transaction with rollback BEGIN IMMEDIATE WITH TABLE (users) QUERY INSERT INTO users (id, name) VALUES (1, 'John'); ROLLBACK🧰 Tools
🪛 LanguageTool
[grammar] ~189-~189: This sentence should probably be started with a verb instead of the noun ‘Rollback’. If not, consider inserting a comma for better clarity.
Context: ...BACK**ROLLBACK\r\nRollback the current transaction. Returns: O...(SENT_START_NN_DT)
🪛 Markdownlint (0.37.0)
161-161: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
169-169: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
178-178: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
186-186: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
193-245: Add language specifiers to code blocks.Add language specifiers to fenced code blocks for better syntax highlighting.
Apply this diff to add language specifiers:
- ``` + ```sql SAVEPOINT [name]\r\n ``` - ``` + ```sql RELEASE [name]\r\n ``` - ``` + ```sql PRAGMA [name]=[VALUE]\r\n PRAGMA [name]\r\n ``` - ``` + ```sql PRINCIPLE [principle_name] [id]\r\n ``` - ``` + ```sql SCROLL [StreamID] [Count]\r\n ``` - ``` + ```sql RESET [ID]\r\n ``` - ``` + ```sql CLEARBINDINGS [ID]\r\n ```🧰 Tools
🪛 Markdownlint (0.37.0)
194-194: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
202-202: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
208-208: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
215-215: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
225-225: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
232-232: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
240-240: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.github/workflows/go.yml(0 hunks)atlas/authorization.go(3 hunks)atlas/bootstrap/client.go(1 hunks)atlas/bootstrap/migrations.sql(1 hunks)atlas/consensus/server.go(6 hunks)atlas/ownership.go(1 hunks)atlas/pool.go(0 hunks)atlas/socket/handler.go(5 hunks)atlas/socket/prepare_test.go(1 hunks)atlas/socket/query.go(1 hunks)docs/reference/commands.md(1 hunks)
💤 Files with no reviewable changes (2)
- .github/workflows/go.yml
- atlas/pool.go
🚧 Files skipped from review as they are similar to previous changes (2)
- atlas/consensus/server.go
- atlas/socket/query.go
🧰 Additional context used
📓 Learnings (1)
atlas/bootstrap/migrations.sql (1)
Learnt from: withinboredom
PR: bottledcode/atlas-db#38
File: atlas/bootstrap/migrations.sql:122-122
Timestamp: 2025-01-12T08:49:53.254Z
Learning: In the `tables` table of Atlas-DB, the `group_id` column intentionally omits a foreign key constraint to the `name` column to allow null references for tables that aren't part of any group and to maintain flexibility in the order of operations.
🪛 GitHub Check: lint
atlas/socket/prepare_test.go
[failure] 128-128:
s.SanitizeBegin undefined (type *Socket has no field or method SanitizeBegin) (typecheck)
atlas/socket/handler.go
[failure] 485-485:
undefined: Begin
[failure] 486-486:
undefined: ParseBegin) (typecheck)
[failure] 485-485:
undefined: Begin
[failure] 486-486:
undefined: ParseBegin
🪛 GitHub Actions: PR #45
atlas/socket/handler.go
[error] 485-485: Undefined reference to 'Begin' in socket handler
🪛 LanguageTool
docs/reference/commands.md
[grammar] ~189-~189: This sentence should probably be started with a verb instead of the noun ‘Rollback’. If not, consider inserting a comma for better clarity.
Context: ...BACK** ROLLBACK\r\n Rollback the current transaction. Returns: O...
(SENT_START_NN_DT)
🪛 Markdownlint (0.37.0)
docs/reference/commands.md
128-128: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
136-136: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
144-144: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
152-152: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
161-161: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
169-169: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
178-178: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
186-186: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
194-194: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
202-202: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
208-208: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
215-215: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
225-225: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
232-232: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
240-240: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
🔇 Additional comments (10)
atlas/ownership.go (2)
55-65: LGTM! Thread-safe implementation of Hold method.The method correctly handles both new and existing table holds with proper mutex synchronization.
84-89: LGTM! Efficient implementation of IsHeld method.The method correctly uses RLock for read-only operation and provides a clean interface to check table holds.
atlas/bootstrap/client.go (1)
360-362: LGTM! Essential quorum manager update.The addition ensures that the node is properly registered with the quorum manager after successful migration, which is critical for cluster management.
atlas/authorization.go (1)
31-34: LGTM! Well-structured authorization enhancements.The new fields provide clear separation of concerns between table tracking and read-only enforcement.
atlas/socket/prepare_test.go (1)
80-137:⚠️ Potential issueTest depends on undefined method
SanitizeBegin.The test is well-structured with comprehensive test cases, but it's trying to use an undefined method
Socket.SanitizeBegin. This needs to be addressed before the test can work.Let's verify if the method exists in other files:
✅ Verification successful
Confirmed:
SanitizeBeginmethod is not implementedThe test is written for a method that doesn't exist yet in the codebase. This appears to be following a Test-Driven Development (TDD) approach, but the implementation needs to be added to make the test pass.
- The method should be implemented in
atlas/socket/prepare.gooratlas/socket/handler.goto match the test's expectations.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for SanitizeBegin method definition ast-grep --pattern 'func (*Socket) SanitizeBegin'Length of output: 51
Script:
#!/bin/bash # Broader search for SanitizeBegin using ripgrep echo "=== Searching for SanitizeBegin in all files ===" rg -l "SanitizeBegin" echo -e "\n=== Checking recent changes in prepare.go ===" fd "prepare.go" --exec cat {} echo -e "\n=== Checking Socket type definition ===" rg "type Socket struct" -A 5Length of output: 2138
🧰 Tools
🪛 GitHub Check: lint
[failure] 128-128:
s.SanitizeBegin undefined (type *Socket has no field or method SanitizeBegin) (typecheck)atlas/bootstrap/migrations.sql (1)
79-79: LGTM!The insertion of the
ATLAS.NODEStable with global replication and no regional restrictions is appropriate for a system table.docs/reference/commands.md (1)
125-126: LGTM!The return value specifications are clear, concise, and consistent with the implementation.
Also applies to: 133-134, 141-142, 149-150, 157-158
atlas/socket/handler.go (3)
51-51: LGTM!The
authorizerfield is appropriately added to theSocketstruct to support authorization functionality.
164-166: LGTM!The zero timeout handling is correctly implemented to disable the connection deadline.
254-262: Handle error return value when unsetting the authorizer.The deferred call to
s.sql.SetAuthorizer(nil)ignores the error returned by the method.Apply this diff to handle the error:
- defer s.sql.SetAuthorizer(nil) + defer func() { + if err := s.sql.SetAuthorizer(nil); err != nil { + atlas.Logger.Error("Error unsetting authorizer", zap.Error(err)) + } + }()
| func (t *TableOwnerships) Release(table string) { | ||
| t.mu.Lock() | ||
| defer t.mu.Unlock() | ||
|
|
||
| if c, ok := t.holds[table]; ok { | ||
| if c == 1 { | ||
| delete(t.holds, table) | ||
| } else { | ||
| t.holds[table] = c - 1 | ||
| } | ||
| } | ||
|
|
||
| if c, ok := t.holds[table]; ok && c <= 0 { | ||
| delete(t.holds, table) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant delete operation and add count validation.
The Release method has two issues:
- The second delete operation (lines 79-81) is redundant as the table entry is already deleted if count equals 1.
- The count can potentially become negative without proper validation.
Apply this diff to fix both issues:
func (t *TableOwnerships) Release(table string) {
t.mu.Lock()
defer t.mu.Unlock()
if c, ok := t.holds[table]; ok {
+ if c <= 0 {
+ delete(t.holds, table)
+ return
+ }
if c == 1 {
delete(t.holds, table)
} else {
t.holds[table] = c - 1
}
}
-
- if c, ok := t.holds[table]; ok && c <= 0 {
- delete(t.holds, table)
- }
}📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (t *TableOwnerships) Release(table string) { | |
| t.mu.Lock() | |
| defer t.mu.Unlock() | |
| if c, ok := t.holds[table]; ok { | |
| if c == 1 { | |
| delete(t.holds, table) | |
| } else { | |
| t.holds[table] = c - 1 | |
| } | |
| } | |
| if c, ok := t.holds[table]; ok && c <= 0 { | |
| delete(t.holds, table) | |
| } | |
| } | |
| func (t *TableOwnerships) Release(table string) { | |
| t.mu.Lock() | |
| defer t.mu.Unlock() | |
| if c, ok := t.holds[table]; ok { | |
| if c <= 0 { | |
| delete(t.holds, table) | |
| return | |
| } | |
| if c == 1 { | |
| delete(t.holds, table) | |
| } else { | |
| t.holds[table] = c - 1 | |
| } | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
atlas/socket/begin.go (2)
27-32: Add documentation for the Begin struct and its fields.Consider adding GoDoc comments to document:
- The purpose of the Begin struct
- The significance of each field
- Any invariants or constraints that must be maintained
Example:
+// Begin represents a database transaction configuration. +// It tracks the transaction's read-only status and affected database objects. type Begin struct { + // isReadonly indicates whether the transaction is read-only isReadonly bool + // tables contains the names of tables affected by this transaction tables []string + // views contains the names of views affected by this transaction views []string + // triggers contains the names of triggers affected by this transaction triggers []string }
88-127: Enhance error handling and validation.The error messages could be more descriptive, and additional validation is needed.
- Add context to error messages
- Validate for empty or whitespace-only names
- Consider using fmt.Errorf for formatted messages
- err = errors.New("expected table name in parentheses") + err = fmt.Errorf("expected table name in parentheses at position %d", n) + if strings.TrimSpace(first) == "" { + err = fmt.Errorf("empty or whitespace-only name at position %d", n) + return + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
atlas/socket/begin.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
atlas/socket/begin.go
1-1: : # github.com/bottledcode/atlas-db/atlas/socket [github.com/bottledcode/atlas-db/atlas/socket.test]
atlas/socket/prepare_test.go:128:30: s.SanitizeBegin undefined (type *Socket has no field or method SanitizeBegin)
(typecheck)
🔇 Additional comments (2)
atlas/socket/begin.go (2)
1-25: LGTM! Well-structured package setup.The license header is properly documented, and imports are appropriately organized.
🧰 Tools
🪛 golangci-lint (1.62.2)
1-1: : # github.com/bottledcode/atlas-db/atlas/socket [github.com/bottledcode/atlas-db/atlas/socket.test]
atlas/socket/prepare_test.go:128:30: s.SanitizeBegin undefined (type *Socket has no field or method SanitizeBegin)(typecheck)
129-146: Add validation for duplicate object names.The function should validate that there are no duplicate entries in tables, views, or triggers arrays.
Consider adding validation:
func ParseBegin(cmd commands.Command) (*Begin, error) { if t, ok := cmd.SelectNormalizedCommand(1); ok && t == "IMMEDIATE" { tables, views, triggers, err := sanitizeBegin(cmd) if err != nil { return nil, err } + + if err := validateNoDuplicates(tables, views, triggers); err != nil { + return nil, err + } return &Begin{ isReadonly: false, tables: tables, views: views, triggers: triggers, }, nil } return &Begin{ isReadonly: true, }, nil } +func validateNoDuplicates(tables, views, triggers []string) error { + seen := make(map[string]struct{}) + for _, name := range tables { + if _, exists := seen[name]; exists { + return fmt.Errorf("duplicate table name: %s", name) + } + seen[name] = struct{}{} + } + // Similar checks for views and triggers + return nil +}
| func (b *Begin) Handle(s *Socket) error { | ||
| s.authorizer.Reset() | ||
| s.authorizer.ForceReadonly = b.isReadonly | ||
|
|
||
| // todo: capture tables, views, and triggers | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
Implement TODO and add error handling for authorization.
The Handle method needs completion and improved error handling:
- The TODO comment indicates incomplete implementation for capturing tables, views, and triggers
- No error handling for potential authorization failures
Would you like me to help implement the table/view/trigger capture logic and add proper error handling? Here's a suggested approach:
func (b *Begin) Handle(s *Socket) error {
+ if err := s.authorizer.Reset(); err != nil {
+ return fmt.Errorf("failed to reset authorizer: %w", err)
+ }
s.authorizer.ForceReadonly = b.isReadonly
- // todo: capture tables, views, and triggers
+ if !b.isReadonly {
+ if err := s.captureObjects(b.tables, b.views, b.triggers); err != nil {
+ return fmt.Errorf("failed to capture objects: %w", err)
+ }
+ }
return nil
}Committable suggestion skipped: line range outside the PR's diff.
| func sanitizeBegin(cmd commands.Command) (tables, views, triggers []string, err error) { | ||
| extractList := func() (list []string, err error) { | ||
| var rip []string | ||
| n := 4 | ||
| expectingFirst := true | ||
| expectingLast := true | ||
| for { | ||
| first, _ := cmd.SelectNormalizedCommand(n) | ||
| rip = append(rip, first) | ||
| n++ | ||
| isLast := false | ||
| first = strings.TrimSuffix(first, ",") | ||
| if first == "" { | ||
| break | ||
| } | ||
| if first == "(" { | ||
| expectingFirst = false | ||
| continue | ||
| } | ||
| if first == ")" { | ||
| expectingLast = false | ||
| break | ||
| } | ||
| if strings.HasPrefix(first, "(") { | ||
| first = strings.TrimPrefix(first, "(") | ||
| expectingFirst = false | ||
| } | ||
| if strings.HasSuffix(first, ")") { | ||
| expectingLast = false | ||
| first = strings.TrimSuffix(first, ")") | ||
| isLast = true | ||
| } | ||
| if expectingFirst { | ||
| err = errors.New("expected table name in parentheses") | ||
| return | ||
| } | ||
|
|
||
| if first == "," { | ||
| continue | ||
| } | ||
|
|
||
| list = append(list, cmd.NormalizeName(first)) | ||
| if isLast { | ||
| break | ||
| } | ||
| } | ||
| if expectingFirst || expectingLast { | ||
| err = errors.New("expected table name in parentheses") | ||
| return | ||
| } | ||
| cmd = cmd.ReplaceCommand(strings.Join(rip, " "), "BEGIN IMMEDIATE WITH") | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor extractList for better maintainability and clarity.
The extractList function is complex and handles multiple responsibilities. Consider breaking it down into smaller, focused functions.
- Extract the parentheses validation logic
- Separate the list parsing logic
- Define constants for magic numbers
- Add input validation
Example refactoring:
+const (
+ cmdPositionOffset = 4
+)
+
+func validateParentheses(item string) (isFirst, isLast bool, content string, err error) {
+ if strings.HasPrefix(item, "(") {
+ isFirst = true
+ content = strings.TrimPrefix(item, "(")
+ }
+ if strings.HasSuffix(item, ")") {
+ isLast = true
+ content = strings.TrimSuffix(content, ")")
+ }
+ return
+}Committable suggestion skipped: line range outside the PR's diff.
| triggers []string | ||
| } | ||
|
|
||
| func sanitizeBegin(cmd commands.Command) (tables, views, triggers []string, err error) { |
There was a problem hiding this comment.
💡 Codebase verification
Update TestSanitizeBegin to use package function instead of struct method
The test is trying to call SanitizeBegin as a method on the Socket struct, but it has been refactored to a package-level function named sanitizeBegin. Update the test to call sanitizeBegin(tt.cmd) directly instead of s.SanitizeBegin(tt.cmd).
- Location:
atlas/socket/prepare_test.go
🔗 Analysis chain
Fix failing test in prepare_test.go.
The static analysis indicates that prepare_test.go is trying to use an undefined method SanitizeBegin. This test needs to be updated to use the new sanitizeBegin function.
Run this script to locate all test references to the old method:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all test files referencing the old SanitizeBegin method
rg -l "SanitizeBegin" --type go --type-add "go:*.go" --glob "*_test.go"
Length of output: 100
Script:
#!/bin/bash
# Show the content around SanitizeBegin usage in the test file
rg "SanitizeBegin" atlas/socket/prepare_test.go -B 2 -A 2
Length of output: 291
withinboredom
force-pushed
the
withinboredom/bot-86-create-operations-layer-2
branch
from
a0c8c0f to
360cbf2
Compare
coderabbitai
bot
added
the
dependencies
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
atlas/socket/handler.go (2)
254-262:⚠️ Potential issueHandle error return value when unsetting the authorizer.
The deferred call to
s.sql.SetAuthorizer(nil)at line 261 ignores the error returned by the method. It's important to handle this error to catch any issues that may occur when unsetting the authorizer.Apply this diff to handle the error:
- defer s.sql.SetAuthorizer(nil) + defer func() { + if err := s.sql.SetAuthorizer(nil); err != nil { + atlas.Logger.Error("Error unsetting authorizer", zap.Error(err)) + } + }()
485-571:⚠️ Potential issueMissing Begin type and ParseBegin function definitions.
The codebase follows a consistent pattern where each command type has:
- A type definition (e.g.,
Query,Prepare,Scroll)- A corresponding Parse function (e.g.,
ParseQuery,ParsePrepare,ParseScroll)However, the
Begintype andParseBeginfunction are missing despite being referenced inPerformBegin. These need to be implemented following the same pattern as other commands.Run the following script to verify the types:
#!/bin/bash # Description: Check for Begin and ParseBegin type definitions. # Test: Search for Begin type definition ast-grep --pattern $'type Begin struct { $$$ }' # Test: Search for ParseBegin function definition ast-grep --pattern $'func ParseBegin($$$) ($$$) { $$$ }'
🧹 Nitpick comments (8)
atlas/socket/server/steal.go (3)
39-41: Validate proper usage of context value keys.The
AttachPrincipalfunction follows a recognized pattern for storing context-specific data, but consider providing a unique (exported or unexported) package-levelcontextKeyrather than embedding the struct literal directly. Doing so helps prevent collisions in complex projects.-type principalKey struct { - Name string -} +type principalKey string ... func AttachPrincipal(ctx context.Context, name, value string) context.Context { - return context.WithValue(ctx, principalKey{Name: name}, principalValue{Value: value}) + return context.WithValue(ctx, principalKey(name), principalValue{Value: value}) }
55-62: Check for partial shard principal coverage.When determining if all shard principals are present, consider partial principal coverage where some but not all are found in the context. The code currently fails on the first missing principal. If multiple principals are missing, the error message might be incomplete (only references the first encountered). For better debugging, you might accumulate missing principal names and return a more informative error message.
101-136: Enhance naming to reflect migration logic side effects.
stealTablehandles more than just stealing ownership—it also applies schema/data migrations. Consider splitting these responsibilities into distinct functions or renaming it to better reflect the combined ownership/migration management. This will improve maintainability and clarity.atlas/consensus/majority-quorum.go (1)
34-34: Initialize atomic field to a known state.While
atomic.Int64starts at zero by default, explicitly documenting or initializing the field (e.g., in a constructor) may clarify the initial migration version.atlas/authorization.go (1)
98-105: Consider adding error messages for denied operations.When denying write operations in read-only mode, it would be helpful to return a descriptive error message to help users understand why their operation was denied.
if a.ForceReadonly && a.isWrite(action) { - return sqlite.AuthResultDeny + Logger.Info("Write operation denied: read-only mode is enabled", + zap.String("table", action.Table()), + zap.String("operation", action.Type().String())) + return sqlite.AuthResultDeny }docs/reference/commands.md (3)
159-176: Consider clarifying transaction isolation levels.While the
BEGINandBEGIN IMMEDIATEcommands are well-documented, it would be helpful to specify the transaction isolation level provided (e.g., SERIALIZABLE, READ COMMITTED).🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
161-161: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
169-169: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
189-189: Fix grammar in command description.Change "Rollback the current transaction" to "Rolls back the current transaction" for consistency with other command descriptions.
🧰 Tools
🪛 LanguageTool
[grammar] ~189-~189: This sentence should probably be started with a verb instead of the noun ‘Rollback’. If not, consider inserting a comma for better clarity.
Context: ...BACK**ROLLBACK\r\nRollback the current transaction. Returns: O...(SENT_START_NN_DT)
161-241: Add language identifiers to code blocks.Add appropriate language identifiers to the fenced code blocks to enable syntax highlighting:
-``` +```text🧰 Tools
🪛 LanguageTool
[grammar] ~189-~189: This sentence should probably be started with a verb instead of the noun ‘Rollback’. If not, consider inserting a comma for better clarity.
Context: ...BACK**ROLLBACK\r\nRollback the current transaction. Returns: O...(SENT_START_NN_DT)
🪛 markdownlint-cli2 (0.17.2)
161-161: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
169-169: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
178-178: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
186-186: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
194-194: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
202-202: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
208-208: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
215-215: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
225-225: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
232-232: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
240-240: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.github/workflows/go.yml(0 hunks)atlas/authorization.go(3 hunks)atlas/bootstrap/client.go(1 hunks)atlas/bootstrap/migrations.sql(1 hunks)atlas/consensus/majority-quorum.go(1 hunks)atlas/consensus/quorum.go(1 hunks)atlas/consensus/server.go(6 hunks)atlas/ownership.go(1 hunks)atlas/pool.go(0 hunks)atlas/socket/begin.go(1 hunks)atlas/socket/handler.go(5 hunks)atlas/socket/prepare_test.go(1 hunks)atlas/socket/query.go(1 hunks)atlas/socket/server/steal.go(1 hunks)docs/reference/commands.md(1 hunks)docs/reference/protocol.md(0 hunks)docs/reference/queries.md(1 hunks)
💤 Files with no reviewable changes (3)
- .github/workflows/go.yml
- docs/reference/protocol.md
- atlas/pool.go
🚧 Files skipped from review as they are similar to previous changes (4)
- atlas/bootstrap/client.go
- atlas/socket/query.go
- atlas/socket/begin.go
- atlas/ownership.go
🧰 Additional context used
📓 Learnings (1)
atlas/bootstrap/migrations.sql (1)
Learnt from: withinboredom
PR: bottledcode/atlas-db#38
File: atlas/bootstrap/migrations.sql:122-122
Timestamp: 2025-01-12T08:49:53.254Z
Learning: In the `tables` table of Atlas-DB, the `group_id` column intentionally omits a foreign key constraint to the `name` column to allow null references for tables that aren't part of any group and to maintain flexibility in the order of operations.
🪛 LanguageTool
docs/reference/queries.md
[style] ~20-~20: Consider placing the discourse marker ‘first’ at the beginning of the sentence for more clarity.
Context: ... specific conditions require otherwise. The system first determines the leader of the table bein...
(SENT_START_FIRST_PREMIUM)
docs/reference/commands.md
[grammar] ~189-~189: This sentence should probably be started with a verb instead of the noun ‘Rollback’. If not, consider inserting a comma for better clarity.
Context: ...BACK** ROLLBACK\r\n Rollback the current transaction. Returns: O...
(SENT_START_NN_DT)
🪛 markdownlint-cli2 (0.17.2)
docs/reference/commands.md
128-128: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
136-136: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
144-144: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
152-152: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
161-161: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
169-169: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
178-178: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
186-186: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
194-194: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
202-202: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
208-208: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
215-215: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
225-225: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
232-232: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
240-240: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
🪛 GitHub Check: lint
atlas/socket/prepare_test.go
[failure] 128-128:
s.SanitizeBegin undefined (type *Socket has no field or method SanitizeBegin) (typecheck)
🪛 GitHub Check: build
atlas/socket/prepare_test.go
[failure] 128-128:
s.SanitizeBegin undefined (type *Socket has no field or method SanitizeBegin)
🪛 GitHub Actions: Go
atlas/socket/prepare_test.go
[error] 128-128: s.SanitizeBegin undefined (type *Socket has no field or method SanitizeBegin)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (11)
atlas/socket/server/steal.go (1)
138-197: Ensure transaction finalization always occurs.While the deferred rollback is present, ensure that commit or rollback logic covers all code paths. If a panic or unexpected error arises before the deferred function runs, your DB connection pool may still handle it safely, but confirming consistent finalization is crucial. Testing these edge cases is recommended.
Let me know if you need a test harness or integration test approach to validate this behavior under failure conditions.
atlas/consensus/majority-quorum.go (2)
28-28: Confirm concurrency assumptions for atomic usage.Importing
sync/atomicis appropriate for concurrent use ofnextMigrationVersion. Ensure that no other package-level concurrency manipulates the same state in a non-atomic way (such as direct field assignment).
37-42: Use atomic operations consistently.The
SetNextMigrationVersionandGetNextMigrationVersionmethods properly wrap atomic store/load. Ensure all relevant increments or other numeric operations also occur through these methods rather than direct usage ofnextMigrationVersion.atlas/consensus/quorum.go (1)
76-77: Validate new migration version methods in all quorum implementations.The
Quoruminterface now hasSetNextMigrationVersionandGetNextMigrationVersion. Confirm that all Quorum implementers (beyondmajorityQuorum) satisfy these methods or embed them properly. Otherwise, consumers may encounter runtime errors if alternative Quorum types are used.atlas/authorization.go (3)
31-34: LGTM: Well-structured additions to the Authorizer struct.The new fields serve clear purposes:
LastTablesfor tracking accessed tablesForceReadonlyfor controlling write operations
80-80: LGTM: Proper initialization in Reset method.The Reset method correctly initializes the LastTables map, preventing nil map panics.
102-104: Verify case sensitivity handling in table names.The code concatenates uppercase database and table names. Let's verify that this matches SQLite's case sensitivity behavior.
✅ Verification successful
Case sensitivity handling is implemented correctly
The uppercase conversion is used only for internal tracking in the LastTables map, ensuring consistent lookups. This aligns with SQLite's case-insensitive behavior for identifiers while the actual table operations preserve the original case through proper name normalization.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other instances of table name case handling rg -A 2 "ToUpper.*Table|Table.*ToUpper"Length of output: 232
Script:
#!/bin/bash # Search for table name comparisons and SQLite identifier handling rg -i "table.*name|sqlite.*case|identifier.*case" -A 2 # Search for any documentation about case sensitivity fd README.md --exec cat {} \; | rg -i "case.*sensitive|sqlite.*case" # Search for other places where table names are used in comparisons ast-grep --pattern 'Table() == $_'Length of output: 16057
docs/reference/queries.md (1)
1-93: Excellent documentation of the query execution model.The documentation provides a clear and comprehensive explanation of:
- Query execution model
- Borrowing process
- SQLite WAL mode integration
- Definitions of key concepts
🧰 Tools
🪛 LanguageTool
[style] ~20-~20: Consider placing the discourse marker ‘first’ at the beginning of the sentence for more clarity.
Context: ... specific conditions require otherwise. The system first determines the leader of the table bein...(SENT_START_FIRST_PREMIUM)
atlas/bootstrap/migrations.sql (1)
79-79: Verify table name case consistency.The table name 'ATLAS.NODES' is in uppercase, which aligns with the case handling in authorization.go. This is good for consistency.
atlas/consensus/server.go (1)
Line range hint
394-404: Verify the function visibility change in the codebase.The function logic is correct. However, ensure that all references to
constructCurrentNodehave been updated to match the new exported nameConstructCurrentNode.Run the following script to verify the function usage:
✅ Verification successful
Function visibility change has been properly propagated
All references to the function have been updated to use the new exported name
ConstructCurrentNode. No instances of the old unexported name remain in the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all references to constructCurrentNode have been updated. # Test: Search for any remaining references to the old function name. rg -A 5 $'constructCurrentNode' # Test: Search for references to the new function name. rg -A 5 $'ConstructCurrentNode'Length of output: 1587
atlas/socket/handler.go (1)
164-166: LGTM!The function correctly handles both zero and non-zero timeout values.
| func StealTableByName(ctx context.Context, tableName string) (consensus.Quorum, error) { | ||
| table, err := getTable(ctx, tableName) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if table == nil { | ||
| return nil, fmt.Errorf("table isn't found: %s", tableName) | ||
| } | ||
|
|
||
| // resolve groups | ||
| goUp: | ||
| if table.GetGroup() != "" { | ||
| table, err = getTable(ctx, table.GetGroup()) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| goto goUp | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prefer a loop or recursion to labeled goto.
The label goUp: is used to repeatedly fetch the table’s group. Although functional, using goto can diminish readability. A simple loop or small recursive function may be clearer and less error-prone.
-func StealTableByName(ctx context.Context, tableName string) (consensus.Quorum, error) {
- table, err := getTable(ctx, tableName)
- if err != nil {
- return nil, err
- }
-
- if table == nil {
- return nil, fmt.Errorf("table isn't found: %s", tableName)
- }
-
- // resolve groups
-goUp:
- if table.GetGroup() != "" {
- table, err = getTable(ctx, table.GetGroup())
- if err != nil {
- return nil, err
- }
- goto goUp
- }
-
- return stealTable(ctx, table)
+func StealTableByName(ctx context.Context, tableName string) (consensus.Quorum, error) {
+ table, err := getTable(ctx, tableName)
+ if err != nil {
+ return nil, err
+ }
+ if table == nil {
+ return nil, fmt.Errorf("table isn't found: %s", tableName)
+ }
+
+ for table.GetGroup() != "" {
+ table, err = getTable(ctx, table.GetGroup())
+ if err != nil {
+ return nil, err
+ }
+ }
+
+ return stealTable(ctx, table)
}📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func StealTableByName(ctx context.Context, tableName string) (consensus.Quorum, error) { | |
| table, err := getTable(ctx, tableName) | |
| if err != nil { | |
| return nil, err | |
| } | |
| if table == nil { | |
| return nil, fmt.Errorf("table isn't found: %s", tableName) | |
| } | |
| // resolve groups | |
| goUp: | |
| if table.GetGroup() != "" { | |
| table, err = getTable(ctx, table.GetGroup()) | |
| if err != nil { | |
| return nil, err | |
| } | |
| goto goUp | |
| } | |
| func StealTableByName(ctx context.Context, tableName string) (consensus.Quorum, error) { | |
| table, err := getTable(ctx, tableName) | |
| if err != nil { | |
| return nil, err | |
| } | |
| if table == nil { | |
| return nil, fmt.Errorf("table isn't found: %s", tableName) | |
| } | |
| for table.GetGroup() != "" { | |
| table, err = getTable(ctx, table.GetGroup()) | |
| if err != nil { | |
| return nil, err | |
| } | |
| } | |
| return stealTable(ctx, table) | |
| } |
| func TestSanitizeBegin(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| cmd commands.Command | ||
| want [][]string | ||
| wantErr bool | ||
| }{ | ||
| { | ||
| name: "Transaction in progress", | ||
| cmd: commands.CommandFromString("BEGIN IMMEDIATE TABLE Users"), | ||
| want: nil, | ||
| wantErr: true, | ||
| }, | ||
| { | ||
| name: "Transaction in progress", | ||
| cmd: commands.CommandFromString("BEGIN IMMEDIATE TABLE (Users"), | ||
| want: nil, | ||
| wantErr: true, | ||
| }, | ||
| { | ||
| name: "Valid command with tables", | ||
| cmd: commands.CommandFromString("BEGIN IMMEDIATE TABLE (Users, Orders)"), | ||
| want: [][]string{{"MAIN.USERS", "MAIN.ORDERS"}, nil, nil}, | ||
| wantErr: false, | ||
| }, | ||
| { | ||
| name: "Valid command with tables", | ||
| cmd: commands.CommandFromString("BEGIN IMMEDIATE TABLE ( Users, Orders )"), | ||
| want: [][]string{{"MAIN.USERS", "MAIN.ORDERS"}, nil, nil}, | ||
| wantErr: false, | ||
| }, | ||
| { | ||
| name: "Valid command with tables", | ||
| cmd: commands.CommandFromString("BEGIN IMMEDIATE TABLE ( Users, Orders ) VIEW (STUFF)"), | ||
| want: [][]string{{"MAIN.USERS", "MAIN.ORDERS"}, {"MAIN.STUFF"}, nil}, | ||
| wantErr: false, | ||
| }, | ||
| { | ||
| name: "Invalid command length", | ||
| cmd: commands.CommandFromString("BEGIN IMMEDIATE"), | ||
| want: nil, | ||
| wantErr: true, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| s := &Socket{} | ||
| got, got2, got3, err := s.SanitizeBegin(tt.cmd) | ||
| if tt.wantErr { | ||
| assert.Error(t, err) | ||
| } else { | ||
| assert.NoError(t, err) | ||
| assert.Equal(t, tt.want, [][]string{got, got2, got3}) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Test implementation precedes method implementation.
The test TestSanitizeBegin is well-structured with comprehensive test cases, but the SanitizeBegin method is not yet implemented in the Socket struct. This is causing build failures.
Would you like me to help generate the implementation of the SanitizeBegin method based on the test cases?
🧰 Tools
🪛 GitHub Check: lint
[failure] 128-128:
s.SanitizeBegin undefined (type *Socket has no field or method SanitizeBegin) (typecheck)
🪛 GitHub Check: build
[failure] 128-128:
s.SanitizeBegin undefined (type *Socket has no field or method SanitizeBegin)
🪛 GitHub Actions: Go
[error] 128-128: s.SanitizeBegin undefined (type *Socket has no field or method SanitizeBegin)
withinboredom
force-pushed
the
withinboredom/bot-86-create-operations-layer-2
branch
from
360cbf2 to
4f0d0ed
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
atlas/socket/handler.go (1)
485-571:⚠️ Potential issueMissing Begin type and ParseBegin function definitions.
The codebase follows a consistent pattern where each command type has a type definition and a corresponding Parse function. However, the
Begintype andParseBeginfunction are missing.
🧹 Nitpick comments (5)
atlas/socket/server/steal.go (1)
101-197: Consider splitting the migration logic out for better maintainability.
The logic in the stealTable function is extensive and weaves together multiple steps—stealing ownership, opening the connection, beginning a transaction, applying missing migrations, and finalizing the transaction. Consider extracting the migration application into a dedicated function or module for clearer separation of concerns.For example:
func stealTable(ctx context.Context, table *consensus.Table) (consensus.Quorum, error) { ... + // Proposed: applyMigrations could encapsulate the missing migrations logic + if err := applyMigrations(ctx, conn, response.GetSuccess().GetMissingMigrations()); err != nil { + return nil, err + } ... } +func applyMigrations(ctx context.Context, conn *sqlite.Conn, migrations []*consensus.MissingMigration) error { + // handle transaction, data, schema migrations + // finalize or rollback + return nil +}atlas/socket/begin.go (2)
34-127: Break down sanitizeBegin to improve readability.
The sanitizeBegin function handles command validation, parentheses parsing, and extracting lists of tables/views/triggers in one large block. Consider moving the embedded extractList function and parentheses logic into separate helper functions for clearer structure.
152-152: Address the TODO.
You have a TODO comment for capturing tables, views, and triggers. Implementing object capture within this method will be essential to handle state changes properly when not in read-only mode.Would you like help drafting this implementation?
atlas/consensus/server.go (1)
308-320: Consider using case-insensitive prefix check.The table name prefix check might be inconsistent with the new uppercase naming convention. Consider using a case-insensitive comparison.
Apply this diff to make the comparison case-insensitive:
-if !strings.HasPrefix(req.GetMigration().GetVersion().GetTableName(), "ATLAS.") { +if !strings.HasPrefix(strings.ToUpper(req.GetMigration().GetVersion().GetTableName()), "ATLAS.") {docs/reference/commands.md (1)
189-189: Fix grammar in command description.The sentence should start with a verb.
Apply this diff to fix the grammar:
-Rollback the current transaction. +Rolls back the current transaction.🧰 Tools
🪛 LanguageTool
[grammar] ~189-~189: This sentence should probably be started with a verb instead of the noun ‘Rollback’. If not, consider inserting a comma for better clarity.
Context: ...BACK**ROLLBACK\r\nRollback the current transaction. Returns: O...(SENT_START_NN_DT)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.github/workflows/go.yml(0 hunks)atlas/authorization.go(3 hunks)atlas/bootstrap/client.go(1 hunks)atlas/bootstrap/migrations.sql(1 hunks)atlas/consensus/majority-quorum.go(1 hunks)atlas/consensus/quorum.go(1 hunks)atlas/consensus/server.go(6 hunks)atlas/ownership.go(1 hunks)atlas/pool.go(0 hunks)atlas/socket/begin.go(1 hunks)atlas/socket/handler.go(5 hunks)atlas/socket/prepare_test.go(1 hunks)atlas/socket/query.go(1 hunks)atlas/socket/server/steal.go(1 hunks)docs/reference/commands.md(1 hunks)docs/reference/protocol.md(0 hunks)docs/reference/queries.md(1 hunks)
💤 Files with no reviewable changes (3)
- .github/workflows/go.yml
- docs/reference/protocol.md
- atlas/pool.go
🚧 Files skipped from review as they are similar to previous changes (7)
- atlas/bootstrap/client.go
- atlas/consensus/quorum.go
- atlas/authorization.go
- atlas/socket/query.go
- atlas/bootstrap/migrations.sql
- atlas/consensus/majority-quorum.go
- atlas/ownership.go
🧰 Additional context used
🪛 LanguageTool
docs/reference/commands.md
[grammar] ~189-~189: This sentence should probably be started with a verb instead of the noun ‘Rollback’. If not, consider inserting a comma for better clarity.
Context: ...BACK** ROLLBACK\r\n Rollback the current transaction. Returns: O...
(SENT_START_NN_DT)
docs/reference/queries.md
[style] ~20-~20: Consider placing the discourse marker ‘first’ at the beginning of the sentence for more clarity.
Context: ... specific conditions require otherwise. The system first determines the leader of the table bein...
(SENT_START_FIRST_PREMIUM)
🪛 markdownlint-cli2 (0.17.2)
docs/reference/commands.md
128-128: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
136-136: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
144-144: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
152-152: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
161-161: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
169-169: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
178-178: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
186-186: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
194-194: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
202-202: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
208-208: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
215-215: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
225-225: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
232-232: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
240-240: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
🪛 GitHub Check: lint
atlas/socket/prepare_test.go
[failure] 128-128:
s.SanitizeBegin undefined (type *Socket has no field or method SanitizeBegin) (typecheck)
🪛 GitHub Check: build
atlas/socket/prepare_test.go
[failure] 128-128:
s.SanitizeBegin undefined (type *Socket has no field or method SanitizeBegin)
🪛 GitHub Actions: Go
atlas/socket/prepare_test.go
[error] 128-128: s.SanitizeBegin undefined (type *Socket has no field or method SanitizeBegin)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (8)
atlas/socket/server/steal.go (1)
78-99: Avoid using labeled goto for clarity.
Using a labeled goto statement (line 89 with label "goUp") can negatively impact readability. A simple loop or a small recursive function is typically easier to follow and maintain.atlas/socket/prepare_test.go (1)
127-128:⚠️ Potential issueFix undefined method call.
The test calls s.SanitizeBegin, but no such method exists on the Socket struct. Rename the call to use the package-level sanitizeBegin function (or expose a new SanitizeBegin method) to fix the build error.Here’s a possible fix:
- got, got2, got3, err := s.SanitizeBegin(tt.cmd) + got, got2, got3, err := sanitizeBegin(tt.cmd)Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Check: lint
[failure] 128-128:
s.SanitizeBegin undefined (type *Socket has no field or method SanitizeBegin) (typecheck)🪛 GitHub Check: build
[failure] 128-128:
s.SanitizeBegin undefined (type *Socket has no field or method SanitizeBegin)🪛 GitHub Actions: Go
[error] 128-128: s.SanitizeBegin undefined (type *Socket has no field or method SanitizeBegin)
atlas/consensus/server.go (3)
55-58: LGTM! Proper error handling added.The error handling for the "BEGIN IMMEDIATE" SQL command is well-implemented, following best practices by immediately returning any errors.
394-394: LGTM! Function visibility change is appropriate.The function has been made public by renaming from
constructCurrentNodetoConstructCurrentNode, which is appropriate given its usage across multiple components.
37-37: Verify the impact of NodeTable constant change.The constant value has been changed from "atlas.nodes" to "ATLAS.NODES". While this aligns with the uppercase naming convention, it's a breaking change that could affect existing code.
Run the following script to check for any references to the old value:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check for references to the old NodeTable value. # Test: Search for references to "atlas.nodes" rg -i "atlas\.nodes" --type goLength of output: 90
Action: Confirming NodeTable constant impact
The search for "atlas.nodes" returned a single instance in atlas/consensus/server.go—showing the updated value "ATLAS.NODES". There are no lingering references to a lowercase "atlas.nodes" in the codebase. The change aligns with the uppercase naming convention found in migrations.sql, meaning the update is intentional and its impact should be scoped to the affected code paths.
- Verify that all dependent areas (e.g., migration scripts, database interactions) expect the uppercase format.
atlas/socket/handler.go (2)
51-51: LGTM! Authorization field added.The
authorizerfield is appropriately added to theSocketstruct to manage authorization within socket connections.
254-262: Handle error return value when unsetting the authorizer.The deferred call to
s.sql.SetAuthorizer(nil)ignores the error returned by the method.Apply this diff to handle the error:
- defer s.sql.SetAuthorizer(nil) + defer func() { + if err := s.sql.SetAuthorizer(nil); err != nil { + atlas.Logger.Error("Error unsetting authorizer", zap.Error(err)) + } + }()docs/reference/queries.md (1)
1-94: LGTM! Comprehensive and well-structured documentation.The documentation provides clear and thorough explanations of query execution and serializability in AtlasDb, accurately reflecting the implemented features.
🧰 Tools
🪛 LanguageTool
[style] ~20-~20: Consider placing the discourse marker ‘first’ at the beginning of the sentence for more clarity.
Context: ... specific conditions require otherwise. The system first determines the leader of the table bein...(SENT_START_FIRST_PREMIUM)
Summary by CodeRabbit
Release Notes
New Features
BEGIN,BEGIN IMMEDIATE,COMMIT,ROLLBACK, andSAVEPOINT.Improvements
Documentation