feat: [#280] Add primary method #712
Conversation
WalkthroughThe changes in this pull request enhance the database schema management functionality by introducing new methods and structures to the Changes
Possibly related PRs
Suggested reviewers
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 #712 +/- ##
==========================================
+ Coverage 69.66% 69.70% +0.04%
==========================================
Files 197 197
Lines 15271 15298 +27
==========================================
+ Hits 10638 10663 +25
- Misses 4057 4059 +2
Partials 576 576 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (10)
contracts/database/schema/blueprint.go (1)
36-39: Add documentation and consider type safety improvements.While the struct is well-structured, consider these enhancements:
- Add documentation comments for the struct and its fields
- Consider using a custom type or constants for
Algorithmto restrict valid valuesHere's a suggested improvement:
+// IndexConfig holds configuration options for database indexes type IndexConfig struct { + // Algorithm specifies the index algorithm (e.g., "BTREE", "HASH") Algorithm string + // Name specifies the custom name for the index Name string } +// Common index algorithms +const ( + IndexAlgorithmBTree = "BTREE" + IndexAlgorithmHash = "HASH" +)contracts/database/schema/grammar.go (1)
22-22: Consider enhancing the method documentation.While the current comment follows the codebase style, consider adding more details about the expected format of the command and what the returned string represents (e.g., the SQL statement format).
-// CompilePrimary Compile a primary key command. +// CompilePrimary Compile a primary key command. +// The command's Columns field should contain the column names to be used in the primary key. +// Returns the SQL statement for adding a primary key constraint.contracts/database/schema/schema.go (1)
69-69: LGTM! Consider adding field documentation.The addition of the
Columns []stringfield to theCommandstruct is well-designed for supporting multiple column operations, particularly useful for composite primary keys. The implementation is clean and consistent with the existing structure.Consider adding a comment to document the purpose of this field:
type Command struct { Algorithm string Column ColumnDefinition + // Columns holds multiple column names for operations like primary keys Columns []string From stringdatabase/schema/grammars/postgres.go (1)
61-63: Add column name escaping for SQL safety.While the basic implementation is correct, the column names should be properly escaped to handle special characters and reserved words in PostgreSQL.
Consider using the existing
EscapeNamesmethod:func (r *Postgres) CompilePrimary(blueprint schema.Blueprint, command *schema.Command) string { - return fmt.Sprintf("alter table %s add primary key (%s)", blueprint.GetTableName(), strings.Join(command.Columns, ",")) + return fmt.Sprintf("alter table %s add primary key (%s)", blueprint.GetTableName(), strings.Join(r.EscapeNames(command.Columns), ",")) }This change ensures SQL safety when column names contain spaces, special characters, or match PostgreSQL reserved words.
database/schema/blueprint.go (1)
191-197: Consider defining constants for special characters.The
createIndexNamemethod handles special character replacement well, but could benefit from defining constants for the replaceable characters (-and.) to improve maintainability and documentation.+ const ( + indexNameHyphenReplacement = "_" + indexNameDotReplacement = "_" + ) func (r *Blueprint) createIndexName(ttype string, columns []string) string { table := r.GetTableName() index := strings.ToLower(table + "_" + strings.Join(columns, "_") + "_" + ttype) - index = strings.ReplaceAll(index, "-", "_") + index = strings.ReplaceAll(index, "-", indexNameHyphenReplacement) - return strings.ReplaceAll(index, ".", "_") + return strings.ReplaceAll(index, ".", indexNameDotReplacement) }database/schema/schema_test.go (3)
90-107: Enhance test coverage for primary key functionality.While the test follows the suite's pattern, consider adding these test cases for robustness:
- Negative test cases (e.g., non-existent columns)
- Single column primary key
- Verification of the actual primary key constraint
Here's a suggested enhancement:
func (s *SchemaSuite) TestPrimary() { for driver, testQuery := range s.driverToTestQuery { s.Run(driver.String(), func() { schema := GetTestSchema(testQuery, s.driverToTestQuery) - table := "primaries" + + // Test composite primary key + tableComposite := "primaries_composite" + s.NoError(schema.Create(tableComposite, func(table contractsschema.Blueprint) { + table.String("name") + table.String("age") + table.Primary("name", "age") + })) + s.Require().True(schema.HasTable(tableComposite)) - s.NoError(schema.Create(table, func(table contractsschema.Blueprint) { + // Test single column primary key + tableSingle := "primaries_single" + s.NoError(schema.Create(tableSingle, func(table contractsschema.Blueprint) { table.String("name") - table.String("age") - table.Primary("name", "age") + table.Primary("name") })) + s.Require().True(schema.HasTable(tableSingle)) - s.Require().True(schema.HasTable(table)) - // TODO Open below when implementing index methods - //s.Require().True(schema.HasIndex(table, "primaries_pkey")) + // Test invalid column + tableInvalid := "primaries_invalid" + err := schema.Create(tableInvalid, func(table contractsschema.Blueprint) { + table.String("name") + table.Primary("invalid_column") + }) + s.Error(err) }) } }
96-101: Maintain consistent error handling pattern.The error handling could be more consistent with other tests in the suite. Consider using
s.Require().NoError()for critical setup steps.- s.NoError(schema.Create(table, func(table contractsschema.Blueprint) { + s.Require().NoError(schema.Create(table, func(table contractsschema.Blueprint) { table.String("name") table.String("age") table.Primary("name", "age") }))
103-104: Enhance TODO documentation with specific requirements.The TODO comment could be more descriptive about the dependencies and requirements for implementing index methods.
- // TODO Open below when implementing index methods + // TODO: Implement index verification after adding HasIndex method to the Schema interface + // Dependencies: + // 1. Implement HasIndex method in Schema interface + // 2. Add index information retrieval in the Grammar implementation + // Related issue: #280 //s.Require().True(schema.HasIndex(table, "primaries_pkey"))database/schema/blueprint_test.go (2)
139-142: Consider adding more test cases for edge scenarios.The test correctly verifies the basic index name generation. Consider adding test cases for:
- Empty column list
- Column names with other special characters
- Maximum length handling
func (s *BlueprintTestSuite) TestCreateIndexName() { name := s.blueprint.createIndexName("index", []string{"id", "name-1", "name.2"}) s.Equal("goravel_users_id_name_1_name_2_index", name) + + // Test empty column list + name = s.blueprint.createIndexName("index", []string{}) + s.Equal("goravel_users_index", name) + + // Test special characters + name = s.blueprint.createIndexName("index", []string{"user@name", "email#address"}) + s.Equal("goravel_users_user_name_email_address_index", name) }
167-185: Consider adding test cases for error scenarios.The test effectively covers the happy path for both basic and configured index creation. Consider adding test cases for:
- Invalid column names
- Empty column list
- Other IndexConfig fields (if any)
func (s *BlueprintTestSuite) TestIndexCommand() { s.blueprint.indexCommand("index", []string{"id", "name"}) s.Contains(s.blueprint.commands, &schema.Command{ Columns: []string{"id", "name"}, Name: "index", Index: "goravel_users_id_name_index", }) s.blueprint.indexCommand("index", []string{"id", "name"}, schema.IndexConfig{ Algorithm: "custom_algorithm", Name: "custom_name", }) s.Contains(s.blueprint.commands, &schema.Command{ Algorithm: "custom_algorithm", Columns: []string{"id", "name"}, Name: "index", Index: "custom_name", }) + + // Test empty column list + s.blueprint.indexCommand("index", []string{}) + s.Contains(s.blueprint.commands, &schema.Command{ + Columns: []string{}, + Name: "index", + Index: "goravel_users_index", + }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
mocks/database/schema/Blueprint.gois excluded by!mocks/**mocks/database/schema/Grammar.gois excluded by!mocks/**
📒 Files selected for processing (8)
contracts/database/schema/blueprint.go(2 hunks)contracts/database/schema/grammar.go(1 hunks)contracts/database/schema/schema.go(1 hunks)database/schema/blueprint.go(4 hunks)database/schema/blueprint_test.go(2 hunks)database/schema/constants/constants.go(1 hunks)database/schema/grammars/postgres.go(1 hunks)database/schema/schema_test.go(1 hunks)
🔇 Additional comments (8)
database/schema/constants/constants.go (1)
8-8: LGTM! The new constant follows established patterns.
The addition of CommandPrimary is well-structured:
- Follows the command naming convention
- Maintains alphabetical ordering
- Uses appropriate database terminology
Let's verify the removal of CommandChange constant and its usage:
contracts/database/schema/blueprint.go (1)
22-23: LGTM! Verify implementations across concrete types.
The Primary method signature is well-designed, allowing for both single and composite primary keys through variadic parameters.
Let's verify that all concrete types implementing the Blueprint interface have added this method:
✅ Verification successful
Let me try a different search strategy to find Blueprint implementations and Primary method.
Based on the search results, I can now generate the final response:
Primary method is properly implemented in concrete Blueprint type
The verification shows:
- The concrete implementation exists in
database/schema/blueprint.go:110 - Mock implementation exists for testing in
mocks/database/schema/Blueprint.go - Supporting grammar implementation for Postgres in
database/schema/grammars/postgres.go:61
The method is consistently implemented across the codebase with proper test coverage and database-specific handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all types that implement Blueprint interface and verify Primary method implementation
# Find all .go files that might contain Blueprint implementations
echo "Searching for potential Blueprint implementations..."
rg -l "struct.*Blueprint"
# Find the Primary method implementations
echo -e "\nSearching for Primary method implementations..."
ast-grep --pattern 'func ($x *$type) Primary($params) {
$$$
}'
Length of output: 320
Script:
#!/bin/bash
# First find the Blueprint interface definition to understand the package
echo "Finding Blueprint interface definition..."
rg -p "type Blueprint interface" -A 5
# Search for files importing the package containing Blueprint
echo -e "\nSearching for files importing Blueprint..."
rg -l "\".*database/schema\""
# Look for Primary method implementations
echo -e "\nSearching for Primary method implementations..."
rg -p "func.*Primary.*string"
Length of output: 2734
contracts/database/schema/grammar.go (1)
22-23: LGTM! Verify implementations across database drivers.
The new CompilePrimary method is a logical addition to the Grammar interface and follows the established pattern of compilation methods.
Let's verify that all database drivers implement this new interface method:
database/schema/grammars/postgres.go (1)
61-63: Verify handling of edge cases.
Consider adding validation for:
- Empty columns array to prevent invalid SQL
- Existing primary key constraints to avoid errors
Let's check how other implementations handle these cases:
database/schema/blueprint.go (4)
4-5: LGTM!
The addition of the strings package import is appropriate for the new string manipulation functionality.
110-112: LGTM!
The Primary method implementation is clean and follows the single responsibility principle by delegating to the indexCommand method.
146-147: LGTM!
The addition of the primary key compilation case follows the existing pattern and integrates well with the grammar interface.
199-215: LGTM!
The indexCommand method is well-structured with:
- Clear separation of concerns
- Proper handling of optional configuration
- Consistent command structure
📑 Description
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✅ Checks