MSSQL Sync2 reports incorrect db struct types for self-generated tables #1782

New Issue

zeripath · 2020-09-04T18:44:16Z

zeripath commented

2020-09-04 18:44:16 +00:00

Gitea uses engine.Dialect().SetParams(map[string]string{"DEFAULT_VARCHAR": "nvarchar"}) meaning that its tables, (created with Sync2) will be set to nvarchar.

The example table User:

type User struct {
		LowerName string `xorm:"UNIQUE NOT NULL"`
}

Will cause Sync2 when ran twice to report:

Table user column lower_name db type is NVARCHAR(510), struct type is nvarchar(255)

Gitea uses `engine.Dialect().SetParams(map[string]string{"DEFAULT_VARCHAR": "nvarchar"})` meaning that its tables, (created with Sync2) will be set to nvarchar. The example table User: ```go type User struct { LowerName string `xorm:"UNIQUE NOT NULL"` } ``` Will cause Sync2 when ran twice to report: ``` Table user column lower_name db type is NVARCHAR(510), struct type is nvarchar(255) ```

zeripath commented

2020-09-04 19:10:21 +00:00

The following patch adjusts the TestSyncTable unit test to replicate the problem.

Applying the following patch and set TEST_MSSQL_DEFAULT_VARCHAR=nvarchar then run make test-mssql:

From 011fcf5c474af1eb1dc6e074199f6eec5740d82b Mon Sep 17 00:00:00 2001
From: Andrew Thornton <art27@cantab.net>
Date: Fri, 4 Sep 2020 20:06:46 +0100
Subject: [PATCH] Add failing test

Signed-off-by: Andrew Thornton <art27@cantab.net>

