Skip to content

Commit 8eb1d30

Browse files
dhuidhui
authored
Merge pull request #409 from alethenorio/spanner-comments
Add support for comments in Spanner migrations
2 parents 8b6e3f1 + e6faa29 commit 8eb1d30

File tree

5 files changed

+173
-73
lines changed

5 files changed

+173
-73
lines changed

‎database/spanner/README.md‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ See [Google Spanner Documentation](https://cloud.google.com/spanner/docs) for de
1212
| Param | WithInstance Config | Description |
1313
| ----- | ------------------- | ----------- |
1414
| `x-migrations-table` | `MigrationsTable` | Name of the migrations table |
15+
| `x-clean-statements` | `CleanStatements` | Whether to parse and clean DDL statements before running migration towards Spanner (Required for comments and multiple statements) |
1516
| `url` | `DatabaseName` | The full path to the Spanner database resource. If provided as part of `Config` it must not contain a scheme or query string to match the format `projects/{projectId}/instances/{instanceId}/databases/{databaseName}`|
1617
| `projectId` || The Google Cloud Platform project id
1718
| `instanceId` || The id of the instance running Spanner
@@ -28,6 +29,15 @@ See [Google Spanner Documentation](https://cloud.google.com/spanner/docs) for de
2829
> 1496601752/u add_index_on_user_emails (2m12.155787369s)
2930
> 1496602638/u create_books_table (2m30.77299181s)
3031
32+
## DDL with comments
33+
34+
At the moment the GCP Spanner backed does not seem to allow for comments (See https://issuetracker.google.com/issues/159730604)
35+
so in order to be able to use migration with DDL containing comments `x-clean-stamements` is required
36+
37+
## Multiple statements
38+
39+
In order to be able to use more than 1 DDL statement in the same migration file, the file has to be parsed and therefore the `x-clean-statements` flag is required
40+
3141
## Testing
3242
3343
To unit test the `spanner` driver, `SPANNER_DATABASE` needs to be set. You'll

‎database/spanner/spanner.go‎

Lines changed: 37 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -7,12 +7,14 @@ import (
77
"log"
88
nurl "net/url"
99
"regexp"
10+
"strconv"
1011
"strings"
1112

1213
"golang.org/x/net/context"
1314

1415
"cloud.google.com/go/spanner"
1516
sdb "cloud.google.com/go/spanner/admin/database/apiv1"
17+
"cloud.google.com/go/spanner/spansql"
1618

1719
"github.com/golang-migrate/migrate/v4"
1820
"github.com/golang-migrate/migrate/v4/database"
@@ -42,6 +44,11 @@ var (
4244
type Config struct {
4345
MigrationsTable string
4446
DatabaseName string
47+
// Whether to parse the migration DDL with spansql before
48+
// running them towards Spanner.
49+
// Parsing outputs clean DDL statements such as reformatted
50+
// and void of comments.
51+
CleanStatements bool
4552
}
4653

4754
// Spanner implements database.Driver for Google Cloud Spanner
@@ -110,10 +117,20 @@ func (s *Spanner) Open(url string) (database.Driver, error) {
110117

111118
migrationsTable := purl.Query().Get("x-migrations-table")
112119

120+
cleanQuery := purl.Query().Get("x-clean-statements")
121+
clean := false
122+
if cleanQuery != "" {
123+
clean, err = strconv.ParseBool(cleanQuery)
124+
if err != nil {
125+
return nil, err
126+
}
127+
}
128+
113129
db := &DB{admin: adminClient, data: dataClient}
114130
return WithInstance(db, &Config{
115131
DatabaseName: dbname,
116132
MigrationsTable: migrationsTable,
133+
CleanStatements: clean,
117134
})
118135
}
119136

@@ -141,10 +158,15 @@ func (s *Spanner) Run(migration io.Reader) error {
141158
return err
142159
}
143160

144-
// run migration
145-
stmts := migrationStatements(migr)
146-
ctx := context.Background()
161+
stmts := []string{string(migr)}
162+
if s.config.CleanStatements {
163+
stmts, err = cleanStatements(migr)
164+
if err != nil {
165+
return err
166+
}
167+
}
147168

169+
ctx := context.Background()
148170
op, err := s.db.admin.UpdateDatabaseDdl(ctx, &adminpb.UpdateDatabaseDdlRequest{
149171
Database: s.config.DatabaseName,
150172
Statements: stmts,
@@ -302,17 +324,17 @@ func (s *Spanner) ensureVersionTable() (err error) {
302324
return nil
303325
}
304326

305-
func migrationStatements(migration []byte) []string {
306-
migrationString := string(migration[:])
307-
migrationString = strings.TrimSpace(migrationString)
308-
309-
allStatements := strings.Split(migrationString, ";")
310-
nonEmptyStatements := allStatements[:0]
311-
for _, s := range allStatements {
312-
s = strings.TrimSpace(s)
313-
if s != "" {
314-
nonEmptyStatements = append(nonEmptyStatements, s)
315-
}
327+
func cleanStatements(migration []byte) ([]string, error) {
328+
// The Spanner GCP backend does not yet support comments for the UpdateDatabaseDdl RPC
329+
// (see https://issuetracker.google.com/issues/159730604) we use
330+
// spansql to parse the DDL and output valid stamements without comments
331+
ddl, err := spansql.ParseDDL("", string(migration))
332+
if err != nil {
333+
return nil, err
334+
}
335+
stmts := make([]string, 0, len(ddl.List))
336+
for _, stmt := range ddl.List {
337+
stmts = append(stmts, stmt.SQL())
316338
}
317-
return nonEmptyStatements
339+
return stmts, nil
318340
}

‎database/spanner/spanner_test.go‎

Lines changed: 60 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -4,16 +4,14 @@ import (
44
"fmt"
55
"os"
66
"testing"
7-
)
87

9-
import (
108
"github.com/golang-migrate/migrate/v4"
9+
1110
dt "github.com/golang-migrate/migrate/v4/database/testing"
12-
_ "github.com/golang-migrate/migrate/v4/source/file"
13-
)
1411

15-
import (
12+
_ "github.com/golang-migrate/migrate/v4/source/file"
1613
"github.com/stretchr/testify/assert"
14+
"github.com/stretchr/testify/require"
1715
)
1816

1917
func Test(t *testing.T) {
@@ -58,71 +56,97 @@ func TestMigrate(t *testing.T) {
5856
dt.TestMigrate(t, m)
5957
}
6058

61-
func TestMultistatementSplit(t *testing.T) {
59+
func TestCleanStatements(t *testing.T) {
6260
testCases := []struct {
6361
name string
6462
multiStatement string
6563
expected []string
6664
}{
6765
{
68-
name: "single statement, single line, no semicolon",
66+
name: "no statement",
67+
multiStatement: "",
68+
expected: []string{},
69+
},
70+
{
71+
name: "single statement, single line, no semicolon, no comment",
6972
multiStatement: "CREATE TABLE table_name (id STRING(255) NOT NULL) PRIMARY KEY (id)",
70-
expected: []string{"CREATE TABLE table_name (id STRING(255) NOT NULL) PRIMARY KEY (id)"},
73+
expected: []string{"CREATE TABLE table_name (\n id STRING(255) NOT NULL,\n) PRIMARY KEY(id)"},
7174
},
7275
{
73-
name: "single statement, multi line, no semicolon",
76+
name: "single statement, multi line, no semicolon, no comment",
7477
multiStatement: `CREATE TABLE table_name (
75-
id STRING(255) NOT NULL,
76-
) PRIMARY KEY (id)`,
77-
expected: []string{`CREATE TABLE table_name (
78-
id STRING(255) NOT NULL,
79-
) PRIMARY KEY (id)`},
78+
id STRING(255) NOT NULL,
79+
) PRIMARY KEY (id)`,
80+
expected: []string{"CREATE TABLE table_name (\n id STRING(255) NOT NULL,\n) PRIMARY KEY(id)"},
8081
},
8182
{
82-
name: "single statement, single line, with semicolon",
83+
name: "single statement, single line, with semicolon, no comment",
8384
multiStatement: "CREATE TABLE table_name (id STRING(255) NOT NULL) PRIMARY KEY (id);",
84-
expected: []string{"CREATE TABLE table_name (id STRING(255) NOT NULL) PRIMARY KEY (id)"},
85+
expected: []string{"CREATE TABLE table_name (\n id STRING(255) NOT NULL,\n) PRIMARY KEY(id)"},
8586
},
8687
{
87-
name: "single statement, multi line, with semicolon",
88+
name: "single statement, multi line, with semicolon, no comment",
8889
multiStatement: `CREATE TABLE table_name (
89-
id STRING(255) NOT NULL,
90-
) PRIMARY KEY (id);`,
90+
id STRING(255) NOT NULL,
91+
) PRIMARY KEY (id);`,
92+
expected: []string{"CREATE TABLE table_name (\n id STRING(255) NOT NULL,\n) PRIMARY KEY(id)"},
93+
},
94+
{
95+
name: "multi statement, with trailing semicolon. no comment",
96+
// From https://github.com/mattes/migrate/pull/281
97+
multiStatement: `CREATE TABLE table_name (
98+
id STRING(255) NOT NULL,
99+
) PRIMARY KEY(id);
100+
101+
CREATE INDEX table_name_id_idx ON table_name (id);`,
102+
expected: []string{`CREATE TABLE table_name (
103+
id STRING(255) NOT NULL,
104+
) PRIMARY KEY(id)`, "CREATE INDEX table_name_id_idx ON table_name(id)"},
105+
},
106+
{
107+
name: "multi statement, no trailing semicolon, no comment",
108+
// From https://github.com/mattes/migrate/pull/281
109+
multiStatement: `CREATE TABLE table_name (
110+
id STRING(255) NOT NULL,
111+
) PRIMARY KEY(id);
112+
113+
CREATE INDEX table_name_id_idx ON table_name (id)`,
91114
expected: []string{`CREATE TABLE table_name (
92-
id STRING(255) NOT NULL,
93-
) PRIMARY KEY (id)`},
115+
id STRING(255) NOT NULL,
116+
) PRIMARY KEY(id)`, "CREATE INDEX table_name_id_idx ON table_name(id)"},
94117
},
95118
{
96-
name: "multi statement, with trailing semicolon",
119+
name: "multi statement, no trailing semicolon, standalone comment",
97120
// From https://github.com/mattes/migrate/pull/281
98121
multiStatement: `CREATE TABLE table_name (
99-
id STRING(255) NOT NULL,
100-
) PRIMARY KEY(id);
122+
-- standalone comment
123+
id STRING(255) NOT NULL,
124+
) PRIMARY KEY(id);
101125
102-
CREATE INDEX table_name_id_idx ON table_name (id);`,
126+
CREATE INDEX table_name_id_idx ON table_name (id)`,
103127
expected: []string{`CREATE TABLE table_name (
104-
id STRING(255) NOT NULL,
105-
) PRIMARY KEY(id)`, "CREATE INDEX table_name_id_idx ON table_name (id)"},
128+
id STRING(255) NOT NULL,
129+
) PRIMARY KEY(id)`, "CREATE INDEX table_name_id_idx ON table_name(id)"},
106130
},
107131
{
108-
name: "multi statement, no trailing semicolon",
132+
name: "multi statement, no trailing semicolon, inline comment",
109133
// From https://github.com/mattes/migrate/pull/281
110134
multiStatement: `CREATE TABLE table_name (
111-
id STRING(255) NOT NULL,
112-
) PRIMARY KEY(id);
135+
id STRING(255) NOT NULL, -- inline comment
136+
) PRIMARY KEY(id);
113137
114-
CREATE INDEX table_name_id_idx ON table_name (id)`,
138+
CREATE INDEX table_name_id_idx ON table_name (id)`,
115139
expected: []string{`CREATE TABLE table_name (
116-
id STRING(255) NOT NULL,
117-
) PRIMARY KEY(id)`, "CREATE INDEX table_name_id_idx ON table_name (id)"},
140+
id STRING(255) NOT NULL,
141+
) PRIMARY KEY(id)`, "CREATE INDEX table_name_id_idx ON table_name(id)"},
118142
},
119143
}
120144

121145
for _, tc := range testCases {
122146
t.Run(tc.name, func(t *testing.T) {
123-
if stmts := migrationStatements([]byte(tc.multiStatement)); !assert.Equal(t, stmts, tc.expected) {
124-
t.Error()
125-
}
147+
stmts, err := cleanStatements([]byte(tc.multiStatement))
148+
require.NoError(t, err, "Error cleaning statements")
149+
assert.Equal(t, tc.expected, stmts)
126150
})
127151
}
128152
}

‎go.mod‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,8 @@
11
module github.com/golang-migrate/migrate/v4
22

33
require (
4-
cloud.google.com/go/spanner v1.5.0
5-
cloud.google.com/go/storage v1.6.0
4+
cloud.google.com/go/spanner v1.6.0
5+
cloud.google.com/go/storage v1.8.0
66
github.com/ClickHouse/clickhouse-go v1.3.12
77
github.com/Microsoft/go-winio v0.4.14 // indirect
88
github.com/aws/aws-sdk-go v1.17.7
@@ -40,10 +40,10 @@ require (
4040
github.com/xdg/stringprep v1.0.0 // indirect
4141
gitlab.com/nyarla/go-crypt v0.0.0-20160106005555-d9a5dc2b789b // indirect
4242
go.mongodb.org/mongo-driver v1.1.0
43-
golang.org/x/net v0.0.0-20200324143707-d3edc9973b7e
44-
golang.org/x/tools v0.0.0-20200413015812-1f08ef6002a8
45-
google.golang.org/api v0.21.0
46-
google.golang.org/genproto v0.0.0-20200413115906-b5235f65be36
43+
golang.org/x/net v0.0.0-20200520182314-0ba52f642ac2
44+
golang.org/x/tools v0.0.0-20200522201501-cb1345f3a375
45+
google.golang.org/api v0.25.0
46+
google.golang.org/genproto v0.0.0-20200526151428-9bb895338b15
4747
modernc.org/b v1.0.0 // indirect
4848
modernc.org/db v1.0.0 // indirect
4949
modernc.org/file v1.0.0 // indirect

0 commit comments

Comments
 (0)