Skip to content

Commit 134f680

Browse files
ajitpratap0Ajit Pratap Singhclaude
authored
feat: code review fixes and LSP enhancements (#129)
* fix: address critical issues from code review CRITICAL fixes: - Fix unchecked type assertions in alter.go (RoleOption.String()) - Fix unchecked type assertions in value.go (Value.String()) - Fix race condition in metrics.go using atomic operations - Fix race condition in LSP documents.go with defensive copy - Fix race condition in watch.go with RWMutex protection HIGH priority fixes: - Add missing Node interface methods to WindowFrameBound - Fix LSP hover returning nil instead of empty response MEDIUM priority fixes: - Add semantic error builders (E3001-E3004) for UndefinedTable, UndefinedColumn, TypeMismatch, AmbiguousColumn - Add missing object pools for FunctionCall, CaseExpression, BetweenExpression, InExpression, SubqueryExpression, CastExpression - Improve UTF-8 handling in LSP GetWordAtPosition All changes verified with race detection tests. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * feat: add MATERIALIZED CTE support and improve metrics Features: - Add MATERIALIZED/NOT MATERIALIZED parsing for CTEs (SQL standard) - Add comprehensive parser and AST metrics tracking - Add pool metrics for AST, statement, and expression objects Fixes: - Fix tokenizer negative column numbers in toSQLPosition - Correct column calculation to be consistently 1-based Metrics additions: - RecordParse() for parser operation tracking - RecordASTPoolGet/Put() for AST pool metrics - RecordStatementPoolGet/Put() for statement pool metrics - RecordExpressionPoolGet/Put() for expression pool metrics - Extended Stats struct with parser and pool metrics Tests: - Add TestParser_MaterializedCTE test cases - Update tokenizer error location test for correct column value - Add column validation to fuzz tests 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * feat(lsp): add documentSymbol, signatureHelp, and codeAction methods - Add textDocument/documentSymbol for SQL statement outline - Add textDocument/signatureHelp with signatures for 20+ SQL functions - Add textDocument/codeAction for quick fixes (semicolon, uppercase) - Add protocol types: DocumentSymbol, SignatureHelp, CodeAction, etc. - Update ServerCapabilities to advertise new features 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * fix: improve LSP security and AST pool efficiency LSP Server Security: - Add rate limiting (100 requests/second) to prevent DoS - Add content length validation (max 10MB per message) - Add document size limits (max 5MB for validation) - Improve error handling for malformed JSON-RPC requests - Add missing LSP error codes (RequestCancelled, etc.) AST Pool Efficiency: - Convert recursive PutExpression() to iterative approach with work queue - Add MaxCleanupDepth and MaxWorkQueueSize limits to prevent stack overflow - Add pools for missing expression types: ExistsExpression, AnyExpression, AllExpression, ListExpression, UnaryExpression, ExtractExpression, PositionExpression, SubstringExpression - Update PutSelectStatement to use iterative expression cleanup 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Ajit Pratap Singh <ajitpratapsingh@Ajits-Mac-mini.local> Co-authored-by: Claude <noreply@anthropic.com>
1 parent a0b5675 commit 134f680

17 files changed

Lines changed: 1737 additions & 103 deletions

File tree

cmd/gosqlx/cmd/watch.go

Lines changed: 36 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@ type FileWatcher struct {
4343
debounceMu sync.Mutex
4444
watchedFiles map[string]bool
4545
watchedDirs map[string]bool
46+
watchedMu sync.RWMutex // Protects watchedFiles and watchedDirs
4647
}
4748

4849
// NewFileWatcher creates a new file watcher instance
@@ -75,7 +76,10 @@ func (fw *FileWatcher) Watch(args []string) error {
7576
return err
7677
}
7778

78-
if len(fw.watchedFiles) == 0 && len(fw.watchedDirs) == 0 {
79+
fw.watchedMu.RLock()
80+
noFiles := len(fw.watchedFiles) == 0 && len(fw.watchedDirs) == 0
81+
fw.watchedMu.RUnlock()
82+
if noFiles {
7983
return fmt.Errorf("no files or directories to watch")
8084
}
8185

@@ -119,7 +123,9 @@ func (fw *FileWatcher) watchLoop(ctx context.Context) {
119123
if event.Has(fsnotify.Create) {
120124
info, err := os.Stat(event.Name)
121125
if err == nil && !info.IsDir() && fw.shouldProcessFile(event.Name) {
126+
fw.watchedMu.Lock()
122127
fw.watchedFiles[event.Name] = true
128+
fw.watchedMu.Unlock()
123129
}
124130
}
125131

@@ -228,7 +234,15 @@ func (fw *FileWatcher) formatFile(filename string, timestamp string) {
228234
func (fw *FileWatcher) processAllFiles() error {
229235
timestamp := time.Now().Format("15:04:05")
230236

237+
// Copy files under lock to avoid race conditions
238+
fw.watchedMu.RLock()
239+
files := make([]string, 0, len(fw.watchedFiles))
231240
for file := range fw.watchedFiles {
241+
files = append(files, file)
242+
}
243+
fw.watchedMu.RUnlock()
244+
245+
for _, file := range files {
232246
switch fw.opts.Mode {
233247
case WatchModeValidate:
234248
fw.validateFile(file, timestamp)
@@ -291,6 +305,9 @@ func (fw *FileWatcher) addSinglePath(path string) error {
291305
return fmt.Errorf("cannot get absolute path for '%s': %w", path, err)
292306
}
293307

308+
fw.watchedMu.Lock()
309+
defer fw.watchedMu.Unlock()
310+
294311
if info.IsDir() {
295312
if !fw.watchedDirs[absPath] {
296313
if err := fw.watcher.Add(absPath); err != nil {
@@ -328,6 +345,9 @@ func (fw *FileWatcher) addDirectoryRecursive(root string) error {
328345
return err
329346
}
330347

348+
fw.watchedMu.Lock()
349+
defer fw.watchedMu.Unlock()
350+
331351
if info.IsDir() {
332352
if !fw.watchedDirs[absPath] {
333353
if err := fw.watcher.Add(absPath); err != nil {
@@ -358,15 +378,27 @@ func (fw *FileWatcher) printWatchStatus() {
358378
modeStr = "formatting"
359379
}
360380

381+
fw.watchedMu.RLock()
382+
numFiles := len(fw.watchedFiles)
383+
numDirs := len(fw.watchedDirs)
384+
var files []string
385+
if fw.opts.Verbose {
386+
files = make([]string, 0, numFiles)
387+
for file := range fw.watchedFiles {
388+
files = append(files, file)
389+
}
390+
}
391+
fw.watchedMu.RUnlock()
392+
361393
fmt.Fprintf(fw.opts.Out, "%s Watching %d file(s) in %d director(y/ies) for %s\n",
362394
colorCyan("👁"),
363-
len(fw.watchedFiles),
364-
len(fw.watchedDirs),
395+
numFiles,
396+
numDirs,
365397
modeStr)
366398

367399
if fw.opts.Verbose {
368400
fmt.Fprintf(fw.opts.Out, "Watched files:\n")
369-
for file := range fw.watchedFiles {
401+
for _, file := range files {
370402
fmt.Fprintf(fw.opts.Out, " - %s\n", file)
371403
}
372404
}

pkg/errors/builders.go

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -202,3 +202,67 @@ func InvalidSetOperationError(operation, description string, location models.Loc
202202
location,
203203
).WithContext(sql, len(operation)).WithHint("Ensure both queries have the same number and compatible types of columns")
204204
}
205+
206+
// Semantic Errors (E3001-E3004)
207+
208+
// UndefinedTableError creates an error for referencing an undefined table
209+
func UndefinedTableError(tableName string, location models.Location, sql string) *Error {
210+
return NewError(
211+
ErrCodeUndefinedTable,
212+
fmt.Sprintf("table '%s' does not exist", tableName),
213+
location,
214+
).WithContext(sql, len(tableName)).WithHint(fmt.Sprintf("Check the table name '%s' for typos or ensure it exists in the schema", tableName))
215+
}
216+
217+
// UndefinedColumnError creates an error for referencing an undefined column
218+
func UndefinedColumnError(columnName, tableName string, location models.Location, sql string) *Error {
219+
message := fmt.Sprintf("column '%s' does not exist", columnName)
220+
hint := fmt.Sprintf("Check the column name '%s' for typos or ensure it exists in the table", columnName)
221+
if tableName != "" {
222+
message = fmt.Sprintf("column '%s' does not exist in table '%s'", columnName, tableName)
223+
hint = fmt.Sprintf("Check that column '%s' exists in table '%s'", columnName, tableName)
224+
}
225+
return NewError(
226+
ErrCodeUndefinedColumn,
227+
message,
228+
location,
229+
).WithContext(sql, len(columnName)).WithHint(hint)
230+
}
231+
232+
// TypeMismatchError creates an error for type mismatch in expressions
233+
func TypeMismatchError(leftType, rightType, context string, location models.Location, sql string) *Error {
234+
message := fmt.Sprintf("type mismatch: cannot compare %s with %s", leftType, rightType)
235+
if context != "" {
236+
message = fmt.Sprintf("type mismatch in %s: cannot compare %s with %s", context, leftType, rightType)
237+
}
238+
return NewError(
239+
ErrCodeTypeMismatch,
240+
message,
241+
location,
242+
).WithContext(sql, 1).WithHint(fmt.Sprintf("Ensure compatible types or use explicit CAST to convert %s to %s", leftType, rightType))
243+
}
244+
245+
// AmbiguousColumnError creates an error for ambiguous column reference
246+
func AmbiguousColumnError(columnName string, tables []string, location models.Location, sql string) *Error {
247+
tableList := "multiple tables"
248+
if len(tables) > 0 {
249+
tableList = fmt.Sprintf("tables: %s", joinStrings(tables, ", "))
250+
}
251+
return NewError(
252+
ErrCodeAmbiguousColumn,
253+
fmt.Sprintf("column '%s' is ambiguous (appears in %s)", columnName, tableList),
254+
location,
255+
).WithContext(sql, len(columnName)).WithHint(fmt.Sprintf("Qualify the column with a table name or alias, e.g., 'table_name.%s'", columnName))
256+
}
257+
258+
// joinStrings is a helper to join strings with a separator
259+
func joinStrings(strs []string, sep string) string {
260+
if len(strs) == 0 {
261+
return ""
262+
}
263+
result := strs[0]
264+
for i := 1; i < len(strs); i++ {
265+
result += sep + strs[i]
266+
}
267+
return result
268+
}

pkg/lsp/documents.go

Lines changed: 23 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -72,12 +72,25 @@ func (dm *DocumentManager) Close(uri string) {
7272
delete(dm.documents, uri)
7373
}
7474

75-
// Get retrieves a document
75+
// Get retrieves a copy of a document to avoid race conditions
76+
// The returned document is a snapshot and modifications won't affect the original
7677
func (dm *DocumentManager) Get(uri string) (*Document, bool) {
7778
dm.mu.RLock()
7879
defer dm.mu.RUnlock()
7980
doc, ok := dm.documents[uri]
80-
return doc, ok
81+
if !ok {
82+
return nil, false
83+
}
84+
// Return a copy to prevent race conditions when accessing fields after lock release
85+
docCopy := &Document{
86+
URI: doc.URI,
87+
LanguageID: doc.LanguageID,
88+
Version: doc.Version,
89+
Content: doc.Content,
90+
Lines: make([]string, len(doc.Lines)),
91+
}
92+
copy(docCopy.Lines, doc.Lines)
93+
return docCopy, true
8194
}
8295

8396
// GetContent retrieves a document's content
@@ -138,35 +151,38 @@ func positionToOffset(lines []string, pos Position) int {
138151
}
139152

140153
// GetWordAtPosition returns the word at the given position
154+
// Uses rune-based indexing for proper UTF-8 handling
141155
func (doc *Document) GetWordAtPosition(pos Position) string {
142156
if pos.Line >= len(doc.Lines) {
143157
return ""
144158
}
145159

146160
line := doc.Lines[pos.Line]
147-
if pos.Character >= len(line) {
161+
runes := []rune(line)
162+
163+
if pos.Character >= len(runes) {
148164
return ""
149165
}
150166

151-
// Find word boundaries
167+
// Find word boundaries using rune indexing for UTF-8 safety
152168
start := pos.Character
153169
end := pos.Character
154170

155171
// Move start backwards to find word start
156-
for start > 0 && isWordChar(rune(line[start-1])) {
172+
for start > 0 && isWordChar(runes[start-1]) {
157173
start--
158174
}
159175

160176
// Move end forwards to find word end
161-
for end < len(line) && isWordChar(rune(line[end])) {
177+
for end < len(runes) && isWordChar(runes[end]) {
162178
end++
163179
}
164180

165181
if start == end {
166182
return ""
167183
}
168184

169-
return line[start:end]
185+
return string(runes[start:end])
170186
}
171187

172188
// isWordChar returns true if c is a valid word character

0 commit comments

Comments
 (0)