diff --git a/integrations/session_schema_test.go b/integrations/session_schema_test.go
index 005b661..e40bf3b 100644
--- a/integrations/session_schema_test.go
+++ b/integrations/session_schema_test.go
@@ -102,6 +102,9 @@ func TestSyncTable(t *testing.T) {
 	assert.NoError(t, err)
 	assert.EqualValues(t, 1, len(tables))
 	assert.EqualValues(t, "sync_table1", tables[0].Name)
+	tableInfo, err := testEngine.TableInfo(new(SyncTable1))
+	assert.NoError(t, err)
+	assert.EqualValues(t, testEngine.Dialect().SQLType(tables[0].GetColumn("name")), testEngine.Dialect().SQLType(tableInfo.GetColumn("name")))
 
 	assert.NoError(t, testEngine.Sync2(new(SyncTable2)))
 
@@ -109,6 +112,9 @@ func TestSyncTable(t *testing.T) {
 	assert.NoError(t, err)
 	assert.EqualValues(t, 1, len(tables))
 	assert.EqualValues(t, "sync_table1", tables[0].Name)
+	tableInfo, err = testEngine.TableInfo(new(SyncTable2))
+	assert.NoError(t, err)
+	assert.EqualValues(t, testEngine.Dialect().SQLType(tables[0].GetColumn("name")), testEngine.Dialect().SQLType(tableInfo.GetColumn("name")))
 
 	assert.NoError(t, testEngine.Sync2(new(SyncTable3)))
 
@@ -116,6 +122,9 @@ func TestSyncTable(t *testing.T) {
 	assert.NoError(t, err)
 	assert.EqualValues(t, 1, len(tables))
 	assert.EqualValues(t, "sync_table1", tables[0].Name)
+	tableInfo, err = testEngine.TableInfo(new(SyncTable3))
+	assert.NoError(t, err)
+	assert.EqualValues(t, testEngine.Dialect().SQLType(tables[0].GetColumn("name")), testEngine.Dialect().SQLType(tableInfo.GetColumn("name")))
 }
 
 func TestSyncTable2(t *testing.T) {
-- 
2.25.1

The following patch adjusts the TestSyncTable unit test to replicate the problem. Applying the following patch and set `TEST_MSSQL_DEFAULT_VARCHAR=nvarchar` then run `make test-mssql`: ```patch From 011fcf5c474af1eb1dc6e074199f6eec5740d82b Mon Sep 17 00:00:00 2001 From: Andrew Thornton <art27@cantab.net> Date: Fri, 4 Sep 2020 20:06:46 +0100 Subject: [PATCH] Add failing test Signed-off-by: Andrew Thornton <art27@cantab.net> diff --git a/integrations/session_schema_test.go b/integrations/session_schema_test.go index 005b661..e40bf3b 100644 --- a/integrations/session_schema_test.go +++ b/integrations/session_schema_test.go @@ -102,6 +102,9 @@ func TestSyncTable(t *testing.T) { assert.NoError(t, err) assert.EqualValues(t, 1, len(tables)) assert.EqualValues(t, "sync_table1", tables[0].Name) + tableInfo, err := testEngine.TableInfo(new(SyncTable1)) + assert.NoError(t, err) + assert.EqualValues(t, testEngine.Dialect().SQLType(tables[0].GetColumn("name")), testEngine.Dialect().SQLType(tableInfo.GetColumn("name"))) assert.NoError(t, testEngine.Sync2(new(SyncTable2))) @@ -109,6 +112,9 @@ func TestSyncTable(t *testing.T) { assert.NoError(t, err) assert.EqualValues(t, 1, len(tables)) assert.EqualValues(t, "sync_table1", tables[0].Name) + tableInfo, err = testEngine.TableInfo(new(SyncTable2)) + assert.NoError(t, err) + assert.EqualValues(t, testEngine.Dialect().SQLType(tables[0].GetColumn("name")), testEngine.Dialect().SQLType(tableInfo.GetColumn("name"))) assert.NoError(t, testEngine.Sync2(new(SyncTable3))) @@ -116,6 +122,9 @@ func TestSyncTable(t *testing.T) { assert.NoError(t, err) assert.EqualValues(t, 1, len(tables)) assert.EqualValues(t, "sync_table1", tables[0].Name) + tableInfo, err = testEngine.TableInfo(new(SyncTable3)) + assert.NoError(t, err) + assert.EqualValues(t, testEngine.Dialect().SQLType(tables[0].GetColumn("name")), testEngine.Dialect().SQLType(tableInfo.GetColumn("name"))) } func TestSyncTable2(t *testing.T) { -- 2.25.1 ```

zeripath commented

2020-09-04 19:16:20 +00:00

Actually I've just noticed that schemas.Text, MediumText etc. should use defaultVarchar not schemas.Varchar

From 65547a343c5bb8a8109674c1e050b43ab326dea1 Mon Sep 17 00:00:00 2001
From: Andrew Thornton <art27@cantab.net>
Date: Fri, 4 Sep 2020 20:14:30 +0100
Subject: [PATCH 2/2] schemas.Text should map to db.defaultVarchar

Signed-off-by: Andrew Thornton <art27@cantab.net>
---
 dialects/mssql.go | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/dialects/mssql.go b/dialects/mssql.go
index d76a8c6..328369d 100644
--- a/dialects/mssql.go
+++ b/dialects/mssql.go
@@ -285,7 +285,7 @@ func (db *mssql) SQLType(c *schemas.Column) string {
 	case schemas.MediumInt:
 		res = schemas.Int
 	case schemas.Text, schemas.MediumText, schemas.TinyText, schemas.LongText, schemas.Json:
-		res = schemas.Varchar + "(MAX)"
+		res = db.defaultVarchar + "(MAX)"
 	case schemas.Double:
 		res = schemas.Real
 	case schemas.Uuid:
-- 
2.25.1

Actually I've just noticed that schemas.Text, MediumText etc. should use defaultVarchar not schemas.Varchar ```patch From 65547a343c5bb8a8109674c1e050b43ab326dea1 Mon Sep 17 00:00:00 2001 From: Andrew Thornton <art27@cantab.net> Date: Fri, 4 Sep 2020 20:14:30 +0100 Subject: [PATCH 2/2] schemas.Text should map to db.defaultVarchar Signed-off-by: Andrew Thornton <art27@cantab.net> --- dialects/mssql.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dialects/mssql.go b/dialects/mssql.go index d76a8c6..328369d 100644 --- a/dialects/mssql.go +++ b/dialects/mssql.go @@ -285,7 +285,7 @@ func (db *mssql) SQLType(c *schemas.Column) string { case schemas.MediumInt: res = schemas.Int case schemas.Text, schemas.MediumText, schemas.TinyText, schemas.LongText, schemas.Json: - res = schemas.Varchar + "(MAX)" + res = db.defaultVarchar + "(MAX)" case schemas.Double: res = schemas.Real case schemas.Uuid: -- 2.25.1 ```

zeripath referenced a pull request that will close this issue

2020-09-04 21:37:29 +00:00

Fix warnings with schema Sync2 with default varchar as NVARCHAR #1783

lunny added the

kind

bug

label 2020-09-05 11:43:45 +00:00

lunny added this to the 1.0.5 milestone 2020-09-05 11:43:49 +00:00

lunny closed this issue

2020-09-08 01:37:01 +00:00

Sign in to join this conversation.