fix: Batch 1 critical bug fixes (#288, #289, #291)

[ajitpratap0]

Batch 1 — Critical Bug Fixes

Changes

1. fix(parser): support INSERT ... SELECT syntax (fixes Parser rejects valid INSERT ... SELECT syntax #288)

   • Parser now accepts INSERT INTO ... SELECT in addition to INSERT INTO ... VALUES
   • InsertStatement.Query changed to Statement interface for UNION support
   • 6 new test cases

2. fix(cli): return non-zero exit codes on errors (fixes CLI returns exit code 0 on parse/validate/format errors #289)

   • format and lint commands now return non-zero exit codes on failure
   • Other commands already had correct behavior

3. fix(transform): SetLimit negative validation tests (fixes transform.SetLimit accepts negative and zero values without error #291)

   • Added test coverage for SetLimit(-1), SetLimit(0), SetOffset(0)
   • Validation was already present, tests were missing

Fixes #288, fixes #289, fixes #291

[claude]

Claude finished @ajitpratap0's task —— View job

---

PR Review - Batch 1 Critical Bug Fixes

Reviewing this PR that addresses three critical bug fixes:

Todo List:

• Read and analyze all changed files
• Review Fix Welcome to GoSQLX Discussions! #1: INSERT...SELECT syntax support (Parser rejects valid INSERT ... SELECT syntax #288)
• Review Fix go get .... #2: CLI exit codes (CLI returns exit code 0 on parse/validate/format errors #289)
• Review Fix go.mod has incorrect module name #3: SetLimit validation tests (transform.SetLimit accepts negative and zero values without error #291)
• Check code quality and best practices
• Assess security and performance implications
• Evaluate test coverage
• Provide comprehensive feedback

---

🔍 Review Summary

Fix #1: INSERT...SELECT Support (#288) ✅

Changes Made:

• Modified InsertStatement.Query field from *SelectStatement to Statement interface in pkg/sql/ast/ast.go:1099
• Updated parser in pkg/sql/parser/dml.go:57-68 to support INSERT ... SELECT syntax
• Enhanced SQL formatting in multiple files to handle the new structure
• Added comprehensive test coverage in pkg/sql/parser/insert_select_test.go

Assessment: Excellent implementation

• The change from concrete SelectStatement to Statement interface is well-designed for extensibility
• Properly handles both INSERT ... SELECT and INSERT ... VALUES syntax
• Test coverage includes edge cases like UNION operations and both with/without column specifications
• SQL formatter correctly handles the new structure with proper type checking

Fix #2: CLI Exit Codes (#289) ✅

Changes Made:

• Updated cmd/gosqlx/cmd/format.go:100-108 to return non-zero exit codes on formatting failures
• Updated cmd/gosqlx/cmd/lint.go:188-194 to properly exit with code 1 on lint errors/warnings
• Enhanced error handling in formatter functions

Assessment: Correct implementation

• Properly distinguishes between different error conditions
• Uses os.Exit(1) appropriately for CLI tools
• Maintains backward compatibility while fixing the exit code issue
• Follows Unix conventions for command-line tool exit codes

Fix #3: SetLimit Validation Tests (#291) ✅

Changes Made:

• Added comprehensive test cases in pkg/transform/limit_test.go:67-113
• Tests cover negative values (-1, -100), zero values, and edge cases
• Validates that existing validation logic works correctly

Assessment: Good test coverage

• Tests confirm validation was already present but not thoroughly tested
• Covers important edge cases including SetLimit(0) and SetOffset(0) as valid operations
• Proper error expectations for negative values

---

📊 Code Quality Assessment

Strengths:

1. Clean Architecture: The interface change in Fix Welcome to GoSQLX Discussions! #1 follows SOLID principles
2. Comprehensive Testing: All fixes include proper test coverage
3. Error Handling: Proper error propagation and CLI exit codes
4. Documentation: Good code comments and test descriptions
5. Backwards Compatibility: Changes don't break existing functionality

Code Style:

• Follows Go conventions consistently
• Proper use of receiver types and memory management
• Good separation of concerns

Memory Management:

• No memory leaks introduced
• Proper use of existing object pooling patterns

---

🔒 Security & Performance

Security: ✅ No Issues

• No new attack vectors introduced
• Input validation maintained for transform functions
• SQL injection protection preserved in parser

Performance: ✅ No Regression

• Interface change has minimal overhead
• CLI exit code changes are O(1) operations
• New tests don't impact runtime performance

---

🧪 Test Coverage Analysis

INSERT...SELECT Tests:

• Coverage: Excellent - 6 test cases covering various scenarios
• Edge Cases: UNION operations, column specifications, WHERE clauses
• Validation: SQL roundtrip testing included

CLI Exit Code Tests:

• Coverage: Good - integrated into existing test suites
• Validation: Tests confirm proper exit behavior in cmd/gosqlx/cmd/lint_test.go

SetLimit Tests:

• Coverage: Comprehensive - negative values, zero, edge cases
• Validation: Proper error expectations and success cases

---

✅ Final Recommendation: APPROVE

This PR successfully fixes three critical bugs with:

• High-quality implementations following project patterns
• Comprehensive test coverage for all changes
• No breaking changes or security issues
• Proper documentation and code comments

The changes are production-ready and maintain the project's high standards for code quality and reliability.

---