feat: sqlserver driver supports setting schema #786
Conversation
WalkthroughThe pull request introduces enhancements to schema management across various database systems, particularly focusing on SQL Server. Key changes include the addition of new test functions for SQL Server and PostgreSQL, the refinement of existing test methods, and improvements to schema-related functionality. New types and methods are introduced to support these changes, ensuring consistent behavior in handling schemas and migrations across different database types. Changes
Sequence DiagramsequenceDiagram
participant Client
participant SchemaManager
participant Database
Client->>SchemaManager: Create Schema
SchemaManager->>Database: Verify Schema Existence
Database-->>SchemaManager: Schema Check Result
SchemaManager->>Database: Create Tables in Schema
Database-->>SchemaManager: Table Creation Confirmation
SchemaManager-->>Client: Schema and Tables Ready
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 #786 +/- ##
==========================================
- Coverage 69.72% 69.68% -0.05%
==========================================
Files 215 215
Lines 18456 18523 +67
==========================================
+ Hits 12869 12907 +38
- Misses 4882 4911 +29
Partials 705 705 ☔ 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: 4
🧹 Nitpick comments (6)
database/schema/grammars/wrap_test.go (3)
24-30: Unify Aliased and Non-Aliased Column Logic Cleanly
These unified test assertions are clear and comprehensive. However, consider using sub-tests (e.g.,t.Run("alias", ...),t.Run("no_alias", ...)) for improved organization and clarity.
39-45: Good Consolidation of Quote Tests
Combining non-empty and empty quote checks into a single method is efficient, but consider clarifying function-level comments to distinguish these scenarios if the test logic grows more complex in the future.
63-73: Extended Table Wrapping for Schema and Alias
The test covers multiple scenarios (alias, schema, no alias) under a single function. This approach is succinct. If more table-specific scenarios are added, splitting into sub-tests may further improve maintainability.database/schema/schema.go (1)
221-227: Schema-Aware Table Detection
The updatedHasTablemethod now correctly parses schema-qualified table names by splitting on the dot (if present). This is a sensible enhancement for multi-schema support.Consider logging or validating the extracted schema name if future usage might require more robust error handling (e.g., blank or unexpected schema substrings).
Also applies to: 238-240
database/gorm/test_utils.go (1)
1264-1277: MySQL schema creation
Defining a table calledgoravel.schemascan be confusing in MySQL. MySQL doesn’t typically use “schema.table” naming. Verify if you actually need a “.” in the table name for MySQL.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 1266-1276: database/gorm/test_utils.go#L1266-L1276
Added lines #L1266 - L1276 were not covered by testsdatabase/schema/grammars/wrap.go (1)
92-92: Split and wrap multi-part table names
If the table string contains a dot, the code now separates and wraps the left side and the prefix-plus-right side. For more complex scenarios (multiple dots), consider validating or handling additional segments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
database/gorm/query_test.go(2 hunks)database/gorm/test_models.go(1 hunks)database/gorm/test_utils.go(6 hunks)database/migration/default_migrator_test.go(5 hunks)database/schema/grammars/wrap.go(1 hunks)database/schema/grammars/wrap_test.go(2 hunks)database/schema/schema.go(3 hunks)database/schema/schema_test.go(3 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
database/gorm/test_utils.go
[warning] 233-233: database/gorm/test_utils.go#L233
Added line #L233 was not covered by tests
[warning] 584-588: database/gorm/test_utils.go#L584-L588
Added lines #L584 - L588 were not covered by tests
[warning] 1266-1276: database/gorm/test_utils.go#L1266-L1276
Added lines #L1266 - L1276 were not covered by tests
[warning] 1278-1285: database/gorm/test_utils.go#L1278-L1285
Added lines #L1278 - L1285 were not covered by tests
[warning] 1287-1294: database/gorm/test_utils.go#L1287-L1294
Added lines #L1287 - L1294 were not covered by tests
[warning] 1306-1307: database/gorm/test_utils.go#L1306-L1307
Added lines #L1306 - L1307 were not covered by tests
🔇 Additional comments (26)
database/schema/grammars/wrap_test.go (2)
34-34: Columnize Implementation Looks Good
This change is straightforward and aligns well with the unified test structure.
77-88: Value Wrapping Enhanced
Combining asterisk, non-asterisk, and MySQL driver checks is helpful. The commentary block is nicely separated. Everything looks good.
database/schema/schema.go (1)
6-6: New Import for Handling Schema Logic
Importing strings is necessary here for detecting schema dots. This is adequately justified by the changes below.
database/gorm/test_models.go (1)
423-431: New Schema Type Definition
Adding the Schema struct and TableName method is a straightforward way to group schema data under a standardized table. This fosters better organization for test or production use.
database/migration/default_migrator_test.go (4)
165-167: Renamed Postgres Schema Test
Renaming the test function improves clarity on its specific database target. Looks good.
185-187: Explicit Rollback Verification
Verifying that the users table no longer exists after rollback is essential for robust tests. Good inclusion.
189-210: Added SQL Server Schema Test
Introducing TestDefaultMigratorWithSqlserverSchema significantly improves coverage for SQL Server support. Ensuring the table name is prefixed with goravel. and validating rollback further ensures correctness.
966-987: Dedicated SQL Server Migration and Rollback
The TestMigrationWithSqlserverSchema structure is clean and clearly demonstrates the creation and removal of [schema].[table]. Validations appear comprehensive.
database/gorm/test_utils.go (13)
31-31: Add new enum constant
The addition of TestTableSchema is clear and aligns with the new schema table logic.
219-219: Check driver configuration call
Switching to mockDriver.Common() may remove prefix/singular logic. Ensure that this is intentional and doesn't introduce regressions for schema usage.
229-229: Initialization looks good
Assigning query is standard practice. This snippet is straightforward.
236-239: Return early for SQL Server
Skipping WithSchema for SQL Server is consistent with the approach of building the schema connection differently. The fallback logic is acceptable.
240-253: Build new query with schema
Re-initializing mocks and calling WithSchema is a solid approach for applying the schema logic to Postgres (and other drivers that support it).
260-260: Avoid creating TestTableSchema by default
Skipping the schema table unless explicitly requested helps prevent extraneous table creation in tests.
584-588: User-provided schema in WithSchema
Allowing a custom schema is good, but the fallback "public" is questionable for SQL Server. Consider clarifying the correct default for that driver.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 584-588: database/gorm/test_utils.go#L584-L588
Added lines #L584 - L588 were not covered by tests
634-634: Map TestTableSchema to schema creation
Including the new TestTableSchema logic in All() ensures that schema creation is fully supported in tests.
1278-1286: Postgres table name
goravel.schemas in Postgres might conflict with the notion of “schema.tablename.” If the intent is to store everything in the default schema while referencing “schemas” as a table name, this is okay, but be aware of potential confusion.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 1278-1285: database/gorm/test_utils.go#L1278-L1285
Added lines #L1278 - L1285 were not covered by tests
1287-1295: SQLite table creation
The structure for a new goravel.schemas table is consistent with SQLite’s schema. Nothing obviously problematic here.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 1287-1294: database/gorm/test_utils.go#L1287-L1294
Added lines #L1287 - L1294 were not covered by tests
1296-1305: SQL Server table creation
Similarly, goravel.schemas is fine for naming, but ensure naming does not conflict with established patterns.
1306-1307: Default case
Returning an empty string is consistent with not creating a table on unsupported drivers.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 1306-1307: database/gorm/test_utils.go#L1306-L1307
Added lines #L1306 - L1307 were not covered by tests
1309-1309: Function closing bracket
No issue with the function’s closure.
database/schema/schema_test.go (3)
4-4: Added import for “fmt”
This likely supports formatted output in new tests. That’s appropriate.
Line range hint 2335-2361: Renamed TestSpecificSchema → TestPostgresSchema
This rename clarifies the target driver. The new test focuses specifically on Postgres, which increases clarity.
2363-2387: New TestSqlserverSchema function
Introducing a dedicated test for SQL Server schemas significantly improves coverage for that driver. Good addition.
database/gorm/query_test.go (2)
3941-3941: Added PostgreSQL schema test
Using NewTestQueryWithSchema for the “goravel” schema is an effective way to confirm schema-based logic in Postgres.
3961-3979: Added SQL Server schema test
This test parallels the Postgres test and ensures that schema creation and lookups work as expected on SQL Server. Good for cross-database consistency.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
database/gorm/test_utils.go (2)
236-238: Document SQL Server schema behavior.The early return for SQL Server needs documentation explaining why the schema configuration is skipped, as this behavior differs from PostgreSQL.
Add a comment explaining the SQL Server schema behavior:
if docker.Driver() == contractsdatabase.DriverSqlserver { + // SQL Server schemas are handled differently from PostgreSQL + // as they don't require explicit search_path configuration return testQuery }
1264-1309: Review hardcoded schema name in table definitions.The schema table definitions use a hardcoded "goravel" schema prefix. Consider making this configurable to support different test scenarios.
Consider parameterizing the schema name:
-CREATE TABLE goravel.schemas ( +CREATE TABLE %s.schemas (Then update the schema() method to accept and use the schema parameter:
-func (r *testTables) schema() string { +func (r *testTables) schema(schemaName string) string { switch r.driver { case contractsdatabase.DriverMysql: - return `CREATE TABLE goravel.schemas ...` + return fmt.Sprintf(`CREATE TABLE %s.schemas ...`, schemaName)🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 1266-1276: database/gorm/test_utils.go#L1266-L1276
Added lines #L1266 - L1276 were not covered by tests
[warning] 1278-1285: database/gorm/test_utils.go#L1278-L1285
Added lines #L1278 - L1285 were not covered by tests
[warning] 1287-1294: database/gorm/test_utils.go#L1287-L1294
Added lines #L1287 - L1294 were not covered by tests
[warning] 1306-1307: database/gorm/test_utils.go#L1306-L1307
Added lines #L1306 - L1307 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
database/gorm/test_utils.go(6 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
database/gorm/test_utils.go
[warning] 233-233: database/gorm/test_utils.go#L233
Added line #L233 was not covered by tests
[warning] 584-588: database/gorm/test_utils.go#L584-L588
Added lines #L584 - L588 were not covered by tests
[warning] 1266-1276: database/gorm/test_utils.go#L1266-L1276
Added lines #L1266 - L1276 were not covered by tests
[warning] 1278-1285: database/gorm/test_utils.go#L1278-L1285
Added lines #L1278 - L1285 were not covered by tests
[warning] 1287-1294: database/gorm/test_utils.go#L1287-L1294
Added lines #L1287 - L1294 were not covered by tests
[warning] 1306-1307: database/gorm/test_utils.go#L1306-L1307
Added lines #L1306 - L1307 were not covered by tests
🔇 Additional comments (3)
database/gorm/test_utils.go (3)
31-31: LGTM: New test table constant follows convention.
The addition of TestTableSchema constant follows the existing enumeration pattern.
232-234: Schema creation uses proper identifier quoting.
The schema creation SQL properly quotes the schema name to prevent SQL injection and handle special characters.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 233-233: database/gorm/test_utils.go#L233
Added line #L233 was not covered by tests
553-554: LGTM: SQL Server uses correct default schema.
The mock driver correctly uses "dbo" as the default schema for SQL Server, which follows SQL Server conventions.
Also applies to: 569-570, 577-578
| } | ||
|
|
||
| mockDriver.WithSchema(schema) | ||
| if _, err := query.Exec(fmt.Sprintf(`CREATE SCHEMA "%s"`, schema)); err != nil { | ||
| panic(fmt.Sprintf("create schema %s failed: %v", schema, err)) |
There was a problem hiding this comment.
💡 Codebase verification
Test coverage for schema operations is adequate, but error scenarios need attention
Based on the verification results, I found that:
-
Schema creation and basic operations are well-tested for both PostgreSQL and SQL Server in:
database/schema/schema_test.godatabase/gorm/query_test.go
-
However, error scenarios are not adequately covered:
- The panic in
NewTestQueryWithSchemafor unsupported drivers is tested implicitly - Schema creation error handling (line 233) lacks explicit test coverage
- SQL Server schema configuration (lines 584-588) has basic tests but no error scenarios
- Schema table definitions (lines 1266-1307) have functional tests but lack edge cases
- The panic in
Recommendations:
- Add test cases for schema creation failures
- Include error scenarios for SQL Server schema operations
- Add edge cases for schema table definitions with invalid inputs
🔗 Analysis chain
Improve test coverage for schema operations.
Several critical code paths lack test coverage:
- Schema creation error handling
- SQL Server schema configuration
- Schema table definitions for different databases
Let's verify the existing test coverage:
Would you like me to help create additional test cases to cover these scenarios?
Also applies to: 584-588, 1266-1307
Let's gather more information about the test coverage for schema operations.
Let's check the actual test implementations to verify the coverage of error scenarios.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for existing schema-related tests
# Look for test files that might contain schema-related tests
rg -l "TestSchema|test.*Schema|schema.*Test" --type go
Length of output: 349
Script:
#!/bin/bash
# Check for test cases related to schema creation and error handling
rg "func.*Test.*Schema.*" --type go -A 5
# Check for specific schema creation error tests
rg "TestSchema.*Error|TestCreate.*Schema.*Error" --type go -A 5
# Look for SQL Server schema configuration tests
rg "TestWithSchema|TestSQLServer.*Schema" --type go -A 5
# Check schema table definition tests
rg "TestSchema.*Table|TestCreateTable" --type go -A 5
Length of output: 6846
Script:
#!/bin/bash
# Check the content of schema-related tests
rg "TestPostgresSchema|TestSqlserverSchema" --type go -B 2 -A 20
# Look for panic handling in tests
rg "panic.*schema" --type go -B 2 -A 2
# Check schema creation error handling in tests
rg "create.*schema.*error|schema.*create.*error" --type go -i -B 2 -A 2
Length of output: 7973
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 233-233: database/gorm/test_utils.go#L233
Added line #L233 was not covered by tests
📑 Description
Summary by CodeRabbit
New Features
Schematype to represent schemas in the database.Bug Fixes
HasTablemethod.Documentation
Tests
✅ Checks