feat: [#280] Add integer methods #727
Conversation
WalkthroughThis pull request introduces numerous enhancements to the database schema management system. New methods have been added to the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🔇 Additional comments (2)database/schema/grammars/mysql_test.go (2)
Enhance test coverage and modernize test structure The test should be converted to a table-driven format for better maintainability and coverage of edge cases, consistent with other tests in this file (e.g., func (s *MysqlSuite) TestTypeDecimal() {
- mockColumn := mocksschema.NewColumnDefinition(s.T())
- mockColumn.EXPECT().GetTotal().Return(4).Once()
- mockColumn.EXPECT().GetPlaces().Return(2).Once()
-
- s.Equal("decimal(4, 2)", s.grammar.TypeDecimal(mockColumn))
+ tests := []struct {
+ name string
+ total int
+ places int
+ expected string
+ }{
+ {"standard_case", 4, 2, "decimal(4, 2)"},
+ {"zero_places", 6, 0, "decimal(6, 0)"},
+ {"max_precision", 65, 30, "decimal(65, 30)"}, // MySQL max precision
+ {"zero_total", 0, 0, "decimal(10, 0)"}, // Default behavior
+ }
+
+ for _, test := range tests {
+ s.Run(test.name, func() {
+ mockColumn := mocksschema.NewColumnDefinition(s.T())
+ mockColumn.EXPECT().GetTotal().Return(test.total).Once()
+ mockColumn.EXPECT().GetPlaces().Return(test.places).Once()
+
+ s.Equal(test.expected, s.grammar.TypeDecimal(mockColumn))
+ })
+ }
}
Convert to table-driven tests and add edge cases The test should be converted to a table-driven format for better maintainability and coverage of edge cases, consistent with other tests in this file. func (s *MysqlSuite) TestTypeFloat() {
- mockColumn := mocksschema.NewColumnDefinition(s.T())
- mockColumn.EXPECT().GetPrecision().Return(0).Once()
-
- s.Equal("float", s.grammar.TypeFloat(mockColumn))
-
- mockColumn.EXPECT().GetPrecision().Return(2).Once()
-
- s.Equal("float(2)", s.grammar.TypeFloat(mockColumn))
+ tests := []struct {
+ name string
+ precision int
+ expected string
+ }{
+ {"no_precision", 0, "float"},
+ {"with_precision", 2, "float(2)"},
+ {"max_precision", 53, "float(53)"}, // MySQL max precision for FLOAT
+ {"default_precision", -1, "float"}, // Test negative precision
+ }
+
+ for _, test := range tests {
+ s.Run(test.name, func() {
+ mockColumn := mocksschema.NewColumnDefinition(s.T())
+ mockColumn.EXPECT().GetPrecision().Return(test.precision).Once()
+
+ s.Equal(test.expected, s.grammar.TypeFloat(mockColumn))
+ })
+ }
}Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #727 +/- ##
==========================================
- Coverage 69.61% 69.46% -0.16%
==========================================
Files 210 210
Lines 16583 16680 +97
==========================================
+ Hits 11545 11586 +41
- Misses 4389 4445 +56
Partials 649 649 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Outside diff range and nitpick comments (18)
contracts/database/schema/column.go (1)
16-21: Consider enhancing documentation with numeric type context.While the comments are clear, it would be helpful to add more context about the numeric type specifications. For example:
- GetPlaces: Specify if it's for decimal places
- GetPrecision: Clarify its relationship with floating-point/decimal types
- GetTotal: Explain if it represents total digits
Example enhancement:
- // GetPlaces returns the places value + // GetPlaces returns the number of decimal places for numeric column types - // GetPrecision returns the precision value + // GetPrecision returns the precision for floating-point or decimal column types - // GetTotal returns the total value + // GetTotal returns the total number of digits for numeric column typesdatabase/schema/column.go (1)
64-86: Add documentation and input validationConsider these improvements:
- Add godoc comments explaining each method's purpose and default values
- Add validation to prevent negative values
- Document the relationship between these fields (e.g., precision should be >= places)
Example implementation:
+ // GetPlaces returns the decimal places (scale) for numeric columns. + // Returns 2 if not explicitly set. func (r *ColumnDefinition) GetPlaces() (places int) { if r.places != nil { + if *r.places < 0 { + return 0 + } return *r.places } return 2 }contracts/database/schema/blueprint.go (1)
28-67: Consider grouping related methods with section commentsThe interface has grown with many integer-related methods. Consider adding section comments to group related methods, improving readability and maintenance. For example:
// Auto-incrementing column definitions // ... // Regular integer column definitions // ... // Unsigned integer column definitions // ...database/schema/grammars/sqlite.go (1)
174-184: Consider consolidating duplicate integer type methods.While the implementation is correct (SQLite treats all integers as 64-bit), we can reduce code duplication.
Consider consolidating these methods using a private helper:
+func (r *Sqlite) typeInteger() string { + return "integer" +} + func (r *Sqlite) TypeMediumInteger(column schema.ColumnDefinition) string { - return "integer" + return r.typeInteger() } func (r *Sqlite) TypeTinyInteger(column schema.ColumnDefinition) string { - return "integer" + return r.typeInteger() } func (r *Sqlite) TypeSmallInteger(column schema.ColumnDefinition) string { - return "integer" + return r.typeInteger() }database/schema/grammars/mysql.go (1)
191-193: Consider supporting precision for DOUBLE typeMySQL's DOUBLE type supports optional precision and scale parameters (DOUBLE(M,D)). Consider extending the method to accept these parameters for more precise control over double-precision numbers.
-func (r *Mysql) TypeDouble() string { +func (r *Mysql) TypeDouble(column schema.ColumnDefinition) string { + total := column.GetTotal() + places := column.GetPlaces() + if total > 0 && places >= 0 { + return fmt.Sprintf("double(%d,%d)", total, places) + } return "double" }database/schema/grammars/sqlserver.go (3)
207-209: Document that MEDIUMINT maps to INT in SQL Server.Add a comment explaining that SQL Server doesn't have a MEDIUMINT type, so it maps to INT (4 bytes, -2^31 to 2^31-1).
func (r *Sqlserver) TypeMediumInteger(column schema.ColumnDefinition) string { + // SQL Server doesn't have MEDIUMINT, using INT (4 bytes) instead return "int" }
211-213: Document TINYINT constraints and consider unsigned flag.SQL Server's TINYINT is always unsigned (0-255). Consider adding validation if the column is marked as signed.
func (r *Sqlserver) TypeTinyInteger(column schema.ColumnDefinition) string { + // SQL Server's TINYINT is always unsigned (0-255) + if !column.GetUnsigned() { + // Log a warning that signed TINYINT is not supported in SQL Server + } return "tinyint" }
215-217: Document SMALLINT range.Add a comment documenting the range of SMALLINT in SQL Server (-32,768 to 32,767).
func (r *Sqlserver) TypeSmallInteger(column schema.ColumnDefinition) string { + // SQL Server SMALLINT: 2 bytes, range -32,768 to 32,767 return "smallint" }database/schema/blueprint.go (2)
121-122: LGTM: Comprehensive set of integer type methodsThe implementation provides a complete set of integer types with consistent patterns for both signed and unsigned variants.
Consider documenting the following in the package documentation:
- The actual size/range of each integer type across different database engines
- Any database-specific limitations or behaviors
- Recommendations for choosing the appropriate integer type based on use case
Also applies to: 137-139, 161-163, 197-211
246-255: Consider adding input validationWhile the implementation is clean, consider adding validation for the input parameters to ensure robustness:
- Validate that the column name is not empty
- Validate that the type name is one of the expected integer types
Here's a suggested improvement:
func (r *Blueprint) addIntegerColumn(name, column string) schema.ColumnDefinition { + if column == "" { + panic("Column name cannot be empty") + } + validTypes := map[string]bool{ + "integer": true, "bigInteger": true, "mediumInteger": true, + "smallInteger": true, "tinyInteger": true, + } + if !validTypes[name] { + panic(fmt.Sprintf("Invalid integer column type: %s", name)) + } columnImpl := &ColumnDefinition{ name: &column, ttype: convert.Pointer(name), } r.addColumn(columnImpl) return columnImpl }database/schema/grammars/postgres.go (2)
216-218: Add documentation for type mappingThe implementation correctly maps MEDIUMINT to INTEGER as PostgreSQL doesn't have a native medium integer type. Consider adding a comment to document this intentional mapping.
+// TypeMediumInteger maps to regular INTEGER in PostgreSQL as it doesn't support MEDIUMINT func (r *Postgres) TypeMediumInteger(column schema.ColumnDefinition) string { return r.TypeInteger(column) }
220-222: Add documentation for type mappingThe implementation correctly maps TINYINT to SMALLINT as PostgreSQL doesn't have a native tiny integer type. Consider adding a comment to document this intentional mapping.
+// TypeTinyInteger maps to SMALLINT in PostgreSQL as it doesn't support TINYINT func (r *Postgres) TypeTinyInteger(column schema.ColumnDefinition) string { return r.TypeSmallInteger(column) }database/schema/blueprint_test.go (6)
166-175: Add test cases for integer modifiersThe
TestIntegermethod should include additional test cases to verify the behavior when using modifiers likeAutoIncrement()andUnsigned(), similar to howTestBigIntegeris implemented.Add these test cases:
func (s *BlueprintTestSuite) TestInteger() { name := "name" s.blueprint.Integer(name) s.Contains(s.blueprint.GetAddedColumns(), &ColumnDefinition{ name: &name, ttype: convert.Pointer("integer"), }) + + s.blueprint.Integer(name).AutoIncrement().Unsigned() + s.Contains(s.blueprint.GetAddedColumns(), &ColumnDefinition{ + autoIncrement: convert.Pointer(true), + name: &name, + unsigned: convert.Pointer(true), + ttype: convert.Pointer("integer"), + }) }Also applies to: 232-232
234-252: Add test cases for medium integer modifiersThe
TestMediumIntegermethod should include additional test cases to verify the behavior when using modifiers likeAutoIncrement()andUnsigned().Add these test cases:
func (s *BlueprintTestSuite) TestMediumInteger() { name := "name" s.blueprint.MediumInteger(name) s.Contains(s.blueprint.GetAddedColumns(), &ColumnDefinition{ name: &name, ttype: convert.Pointer("mediumInteger"), }) + + s.blueprint.MediumInteger(name).AutoIncrement().Unsigned() + s.Contains(s.blueprint.GetAddedColumns(), &ColumnDefinition{ + autoIncrement: convert.Pointer(true), + name: &name, + unsigned: convert.Pointer(true), + ttype: convert.Pointer("mediumInteger"), + }) }
254-271: Add test cases for small integer modifiersThe
TestSmallIntegermethod should include additional test cases to verify the behavior when using modifiers likeAutoIncrement()andUnsigned().Add these test cases:
func (s *BlueprintTestSuite) TestSmallInteger() { name := "name" s.blueprint.SmallInteger(name) s.Contains(s.blueprint.GetAddedColumns(), &ColumnDefinition{ name: &name, ttype: convert.Pointer("smallInteger"), }) + + s.blueprint.SmallInteger(name).AutoIncrement().Unsigned() + s.Contains(s.blueprint.GetAddedColumns(), &ColumnDefinition{ + autoIncrement: convert.Pointer(true), + name: &name, + unsigned: convert.Pointer(true), + ttype: convert.Pointer("smallInteger"), + }) }
294-312: Add test cases for tiny integer modifiersThe
TestTinyIntegermethod should include additional test cases to verify the behavior when using modifiers likeAutoIncrement()andUnsigned().Add these test cases:
func (s *BlueprintTestSuite) TestTinyInteger() { name := "name" s.blueprint.TinyInteger(name) s.Contains(s.blueprint.GetAddedColumns(), &ColumnDefinition{ name: &name, ttype: convert.Pointer("tinyInteger"), }) + + s.blueprint.TinyInteger(name).AutoIncrement().Unsigned() + s.Contains(s.blueprint.GetAddedColumns(), &ColumnDefinition{ + autoIncrement: convert.Pointer(true), + name: &name, + unsigned: convert.Pointer(true), + ttype: convert.Pointer("tinyInteger"), + }) }
350-388: Add test cases for unsigned integer modifiersThe unsigned integer test methods should include additional test cases to verify the behavior when using the
AutoIncrement()modifier. This applies to all unsigned integer types:UnsignedInteger,UnsignedMediumInteger,UnsignedSmallInteger, andUnsignedTinyInteger.Here's an example for
UnsignedInteger, apply similar changes to other unsigned integer tests:func (s *BlueprintTestSuite) TestUnsignedInteger() { name := "name" s.blueprint.UnsignedInteger(name) s.Contains(s.blueprint.GetAddedColumns(), &ColumnDefinition{ name: &name, ttype: convert.Pointer("integer"), unsigned: convert.Pointer(true), }) + + s.blueprint.UnsignedInteger(name).AutoIncrement() + s.Contains(s.blueprint.GetAddedColumns(), &ColumnDefinition{ + autoIncrement: convert.Pointer(true), + name: &name, + ttype: convert.Pointer("integer"), + unsigned: convert.Pointer(true), + }) }
Line range hint
166-388: Add error handling test casesThe test suite should include test cases for error scenarios and invalid inputs. Consider adding tests for:
- Empty column names
- Invalid length specifications (where applicable)
- Duplicate column names
- Invalid modifier combinations
Example test case for error handling:
func (s *BlueprintTestSuite) TestIntegerWithEmptyName() { s.blueprint.Integer("") s.Empty(s.blueprint.GetAddedColumns()) } func (s *BlueprintTestSuite) TestIntegerWithDuplicateName() { name := "name" s.blueprint.Integer(name) s.blueprint.Integer(name) columns := s.blueprint.GetAddedColumns() s.Len(columns, 1) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
mocks/database/schema/Blueprint.gois excluded by!mocks/**mocks/database/schema/ColumnDefinition.gois excluded by!mocks/**mocks/database/schema/Grammar.gois excluded by!mocks/**
📒 Files selected for processing (14)
contracts/database/schema/blueprint.go(1 hunks)contracts/database/schema/column.go(1 hunks)contracts/database/schema/grammar.go(1 hunks)database/schema/blueprint.go(7 hunks)database/schema/blueprint_test.go(4 hunks)database/schema/column.go(2 hunks)database/schema/grammars/mysql.go(1 hunks)database/schema/grammars/mysql_test.go(1 hunks)database/schema/grammars/postgres.go(2 hunks)database/schema/grammars/postgres_test.go(1 hunks)database/schema/grammars/sqlite.go(1 hunks)database/schema/grammars/sqlserver.go(1 hunks)database/schema/grammars/sqlserver_test.go(1 hunks)database/schema/postgres_schema.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- database/schema/postgres_schema.go
🔇 Additional comments (14)
contracts/database/schema/column.go (1)
16-21: LGTM! Well-structured interface additions.
The new methods follow consistent naming patterns and are properly documented. They appropriately extend the interface to support numeric column type specifications.
database/schema/column.go (1)
15-17: LGTM! Well-structured field additions
The new pointer fields are appropriately typed and named, following Go conventions. Using pointer types is a good choice here as it allows distinguishing between unset and zero values.
contracts/database/schema/grammar.go (1)
44-49: LGTM! Well-structured integer type methods
The new integer-related methods are well-designed with:
- Consistent method signatures
- Clear documentation
- Proper alignment with SQL integer types
contracts/database/schema/blueprint.go (3)
28-31: LGTM: Medium integer methods are well-defined
The medium integer methods are properly documented and follow the established interface patterns.
46-49: LGTM: Small integer methods are well-defined
The small integer methods are properly documented and follow the established interface patterns.
52-55: LGTM: Tiny integer methods are well-defined
The tiny integer methods are properly documented and follow the established interface patterns.
database/schema/grammars/sqlite.go (1)
174-184: Verify auto-increment handling for all integer types.
The implementation looks correct, but let's verify that auto-increment is properly handled for all integer types.
database/schema/grammars/mysql.go (1)
208-218: LGTM! Implementation aligns with modern MySQL versions
The integer type implementations correctly omit display width specifications, which aligns with MySQL 8.0.17+ recommendations where display widths for integer types are deprecated.
database/schema/grammars/sqlserver.go (1)
186-217: Verify data type implementations against SQL Server documentation.
Let's verify that all numeric data type implementations align with SQL Server's specifications.
✅ Verification successful
SQL Server numeric data type implementations are correctly implemented
The implementation aligns with SQL Server's data type specifications:
decimal(p,s)correctly uses total and places parametersfloatimplementation properly handles both default and precision-specific cases- Integer types map correctly:
intfor standard and medium integerstinyintfor tiny integerssmallintfor small integersdouble precisionfor double type
This is verified by the test suite which covers all these type implementations and shows they generate the expected SQL Server syntax.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any SQL Server-specific type tests and documentation
# Look for SQL Server type-related tests
echo "Searching for SQL Server type tests..."
rg -l "TestType.*" --type go | grep -i "sqlserver.*test"
# Look for documentation about SQL Server types
echo "Searching for SQL Server type documentation..."
fd -e md -e txt | xargs rg -l "SQL Server.*data.*type"
Length of output: 348
Script:
#!/bin/bash
# Let's examine the SQL Server type tests to verify the implementations
echo "Checking SQL Server type test implementations..."
cat database/schema/grammars/sqlserver_test.go
# Let's also check if there are any related interfaces or base implementations
echo -e "\nChecking related column definition interfaces..."
ast-grep --pattern 'type ColumnDefinition interface {
$$$
}'
Length of output: 11858
database/schema/blueprint.go (2)
32-32: LGTM: Good refactoring of integer methods
The refactoring to use the addIntegerColumn helper method reduces code duplication and improves maintainability.
Also applies to: 114-115
91-93: LGTM: Well-structured increment methods
The new increment methods follow a consistent pattern and naming convention. Each method properly chains the unsigned integer type with auto-increment functionality.
Let's verify the consistency of increment column usage across the codebase:
Also applies to: 103-105, 117-119, 133-135, 157-159
✅ Verification successful
Let me gather more information about the implementation of these increment methods to ensure complete verification.
Increment methods implementation verified as correct and consistent
The implementation is verified to be correct and follows a consistent pattern across all increment methods:
- Each increment method properly uses its corresponding unsigned integer method
- The unsigned integer methods are correctly implemented to chain with
.Unsigned() - Test coverage exists for both increment and unsigned integer methods
- The interface contract in
contracts/database/schema/blueprint.goproperly documents all methods
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any existing increment column definitions to ensure consistent usage
rg -g '*.go' -A 2 '\.(Medium|Integer|Small|Tiny)?Increments\('
Length of output: 1160
Script:
#!/bin/bash
# Check the implementation of unsigned integer methods that are used by increment methods
ast-grep --pattern 'func (r *Blueprint) UnsignedMediumInteger($_) $_'
ast-grep --pattern 'func (r *Blueprint) UnsignedInteger($_) $_'
ast-grep --pattern 'func (r *Blueprint) UnsignedSmallInteger($_) $_'
ast-grep --pattern 'func (r *Blueprint) UnsignedTinyInteger($_) $_'
# Check for any other usages of these methods in migrations or schema definitions
rg -g '*.go' 'UnsignedMediumInteger|UnsignedInteger|UnsignedSmallInteger|UnsignedTinyInteger'
Length of output: 9287
database/schema/grammars/postgres.go (3)
195-197: LGTM!
The implementation correctly returns "double precision" which is the proper PostgreSQL type for 8-byte floating-point numbers.
224-230: LGTM!
The implementation correctly handles both regular and auto-increment cases for small integers, using the appropriate PostgreSQL types:
- smallserial for auto-increment
- smallint for regular cases
Line range hint 191-231: Consider adding comprehensive test coverage
While the implementations are correct, consider adding test cases for:
- Edge cases in decimal precision and scale
- Float precision boundaries
- Auto-increment behavior for different integer types
- Type mapping verification for MEDIUMINT and TINYINT
This will ensure robust handling of all possible scenarios across different PostgreSQL versions.
Let's check the current test coverage:
📑 Description
Summary by CodeRabbit
Blueprintinterface with additional methods for diverse column definitions.✅ Checks