-
-
Notifications
You must be signed in to change notification settings - Fork 80
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixed table schema using with SQL Server. #30
Fixed table schema using with SQL Server. #30
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ekhabarov!
Thanks a lot for your PR! A few comments and this is good to go
sqlserver.go
Outdated
// Splitter is a BatchSplitter interface implementation. We need it for | ||
// SQL Server because commands like a `CREATE SCHEMA...` and a `CREATE TABLE...` | ||
// could not be executed in the same batch. | ||
// See https://docs.microsoft.com/en-us/previous-versions/sql/sql-server-2008-r2/ms175502(v=sql.105)#rules-for-using-batches |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ouch!
I think I should move the schema files to either a function or an YAML file (so arrays are allowed to run multiple statements) to prevent logic like this
But for this PR this is good enough as is, we can do that later
testdata/schema/sqlserver.sql
Outdated
@@ -18,6 +21,15 @@ CREATE TABLE tags ( | |||
,created_at DATETIME NOT NULL | |||
,updated_at DATETIME NOT NULL | |||
); | |||
GO | |||
|
|||
CREATE SCHEMA x AUTHORIZATION dbo; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we rename x
to something more descriptive, like non_default_schema
?
testdata/schema/sqlserver.sql
Outdated
GO | ||
|
||
CREATE TABLE x.empty_table_without_fixtures ( | ||
id INT IDENTITY PRIMARY KEY |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep indentation with tabs for now, to be consistent with the rest of the file
testfixtures_test.go
Outdated
@@ -52,14 +51,25 @@ func TestLoadFixtures(t *testing.T) { | |||
log.Fatalf("Failed to ping database: %v\n", err) | |||
} | |||
|
|||
bytes, err = ioutil.ReadFile(database.schemaFile) | |||
batches := [][]byte{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var batches [][]byte
testfixtures_test.go
Outdated
if err != nil { | ||
log.Fatalf("Failed to create schema: %v\n", err) | ||
h, ok := database.helper.(BatchSplitter) | ||
if ok { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if h, ok := database.helper.(BatchSplitter); ok {
testfixtures_test.go
Outdated
|
||
for _, b := range batches { | ||
_, err = db.Exec(string(b)) | ||
if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if _, err = db.Exec(string(b)); err != nil {
helper.go
Outdated
// For Microsoft SQL Server batch splitter is "GO". For details see | ||
// https://docs.microsoft.com/en-us/sql/t-sql/language-elements/sql-server-utilities-statements-go | ||
type BatchSplitter interface { | ||
Splitter() []byte |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about moving the interface and method to lower case? I don't think this need to be exported
Hi @andreynering! Fixed. |
@ekhabarov Thanks! 🙂 |
Hi!
This PR fixes next bug. If you create any table in database in SQL Server within schema different to
dbo
, for instancex.empty_table_without_fixtures
, and run tests you'll get an error... because function
tableName
does not support table schema.