fix: Project names now normalize to lowercase to fix accidental duplicates#87
fix: Project names now normalize to lowercase to fix accidental duplicates#87DylanDevelops wants to merge 2 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR enforces consistent, case-insensitive project name handling by normalizing project names (lowercase + trimmed) across storage operations and via a new database migration, preventing accidental duplicates caused by casing/whitespace differences.
Changes:
- Added migration
002_normalize_project_namesand wired it intorunMigrations. - Introduced
normalizeProjectName()and applied it to project-name writes and lookups in the storage layer. - Expanded migration tests to cover normalization behavior and idempotency.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
internal/storage/migrations.go |
Adds and runs a new migration that normalizes project_name values in time_entries and milestones. |
internal/storage/db.go |
Adds normalizeProjectName() and applies it to inserts/queries/updates involving project_name. |
internal/storage/migrations_test.go |
Extends tests to validate the new normalization migration and its interaction with existing migrations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (d *Database) UpdateTimeEntry(id int64, entry *TimeEntry) error { | ||
| entry.ProjectName = normalizeProjectName(entry.ProjectName) | ||
| startTimeUTC := entry.StartTime.UTC() |
There was a problem hiding this comment.
UpdateTimeEntry mutates the caller-provided entry by rewriting entry.ProjectName in-place. This can be surprising for callers that reuse the struct (e.g., for UI display or further validation). Consider using a local normalizedProjectName := normalizeProjectName(entry.ProjectName) for the query parameters instead of modifying entry.
| _, err = tx.Exec("UPDATE time_entries SET project_name = LOWER(TRIM(project_name))") | ||
| if err != nil { | ||
| return fmt.Errorf("failed to normalize project names in time_entries: %w", err) | ||
| } | ||
|
|
||
| _, err = tx.Exec("UPDATE milestones SET project_name = LOWER(TRIM(project_name))") | ||
| if err != nil { | ||
| return fmt.Errorf("failed to normalize project names in milestones: %w", err) | ||
| } |
There was a problem hiding this comment.
UPDATE milestones SET project_name = LOWER(TRIM(project_name)) can fail due to the UNIQUE(project_name, name) constraint on milestones (see internal/storage/db.go:57-65). If two rows differ only by case/whitespace in project_name but share the same name, normalization will create duplicates and abort the migration, preventing DB initialization. Consider adding conflict handling (e.g., detect collisions up-front and merge/delete deterministically, or rebuild the table with normalized/deduped rows) so the migration is safe on real-world data.
| _, err = tx.Exec("UPDATE time_entries SET project_name = LOWER(TRIM(project_name))") | ||
| if err != nil { | ||
| return fmt.Errorf("failed to normalize project names in time_entries: %w", err) | ||
| } | ||
|
|
||
| _, err = tx.Exec("UPDATE milestones SET project_name = LOWER(TRIM(project_name))") | ||
| if err != nil { | ||
| return fmt.Errorf("failed to normalize project names in milestones: %w", err) | ||
| } | ||
|
|
There was a problem hiding this comment.
The migration’s normalization logic (LOWER(TRIM(project_name))) doesn’t match normalizeProjectName (strings.ToLower(strings.TrimSpace(...))). SQLite TRIM() only removes ASCII spaces by default and LOWER() behavior can differ for non-ASCII text, so existing rows may not end up normalized the same way as new writes/queries. To ensure consistent semantics, consider normalizing in Go using normalizeProjectName (scan + update) or explicitly trimming the same character set in SQL.
| _, err = tx.Exec("UPDATE time_entries SET project_name = LOWER(TRIM(project_name))") | |
| if err != nil { | |
| return fmt.Errorf("failed to normalize project names in time_entries: %w", err) | |
| } | |
| _, err = tx.Exec("UPDATE milestones SET project_name = LOWER(TRIM(project_name))") | |
| if err != nil { | |
| return fmt.Errorf("failed to normalize project names in milestones: %w", err) | |
| } | |
| // Normalize project names in time_entries using the same logic as normalizeProjectName. | |
| rows, queryErr := tx.Query("SELECT id, project_name FROM time_entries") | |
| if queryErr != nil { | |
| err = fmt.Errorf("failed to query time_entries for normalization: %w", queryErr) | |
| return err | |
| } | |
| defer rows.Close() | |
| for rows.Next() { | |
| var id int64 | |
| var projectName string | |
| if scanErr := rows.Scan(&id, &projectName); scanErr != nil { | |
| err = fmt.Errorf("failed to scan time_entries row: %w", scanErr) | |
| return err | |
| } | |
| normalized := normalizeProjectName(projectName) | |
| if normalized == projectName { | |
| continue | |
| } | |
| if _, execErr := tx.Exec( | |
| "UPDATE time_entries SET project_name = ? WHERE id = ?", | |
| normalized, | |
| id, | |
| ); execErr != nil { | |
| err = fmt.Errorf("failed to normalize project name in time_entries (id=%d): %w", id, execErr) | |
| return err | |
| } | |
| } | |
| if rowsErr := rows.Err(); rowsErr != nil { | |
| err = fmt.Errorf("error iterating time_entries rows: %w", rowsErr) | |
| return err | |
| } | |
| // Normalize project names in milestones using the same logic as normalizeProjectName. | |
| milestoneRows, mQueryErr := tx.Query("SELECT id, project_name FROM milestones") | |
| if mQueryErr != nil { | |
| err = fmt.Errorf("failed to query milestones for normalization: %w", mQueryErr) | |
| return err | |
| } | |
| defer milestoneRows.Close() | |
| for milestoneRows.Next() { | |
| var id int64 | |
| var projectName string | |
| if scanErr := milestoneRows.Scan(&id, &projectName); scanErr != nil { | |
| err = fmt.Errorf("failed to scan milestones row: %w", scanErr) | |
| return err | |
| } | |
| normalized := normalizeProjectName(projectName) | |
| if normalized == projectName { | |
| continue | |
| } | |
| if _, execErr := tx.Exec( | |
| "UPDATE milestones SET project_name = ? WHERE id = ?", | |
| normalized, | |
| id, | |
| ); execErr != nil { | |
| err = fmt.Errorf("failed to normalize project name in milestones (id=%d): %w", id, execErr) | |
| return err | |
| } | |
| } | |
| if rowsErr := milestoneRows.Err(); rowsErr != nil { | |
| err = fmt.Errorf("error iterating milestones rows: %w", rowsErr) | |
| return err | |
| } |
| assert.NoError(t, rows.Scan(&name)) | ||
| assert.Equal(t, entries[i].expectedName, name) | ||
| i++ | ||
| } |
There was a problem hiding this comment.
This test iterates rows.Next() and indexes entries[i] without asserting bounds or that the expected number of rows were returned. If the query returns fewer rows, the test can still pass; if it returns more, it can panic. Consider asserting i < len(entries) inside the loop, checking rows.Err(), and asserting i == len(entries) after the loop.
| assert.NoError(t, rows.Scan(&name)) | |
| assert.Equal(t, entries[i].expectedName, name) | |
| i++ | |
| } | |
| assert.NoError(t, rows.Scan(&name)) | |
| assert.Less(t, i, len(entries)) | |
| assert.Equal(t, entries[i].expectedName, name) | |
| i++ | |
| } | |
| assert.NoError(t, rows.Err()) | |
| assert.Equal(t, len(entries), i) |
Pull Request Checklist
mainbranch of tmpo.Description
This pull request implements normalization of project names throughout the database and codebase to ensure consistency (all project names are stored and queried in lowercase and without leading/trailing whitespace). It introduces a new migration to update existing records, updates all relevant database operations to use normalized names, and adds comprehensive tests to verify the migration and normalization logic.
Database normalization and migration:
Added a new database migration (
Migration002_NormalizeProjectNames) that updates all existingproject_namefields in bothtime_entriesandmilestonestables to be lowercase and trimmed of whitespace. This migration is idempotent and tracked in thesettingstable. [1] [2] [3]Introduced the
normalizeProjectNamehelper function to consistently normalize project names before any database operation or update.Codebase updates for normalization:
project_nameto use thenormalizeProjectNamefunction, ensuring new and queried data is always normalized. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11]Testing improvements:
migrations_test.goto cover migration execution, normalization of mixed-case and whitespace-containing project names, idempotency of the migration, and integration with fresh databases. [1] [2]These changes ensure that project names are handled in a consistent and case-insensitive manner across the application and database, preventing subtle bugs and improving data integrity.