feat: [#280] Add Foreign methods #715
Conversation
WalkthroughThe changes in this pull request introduce several new methods and interfaces to enhance the functionality of the database schema management system. Key additions include new methods in 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 #715 +/- ##
==========================================
- Coverage 69.84% 69.77% -0.08%
==========================================
Files 198 198
Lines 15562 15628 +66
==========================================
+ Hits 10869 10904 +35
- Misses 4101 4132 +31
Partials 592 592 ☔ 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: 8
🧹 Outside diff range and nitpick comments (12)
contracts/database/schema/index.go (1)
3-14: Add documentation and consider additional foreign key options.The interface design is well-structured with a fluent API pattern and comprehensive foreign key actions. However, consider the following improvements:
- Add GoDoc comments for the interface and each method to document their purpose and behavior.
- Consider adding methods for additional foreign key options:
SetDefaultOnDelete()andSetDefaultOnUpdate()for SET DEFAULT actionDeferrable()andInitiallyDeferred()for deferrable constraintsExample documentation:
// ForeignKeyDefinition provides a fluent interface for configuring // foreign key constraints in database schemas. type ForeignKeyDefinition interface { // CascadeOnDelete configures the foreign key to cascade deletes // from the referenced table to the referencing table. CascadeOnDelete() ForeignKeyDefinition // ... (document other methods) }contracts/database/schema/blueprint.go (2)
14-15: Enhance method documentation with usage examples.While the comment describes the purpose, it would be more helpful to include:
- Parameter description
- Return value explanation
- Usage example
Consider expanding the documentation like this:
-// Foreign Specify a foreign key for the table. +// Foreign Specify a foreign key for the table. +// Parameters: +// - column: One or more column names to create the foreign key on +// Returns: +// - ForeignKeyDefinition: A builder to define the foreign key constraints +// Example: +// table.Foreign("user_id").References("id").On("users")
14-21: Consider additional foreign key management methods.The interface additions look good, but consider adding complementary methods for comprehensive foreign key management:
DropForeign- To remove foreign key constraintsHasForeign- To check if a foreign key existsGetForeignKeys- To retrieve all foreign keys defined on the tableThese methods would provide a complete API for foreign key lifecycle management. Would you like me to help draft the interface definitions for these methods?
database/schema/index.go (2)
7-15: LGTM with a suggestion for defensive programming.The type definition and constructor are well-implemented. Consider adding a nil check in the constructor for defensive programming.
func NewForeignKeyDefinition(command *schema.Command) schema.ForeignKeyDefinition { + if command == nil { + panic("command cannot be nil") + } return &ForeignKeyDefinition{ command: command, } }
17-27: Consider using constants for action values.The cascade methods are well-implemented, but using string literals for action values could lead to maintenance issues and inconsistencies.
+const ( + ActionCascade = "cascade" + ActionNoAction = "no action" + ActionSetNull = "set null" + ActionRestrict = "restrict" +) func (f *ForeignKeyDefinition) CascadeOnDelete() schema.ForeignKeyDefinition { - f.command.OnDelete = "cascade" + f.command.OnDelete = ActionCascade return f } func (f *ForeignKeyDefinition) CascadeOnUpdate() schema.ForeignKeyDefinition { - f.command.OnUpdate = "cascade" + f.command.OnUpdate = ActionCascade return f }database/schema/blueprint.go (2)
63-67: Add documentation and input validation.While the implementation follows the existing patterns, consider these improvements:
- Add godoc comments explaining the method's purpose and usage
- Add validation to ensure at least one column is provided
+// Foreign creates a new foreign key constraint on the table. +// It accepts one or more column names that will form the foreign key. func (r *Blueprint) Foreign(column ...string) schema.ForeignKeyDefinition { + if len(column) == 0 { + panic("at least one column is required for foreign key") + } command := r.indexCommand(constants.CommandForeign, column) return NewForeignKeyDefinition(command) }
Line range hint
63-163: Consider adding integration tests for foreign key functionality.Since this is part of a larger feature adding foreign key support, ensure that:
- Integration tests cover the full lifecycle of foreign key operations
- Documentation is added explaining the foreign key API usage
- Error cases are handled consistently across different database drivers
Would you like help creating integration tests or documentation for this feature?
database/schema/grammars/postgres.go (1)
60-75: Add input validation for required fields.The method should validate that required fields are not empty to prevent invalid SQL generation.
Add validation checks at the start of the method:
func (r *Postgres) CompileForeign(blueprint schema.Blueprint, command *schema.Command) string { + if command.Index == "" { + panic("Foreign key constraint name is required") + } + if len(command.Columns) == 0 { + panic("Foreign key columns are required") + } + if command.On == "" { + panic("Referenced table is required") + } + if len(command.References) == 0 { + panic("Referenced columns are required") + } + if len(command.Columns) != len(command.References) { + panic("Number of foreign key columns must match number of referenced columns") + } sql := fmt.Sprintf("alter table %s add constraint %s foreign key (%s) references %s (%s)",database/schema/schema_test.go (1)
91-93: Consider improving test isolation.While the test correctly uses the existing test infrastructure, it should ensure proper isolation between different database drivers. Consider wrapping the test in a transaction that can be rolled back.
for driver, testQuery := range s.driverToTestQuery { s.Run(driver.String(), func() { + // Start transaction + tx := testQuery.Query().Begin() + defer tx.Rollback() + schema := GetTestSchema(testQuery, s.driverToTestQuery)database/schema/grammars/postgres_test.go (3)
104-131: Consider adding more test cases for better coverage.While the current test cases cover basic scenarios, consider adding:
- Edge cases with multiple columns in the foreign key
- Negative test cases (e.g., invalid references)
- Error cases validation
Example additional test cases:
{ name: "with multiple columns", command: &contractsschema.Command{ Index: "fk_users_company_department", Columns: []string{"company_id", "department_id"}, On: "departments", References: []string{"company_id", "id"}, OnDelete: "cascade", }, expectSql: "alter table users add constraint fk_users_company_department foreign key (company_id, department_id) references goravel_departments (company_id, id) on delete cascade", }, { name: "with invalid reference columns count", command: &contractsschema.Command{ Index: "fk_invalid", Columns: []string{"role_id"}, On: "roles", References: []string{"id", "extra_id"}, }, expectError: "number of referencing columns must match referenced columns", }
98-102: Enhance mock verification and cleanup.Consider adding mock verification to ensure all expected calls were made and cleaning up between test cases:
func (s *PostgresSuite) TestCompileForeign() { var mockBlueprint *mocksschema.Blueprint beforeEach := func() { mockBlueprint = mocksschema.NewBlueprint(s.T()) mockBlueprint.EXPECT().GetPrefix().Return("goravel_").Once() mockBlueprint.EXPECT().GetTableName().Return("users").Once() } + afterEach := func() { + // Verify that all expected mock calls were made + s.True(mockBlueprint.AssertExpectations(s.T())) + } // ... test cases ... for _, test := range tests { s.Run(test.name, func() { beforeEach() + defer afterEach() sql := s.grammar.CompileForeign(mockBlueprint, test.command) s.Equal(test.expectSql, sql) }) } }Also applies to: 133-141
137-139: Consider enhancing SQL validation.The current SQL validation using string equality is basic. Consider:
- Adding component-wise validation to check individual parts of the SQL statement
- Using case-insensitive comparison for SQL keywords
- Validating SQL syntax structure
Example enhanced validation:
func (s *PostgresSuite) validateForeignKeySQL(sql string, expected string) { // Case-insensitive comparison for SQL keywords normalizedSQL := strings.ToUpper(sql) normalizedExpected := strings.ToUpper(expected) s.Equal(normalizedExpected, normalizedSQL) // Component-wise validation s.Contains(sql, "ALTER TABLE") s.Contains(sql, "ADD CONSTRAINT") s.Contains(sql, "FOREIGN KEY") s.Contains(sql, "REFERENCES") // Validate basic SQL syntax structure parts := strings.Split(sql, " ") s.Greater(len(parts), 8, "SQL should have minimum required parts") }Then use it in the test:
-s.Equal(test.expectSql, sql) +s.validateForeignKeySQL(sql, test.expectSql)
📜 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/ForeignKeyDefinition.gois excluded by!mocks/**mocks/database/schema/Grammar.gois excluded by!mocks/**
📒 Files selected for processing (9)
contracts/database/schema/blueprint.go(1 hunks)contracts/database/schema/grammar.go(1 hunks)contracts/database/schema/index.go(1 hunks)database/schema/blueprint.go(3 hunks)database/schema/constants/constants.go(1 hunks)database/schema/grammars/postgres.go(1 hunks)database/schema/grammars/postgres_test.go(1 hunks)database/schema/index.go(1 hunks)database/schema/schema_test.go(1 hunks)
🔇 Additional comments (9)
database/schema/constants/constants.go (1)
8-8: LGTM! The constant addition follows the established pattern.
The new CommandForeign constant maintains consistency with existing command naming conventions and is properly positioned in alphabetical order.
Let's verify its usage in the codebase:
✅ Verification successful
The constant is properly integrated and used consistently
The verification confirms that CommandForeign is:
- Correctly defined in constants.go
- Used appropriately in Blueprint's foreign key implementation
- Referenced in the SQL compilation logic
- No instances of hardcoded "foreign" string that should be using the constant instead
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of CommandForeign constant
# Expected: Should find references in Blueprint's Foreign method and SQL generation
# Search for direct usage of the constant
rg -A 3 "CommandForeign"
# Search for string literal "foreign" to ensure we're not missing any places
# where we should be using the constant
rg -A 3 '"foreign"'
Length of output: 1153
contracts/database/schema/blueprint.go (1)
20-21: Enhance GetPrefix documentation and verify its usage.
The current documentation lacks context about the purpose and importance of table prefixes.
Consider expanding the documentation:
-// GetPrefix Get the table prefix.
+// GetPrefix Get the table prefix.
+// Returns the configured prefix for database tables, which is used for:
+// - Multi-tenant database organization
+// - Namespace isolation
+// - Database schema organization
+// Returns an empty string if no prefix is configured.Let's verify how this prefix is being used:
contracts/database/schema/grammar.go (2)
18-19: LGTM! Well-designed interface addition.
The new CompileForeign method follows the established interface patterns and maintains consistency with existing compilation methods in terms of naming, parameter types, and return values.
18-19: Consider enhancing method documentation and ensure test coverage.
While the method signature is clear, consider expanding the documentation to include:
- Parameter descriptions (what the Blueprint and Command should contain)
- Return value description (expected SQL format)
- Any specific constraints or requirements for foreign key compilation
Also, noting from the PR objectives that test cases are pending. Please ensure comprehensive test coverage for this new interface method across all implementations.
Let's check for existing test patterns:
database/schema/index.go (2)
41-51: LGTM!
The NoAction methods are well-implemented and follow the established pattern.
65-75: LGTM!
The Restrict methods are well-implemented and follow the established pattern.
database/schema/blueprint.go (1)
82-84: LGTM!
Simple and correct implementation of a getter method.
database/schema/grammars/postgres.go (1)
60-75: Consider adding unit tests.
Based on the PR objectives mentioning unchecked test cases, ensure comprehensive test coverage for the new CompileForeign method.
Let's verify if tests exist:
Would you like me to help generate comprehensive test cases for this method? The tests should cover:
- Basic foreign key creation
- Multiple column foreign keys
- ON DELETE/UPDATE actions
- Invalid input handling
- Identifier escaping
database/schema/grammars/postgres_test.go (1)
95-103: LGTM! Well-structured test setup.
The test follows good practices with a clear setup using beforeEach and proper mock initialization.
📑 Description
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
✅ Checks