Improve support for SQLite #61
Conversation
WalkthroughThe updates introduce comprehensive enhancements for SQLite support, including improved parsing and migration script generation, as well as extensive new tests for SQLite schemas. Internal logic for handling SQLite-specific column options and constraints is refined. The GitHub Actions workflow and a dependency are also updated, and documentation for the test structure is added. Changes
Sequence Diagram(s)sequenceDiagram
participant Tester
participant Sqlize
participant Parser
participant MigrationGen
Tester->>Sqlize: Initialize (SQLite)
Tester->>Sqlize: Parse(schema SQL)
Sqlize->>Parser: Parse SQLite schema
Parser->>Sqlize: Return parsed schema
Tester->>MigrationGen: Generate migration scripts (Up/Down)
MigrationGen->>Sqlize: Use parsed schema for scripts
MigrationGen-->>Tester: Return migration SQL
Tester->>Tester: Assert SQL contains expected constraints
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (10)
test/README.md (1)
1-3: Enhance test README with usage instructions
The README succinctly links to Go test layout conventions but could benefit from a “How to run” section. For example:# SQLize tests +## How to run tests + +Use: +```bash +go test ./test/sqlite +```.github/workflows/go.yml (1)
13-31: Optimize CI with Go module caching
Consider adding anactions/cachestep for Go modules to speed up dependency resolution:- name: Cache Go modules uses: actions/cache@v3 with: path: | ~/.cache/go-build ~/go/pkg/mod key: ${{ runner.os }}-go-${{ hashFiles('**/go.sum') }} restore-keys: | ${{ runner.os }}-go-test/sqlite/consts.go (1)
1-6: Adhere to Go testdata conventions
Consider renaming thedatadirectory totestdata(e.g.,test/sqlite/testdata/...) so Go treats it as a test-only resource and to avoid accidental packaging in production. For instance:-const ( - schemaOneTable = "data/schema_one_table.sql" - schemaTwoTables = "data/schema_two_tables.sql" -) +const ( + schemaOneTable = "testdata/schema_one_table.sql" + schemaTwoTables = "testdata/schema_two_tables.sql" +)test/sqlite/parser_test.go (3)
10-11: Update function comment to match its nameThe function comment incorrectly states "TestSqliteParser" but the function name is "TestParserSingleTable". Update the comment to accurately reflect the purpose of this specific test.
-// TestSqliteParser tests that Sqlize can parse a sqlite schema with one table. +// TestParserSingleTable tests that Sqlize can parse a sqlite schema with one table.
25-26: Update function comment to match its nameThe function comment incorrectly states "TestSqliteParser" but the function name is "TestParserMultipleTables". Update the comment to accurately reflect the purpose of this specific test.
-// TestSqliteParser tests that Sqlize can parse a sqlite schema with foreign keys. +// TestParserMultipleTables tests that Sqlize can parse a sqlite schema with foreign keys.
11-23: Consider validating the parsed schema structureThe test currently only checks that parsing succeeds without errors, but doesn't validate the correctness of the parsed output. Consider adding assertions to verify that the parsed schema correctly represents the expected structure (e.g., table name, columns, constraints).
func TestParserSingleTable(t *testing.T) { sqlizeCurrent := sqlize.NewSqlize( sqlize.WithSqlite(), ) schemaSqlBytes, err := os.ReadFile(schemaOneTable) if err != nil { t.Fatalf("failed to read schema file: %v", err) } if err := sqlizeCurrent.FromString(string(schemaSqlBytes)); err != nil { t.Fatalf("failed to parse schema: %v", err) } + + // Validate the parsed schema structure + tables := sqlizeCurrent.GetTables() + if len(tables) != 1 { + t.Fatalf("expected 1 table, got %d", len(tables)) + } + + table := tables[0] + if table.Name != "table_with_all_types" { + t.Errorf("expected table named 'table_with_all_types', got '%s'", table.Name) + } + + // Validate columns, indexes, etc. }test/sqlite/migration_test.go (1)
10-11: Update function comment to match its nameThe function comment incorrectly states "TestSqliteParser" but the function name is "TestMigrationGeneratorSingleTable". Update the comment to accurately reflect the purpose of this specific test.
-// TestSqliteParser tests that Sqlize can generate a migration script for the simplest schema. +// TestMigrationGeneratorSingleTable tests that Sqlize can generate a migration script for the simplest schema.element/column.go (1)
236-236: Clarify or remove the placeholder commentThe comment "More Sqlite overrides here..." suggests that additional SQLite-specific overrides might be needed. Either document what specific overrides are missing or remove this placeholder comment if all necessary overrides are already implemented.
- // More Sqlite overrides here... + // SQLite-specific column options handled above. Generic options continue below.sql-parser/sqlite.go (2)
11-133: Consider improving error propagation in Visit methodThe current implementation of
Visitalways returnsnil, nil, nilregardless of any errors that might occur during processing of SQLite nodes. This could hide errors and make debugging difficult.Consider modifying the method to propagate errors that might occur during processing:
func (p *Parser) Visit(node sqlite.Node) (w sqlite.Visitor, n sqlite.Node, err error) { // ... existing code ... // Example error handling for a possible error case // if someOperation() != nil { // return nil, nil, err // } - return nil, nil, nil + return p, nil, nil // Return self as visitor for continued traversal }
43-43: Consider addressing the TODO commentThere's a TODO comment mentioning that "rqlite/sql doesn't support parse constraints," but the code now seems to be successfully parsing constraints. This comment might be outdated.
If the library has been updated to support constraint parsing, consider removing or updating this comment to reflect the current state.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
.github/workflows/go.yml(2 hunks)element/column.go(2 hunks)go.mod(1 hunks)sql-parser/sqlite.go(4 hunks)test/README.md(1 hunks)test/sqlite/consts.go(1 hunks)test/sqlite/data/schema_one_table.sql(1 hunks)test/sqlite/data/schema_two_tables.sql(1 hunks)test/sqlite/migration_test.go(1 hunks)test/sqlite/parser_test.go(1 hunks)
🔇 Additional comments (18)
go.mod (1)
9-9: Dependency Update Validity
The bump ofmygithub.libinneed.workers.dev/rqlite/sqltov0.0.0-20241111133259-a4122fabb196aligns with the new SQLite enhancements. Ensure you’ve rungo mod tidyand updatedgo.sumaccordingly to avoid stale dependencies..github/workflows/go.yml (2)
14-14: Approve checkout action version
Upgradingactions/checkoutto@v4keeps the workflow current and secure.
17-19: Verify setup-gogo-version-fileusage
Switching toactions/setup-go@v5and sourcing the Go version fromgo.modis a great improvement. Confirm thatsetup-go@v5supports thego-version-fileinput as expected.test/sqlite/data/schema_two_tables.sql (1)
1-12: Approve multi-table schema for SQLite tests
The SQL correctly defines two tables withAUTOINCREMENT,UNIQUE, and a foreign key. This will effectively validate multi-table and constraint parsing.test/sqlite/data/schema_one_table.sql (2)
1-19: The schema covers essential SQLite features wellThe table schema includes a good mix of SQLite-specific features like AUTOINCREMENT, CHECK constraints, UNIQUE constraints, and various DEFAULT values. This provides comprehensive test coverage for the SQLite parser enhancements.
10-10:✅ Verification successful
Note SQLite's handling of BOOLEAN type
SQLite doesn't have a native BOOLEAN type - it typically stores booleans as integers (0/1). While this will work, be aware that the underlying storage will be an INTEGER, and TRUE will be stored as 1.
🌐 Web query:
How does SQLite handle BOOLEAN data type?💡 Result:
How SQLite Handles the BOOLEAN Data Type
No Native BOOLEAN Storage Class
SQLite does not have a dedicated BOOLEAN storage class like some other relational databases (such as PostgreSQL or MySQL)[2][4][5][6]. Officially, SQLite recognizes only five storage classes: NULL, INTEGER, REAL, TEXT, and BLOB[6][7].
BOOLEAN Is an Alias for INTEGER
When you declare a column as
BOOLEANin a table definition, SQLite internally treats it as having INTEGER affinity[5][6]. This means that, under the hood, Boolean values are stored as integers.
TRUEis stored as 1FALSEis stored as 0You can create a table with a column using the BOOLEAN keyword, but SQLite will store any Boolean values as 0 or 1 in an INTEGER field[1][4][5].
CREATE TABLE user ( id INTEGER PRIMARY KEY, is_active BOOLEAN ); INSERT INTO user (is_active) VALUES (1); -- TRUE INSERT INTO user (is_active) VALUES (0); -- FALSEHandling Values in Practice
When working with Boolean data in SQLite, you must use integer values (0 and 1) in queries, inserts, and application logic[1][2][5].
- For queries:
SELECT * FROM user WHERE is_active = 1;will return rows where the boolean isTRUE(1)[1].- Some drivers and ORMs may help with automatic conversion between booleans and integers, but at the database level, it's always 0 or 1[2][7].
BOOLEAN Affinity and Conversion
If a column is declared as BOOLEAN, SQLite will attempt to convert incoming values (such as 'true', 'false', 'y', etc.) to 0 or 1 during insertion, but this is not strictly enforced. Invalid values will not be rejected unless you explicitly add a CHECK constraint[5]:
CREATE TABLE user ( id INTEGER PRIMARY KEY, is_admin BOOLEAN CHECK (is_admin IN (0, 1)) );This ensures only valid Boolean (0 or 1) values can be set.
Summary Table
Declared Type SQLite Affinity Storage TRUE FALSE Enforced? BOOLEAN INTEGER 0 or 1 1 0 Not without CHECK INTEGER INTEGER 0 or 1 1 0 Not without CHECK Key Points
- SQLite does not natively support the BOOLEAN type; it is treated as INTEGER[4][5][6].
- TRUE and FALSE are stored as 1 and 0, respectively[1][4][5].
- Declaring a column as BOOLEAN is only for semantic clarity; it does not enforce strict Boolean behavior[5].
- To ensure only Boolean values (0 or 1) are stored, use a CHECK constraint[5].
Practical Advice:
Always use INTEGER for Boolean columns, or declare BOOLEAN for clarity and add a CHECK constraint for strict type enforcement. In your application code, treat these fields as integers and convert to/from Boolean as needed[1][2]."Although SQLite does not have a BOOLEAN type, its flexibility allows developers to use INTEGER effectively to store and manipulate Boolean values." [4]
Citations:
- 1: https://www.beekeeperstudio.io/blog/guide-to-boolean-columns-in-sqlite
- 2: https://dev.to/mliakos/how-to-handle-boolean-values-in-sqlite-using-javascript-proxies-1mg1
- 3: https://forum.xojo.com/t/using-boolean-value-with-sqlite/14485
- 4: https://www.w3resource.com/sqlite/snippets/sqlite-boolean-data.php
- 5: https://sqldocs.org/sqlite-database/sqlite-boolean/
- 6: https://www.tutorialspoint.com/sqlite/sqlite_data_types.htm
- 7: https://orm.drizzle.team/docs/column-types/sqlite
- 8: SeaQL/sea-query#375
Be aware: SQLite’s BOOLEAN type uses INTEGER storage
SQLite does not support a native BOOLEAN storage class. Declaring a column asBOOLEANgives it INTEGER affinity, soTRUEis stored as 1 andFALSEas 0. If you need strict enforcement, consider adding aCHECK (is_active IN (0, 1))constraint.element/column.go (1)
208-237: Well-structured SQLite-specific column option handlingThe implementation for SQLite-specific column options is well-organized with clear handling for each option type (DEFAULT, AUTOINCREMENT, UNIQUE, CHECK). This addresses the PR's objective of improving SQLite support for these column options.
sql-parser/sqlite.go (11)
20-21: Improved error handlingThe change to explicitly capture and return the error from
sqlite.Walkis a good improvement in error handling.
35-35: Method signature updated to match library interfaceThe
Visitmethod signature has been updated to match the interface requirements of the updated rqlite/sql library, now returning three values instead of one.
52-53: Enhanced constraint parsingNow passing the complete column definition instead of just constraints, which provides more context for constraint parsing.
126-127: Consistent approach to constraint parsingThis change maintains consistency with the earlier modification, passing the full column definition to
parseSqliteConstrains.
132-132: Updated return statement to match signatureThis properly implements the updated interface with three return values (visitor, node, error).
135-139: Improved constraint parsing functionThe method now accepts a full column definition and extracts constraints internally, which provides greater flexibility and context.
143-147: Added support for autoincrementNow correctly handling the autoincrement option for primary keys in SQLite, which is one of the key improvements mentioned in the PR objectives.
152-152: Better handling of unique constraintsChanged from adding an index to directly using a unique key option, which is a more accurate representation of SQLite's unique constraints.
155-158: Improved check constraint handlingNow properly capturing the expression string from check constraints, enhancing SQLite support as intended.
161-164: Better default value handlingUsing the string representation of the expression for default values, which improves SQLite compatibility.
171-172: Updated VisitEnd method signatureMethod now correctly returns
(sqlite.Node, error)to match the updated interface requirements.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/sqlite/migration_test.go (1)
24-51: LGTM: Comprehensive migration testing with output validationThe helper function effectively tests various migration methods and validates their output against expected SQLite-specific syntax. The validation addresses the suggestion from previous reviews.
🧹 Nitpick comments (3)
test/sqlite/common.go (1)
9-12: Consider using package-relative paths for test dataThe constants use relative paths which assume tests will be run from a specific working directory. To make tests more robust, consider using package-relative paths or a more explicit path resolution mechanism.
const ( - schemaWithOneTable = "data/schema_one_table.sql" - schemaWithTwoTables = "data/schema_two_tables.sql" + schemaWithOneTable = "./data/schema_one_table.sql" + schemaWithTwoTables = "./data/schema_two_tables.sql" )test/sqlite/migration_test.go (1)
10-22: Consider adding a test for the two tables schemaThe test covers the single table schema well, but there's no test utilizing the
schemaWithTwoTablesconstant defined in common.go.+// TestMigrationGeneratorTwoTables tests that Sqlize can generate migration scripts for a schema with multiple tables. +func TestMigrationGeneratorTwoTables(t *testing.T) { + sqlizeCurrent := sqlize.NewSqlize( + sqlize.WithSqlite(), + ) + + schemaSql := readFile(t, schemaWithTwoTables) + if err := sqlizeCurrent.FromString(schemaSql); err != nil { + t.Fatalf("failed to parse schema: %v", err) + } + + runVariousMigrationFunctions(t, sqlizeCurrent) +}test/sqlite/data/schema_one_table.sql (1)
7-7: Specify column type explicitly for clarityThe
number_with_defaultcolumn is missing an explicit type declaration. While SQLite allows this (defaulting to a dynamic type), adding an explicit type would improve clarity and consistency.- number_with_default DEFAULT 123, + number_with_default INTEGER DEFAULT 123,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (5)
element/column.go(2 hunks)test/sqlite/common.go(1 hunks)test/sqlite/data/schema_one_table.sql(1 hunks)test/sqlite/migration_test.go(1 hunks)test/sqlite/parser_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- test/sqlite/parser_test.go
- element/column.go
🔇 Additional comments (4)
test/sqlite/common.go (2)
14-18: LGTM: Well-structured assertion helperThe
assertContainsfunction is well-designed with clear error messages and follows Go testing best practices.
20-27: LGTM: Well-structured file reader with proper error handlingThe
readFilehelper correctly usest.Helper()to improve test output and has thorough error handling.test/sqlite/data/schema_one_table.sql (2)
1-29: LGTM: Comprehensive schema with diverse SQLite featuresThis schema example is excellent as it thoroughly covers a wide range of SQLite features including various data types, constraints (PRIMARY KEY, AUTOINCREMENT, NOT NULL, UNIQUE, CHECK), and default values with different formats and escape sequences.
31-33: LGTM: Good index coverageThe schema properly demonstrates both single-column and multi-column indexes with the recommended
IF NOT EXISTSclause.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/sqlite/migration_test.go (2)
24-51: Consider adding tests for more complex schema scenariosWhile this test effectively validates SQLite features for a single table, consider adding tests for more complex scenarios:
- Multiple tables with foreign key relationships
- Tables with indices
- Tables with complex constraints
These would provide more comprehensive coverage of SQLite migration generation.
29-33: Consider validating SQL statement syntax more preciselyThe current assertions check for the presence of certain keywords but don't validate the exact syntax or positioning. For more thorough testing, consider:
// Validate generated migration scripts assertContains(t, upSQL, "CREATE TABLE", "Up migration should create the table") assertContains(t, upSQL, "AUTOINCREMENT", "Up migration should include AUTOINCREMENT") assertContains(t, upSQL, "CHECK (\"age\" >= 18)", "Up migration should include CHECK constraint") assertContains(t, upSQL, "UNIQUE", "Up migration should include UNIQUE values") assertContains(t, upSQL, "DEFAULT", "Up migration should include DEFAULT values") + +// Validate the complete CREATE TABLE statement structure +expectedCreatePattern := `CREATE TABLE IF NOT EXISTS "users" \(` +assertMatchesRegexp(t, upSQL, expectedCreatePattern, "Create table statement should match expected pattern") + +// Validate the correct order of statements +assertStatementOrder(t, upSQL, []string{"CREATE TABLE", "INSERT INTO"}, "Statements should be in correct order")This would provide more precise validation of the generated SQL syntax.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (7)
sql-parser/sqlite.go(4 hunks)test/README.md(1 hunks)test/sqlite/common.go(1 hunks)test/sqlite/migration_test.go(1 hunks)test/sqlite/parser_test.go(1 hunks)test/sqlite/testdata/schema_one_table.sql(1 hunks)test/sqlite/testdata/schema_two_tables.sql(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- test/sqlite/testdata/schema_two_tables.sql
- test/sqlite/parser_test.go
- test/sqlite/testdata/schema_one_table.sql
🚧 Files skipped from review as they are similar to previous changes (3)
- test/README.md
- test/sqlite/common.go
- sql-parser/sqlite.go
🔇 Additional comments (2)
test/sqlite/migration_test.go (2)
10-22: Good implementation of the SQLite migration test!The test correctly initializes the SQLite configuration and validates the generated SQL. It's a good foundation for testing SQLite-specific migration functionality.
29-33: Great job validating SQLite-specific features!The test properly validates key SQLite features that were added in this PR:
- AUTOINCREMENT for primary keys
- CHECK constraints
- UNIQUE constraints
- DEFAULT values
This ensures that the migration generator correctly handles these SQLite-specific column options.
Add support of:
column options
(Sorry, no foreign keys support within create table expression at the moment)
Add unit tests for SQLite-related cases
Update rqlite version and adapt it
Improve CI pipeline a little
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Documentation