xorm/xorm
xorm/xorm
63
356
Fork 206
Code Issues 370 Pull Requests 18 Actions 9 Releases 35 Activity

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

Merged
lunny merged 22 commits from zeripath/xorm:fix-1782-MSSQL-nvarchar-fixes into master 2020-09-08 01:40:09 +00:00
Conversation 8 Commits 22 Files Changed 5 +104 -4
zeripath commented 2020-09-04 21:37:29 +00:00
Contributor
Copy Link

sys.columns sets the max_length as double that which it really is for NVARCHAR and NCHAR.

Also sets Text, MediumText and LongText as NVARCHAR(MAX).

Adds a drone test running on MSSSQL tests with NVARCHAR and adjusts test cases to force them to fail with #1782

Fix #1782

sys.columns sets the max_length as double that which it really is for NVARCHAR and NCHAR. Also sets Text, MediumText and LongText as NVARCHAR(MAX). Adds a drone test running on MSSSQL tests with NVARCHAR and adjusts test cases to force them to fail with #1782 Fix #1782
zeripath added 5 commits 2020-09-04 21:37:33 +00:00
Add failing test 
Signed-off-by: Andrew Thornton <art27@cantab.net>
011fcf5c47
schemas.Text should map to db.defaultVarchar 
Signed-off-by: Andrew Thornton <art27@cantab.net>
65547a343c
Fix length issue 
Signed-off-by: Andrew Thornton <art27@cantab.net>
558822b6d7
Set defaultVarchar to upper case 
Signed-off-by: Andrew Thornton <art27@cantab.net>
a966473595
add nvarchar as a testcase 
Signed-off-by: Andrew Thornton <art27@cantab.net>
Some checks failed
continuous-integration/drone/pr Build is failing
Details
fafa19feca
zeripath added 1 commit 2020-09-04 21:44:56 +00:00
add depends_on 
Signed-off-by: Andrew Thornton <art27@cantab.net>
ffaa02ec28
zeripath added 1 commit 2020-09-04 21:47:24 +00:00
add depends_on (2) 
Signed-off-by: Andrew Thornton <art27@cantab.net>
All checks were successful
continuous-integration/drone/pr Build is passing
Details
3a7eec0c08
zeripath commented 2020-09-04 22:27:07 +00:00
Author
Contributor
Copy Link

Hmm adding drone for mssql-nvarchar significantly lengthens your CI - I'm not certain if you want to do that.

I could just add a few choice mssql specific tests?

Hmm adding drone for mssql-nvarchar significantly lengthens your CI - I'm not certain if you want to do that. I could just add a few choice mssql specific tests?
zeripath added 1 commit 2020-09-05 08:38:41 +00:00
Remove the duplicate mssql drone test and add a specific sync test 
Signed-off-by: Andrew Thornton <art27@cantab.net>
Some checks failed
continuous-integration/drone/pr Build is failing
Details
9b384f6f8d
zeripath commented 2020-09-05 08:40:02 +00:00
Author
Contributor
Copy Link

OK I've removed the test-mssql-nvarchar drone task and added another sync test that can be switched on or off with a flag and sets the params as appropriate.

OK I've removed the test-mssql-nvarchar drone task and added another sync test that can be switched on or off with a flag and sets the params as appropriate.
zeripath added 1 commit 2020-09-05 09:39:39 +00:00
fix test 
Signed-off-by: Andrew Thornton <art27@cantab.net>
Some checks failed
continuous-integration/drone/pr Build is failing
Details
61503d4db0
zeripath added 1 commit 2020-09-05 10:18:06 +00:00
prod CI
All checks were successful
continuous-integration/drone/pr Build is passing
Details
922604eaad
lunny reviewed 2020-09-05 11:39:41 +00:00
Dismissed
dialects/mssql.go Outdated
@ -424,8 +424,14 @@ func (db *mssql) GetColumns(queryer core.Queryer, ctx context.Context, tableName
col.SQLType = schemas.SQLType{Name: schemas.TimeStampz, DefaultLength: 0, DefaultLength2: 0}
case "NVARCHAR":
col.SQLType = schemas.SQLType{Name: schemas.NVarchar, DefaultLength: 0, DefaultLength2: 0}
col.Length /= 2
lunny commented 2020-09-05 11:39:41 +00:00
Owner
Copy Link

Why we need that?

Why we need that?
zeripath commented 2020-09-05 20:25:00 +00:00
Author
Contributor
Copy Link

https://social.microsoft.com/Forums/Windows/en-US/be4ad98b-473a-4183-a541-71c3fd79d958/length-of-nvarchar-column-in-system-tables?forum=transactsql

There is a column in sys.columns named max_length. This is (for almost all datatypes)
the maximum length of the column in bytes. So a column that is declared to be
varchar(10) will show 10 in that column. A column that is nvarchar(10) will show 20 in
that column since each nvarchar character takes 2 bytes. If the value of the
max_length is -1 then the column was declared as varchar(max), nvarchar(max), or
varbinary(max).

So potentially there is an issue with /2 in the case of MAXs but the test appears to work for TEXT

https://social.microsoft.com/Forums/Windows/en-US/be4ad98b-473a-4183-a541-71c3fd79d958/length-of-nvarchar-column-in-system-tables?forum=transactsql > There is a column in sys.columns named max_length. This is (for almost all datatypes) > the maximum length of the column in bytes. So a column that is declared to be > varchar(10) will show 10 in that column. A column that is nvarchar(10) will show 20 in > that column since each nvarchar character takes 2 bytes. If the value of the > max_length is -1 then the column was declared as varchar(max), nvarchar(max), or > varbinary(max). So potentially there is an issue with /2 in the case of MAXs but the test appears to work for TEXT
zeripath commented 2020-09-05 20:53:38 +00:00
Author
Contributor
Copy Link

hehe - it worked for TEXT because I hadn't tested it... Now... it works for TEXT.

hehe - it worked for TEXT because I hadn't tested it... Now... it works for TEXT.
zeripath commented 2020-09-05 20:29:45 +00:00
Author
Contributor
Copy Link

I've just realised I haven't tested the Text or Char places I'll get another few commits off

I've just realised I haven't tested the Text or Char places I'll get another few commits off
zeripath added 1 commit 2020-09-05 20:30:34 +00:00
Add tests for Test and Char columns 
Signed-off-by: Andrew Thornton <art27@cantab.net>
Some checks failed
continuous-integration/drone/pr Build is failing
Details
c825309e5b
zeripath added 1 commit 2020-09-05 20:50:21 +00:00
Properly handle MAX 
Signed-off-by: Andrew Thornton <art27@cantab.net>
Some checks failed
continuous-integration/drone/pr Build is failing
Details
749818bddd
zeripath added 1 commit 2020-09-05 21:04:29 +00:00
prod CI
Some checks failed
continuous-integration/drone/pr Build is failing
Details
e6f2cb3daa
zeripath added 1 commit 2020-09-05 21:34:27 +00:00
prod CI
Some checks failed
continuous-integration/drone/pr Build is failing
Details
d48af2bb07
zeripath commented 2020-09-05 21:45:07 +00:00
Author
Contributor
Copy Link

Last CI failures are not related to this PR.

Last CI failures are not related to this PR.
zeripath added 1 commit 2020-09-05 21:45:41 +00:00
prod CI
Some checks failed
continuous-integration/drone/pr Build is failing
Details
f38a19fb2b
lunny commented 2020-09-06 05:10:59 +00:00
Owner
Copy Link

test-postgres and test-cockroach failures are related.

`test-postgres` and `test-cockroach` failures are related.
zeripath added 1 commit 2020-09-06 11:33:36 +00:00
Add a few more column testcases in light of postgres weirdness
Some checks failed
continuous-integration/drone/pr Build is failing
Details
fc44a96cfa
zeripath commented 2020-09-06 11:34:07 +00:00
Author
Contributor
Copy Link

Ah the changes in TestSyncTable3 have revealed a similar bug in postgres. I'm gonna push a few more test cases up in case they reveal any more interesting information.

Ah the changes in TestSyncTable3 have revealed a similar bug in postgres. I'm gonna push a few more test cases up in case they reveal any more interesting information.
zeripath added 1 commit 2020-09-06 11:45:40 +00:00
fix test
Some checks failed
continuous-integration/drone/pr Build is failing
Details
d52e8e7e8d
zeripath added 1 commit 2020-09-06 12:07:12 +00:00
Postgres (and cockroachDB by inheritance) maps Char to Varchar 
Signed-off-by: Andrew Thornton <art27@cantab.net>
All checks were successful
continuous-integration/drone/pr Build is passing
Details
2f656b2211
zeripath commented 2020-09-06 12:09:31 +00:00
Author
Contributor
Copy Link

OK it appears that postgres and cockroachdb map Char(n)/NChar(n) as Varchar(n) because there is no benefit for char so I've just forciby mapped Char as Varchar

OK it appears that postgres and cockroachdb map Char(n)/NChar(n) as Varchar(n) because there is no benefit for char so I've just forciby mapped Char as Varchar
zeripath added 1 commit 2020-09-06 12:28:04 +00:00
Merge branch 'master' into fix-1782-MSSQL-nvarchar-fixes
All checks were successful
continuous-integration/drone/pr Build is passing
Details
b2c14d04ac
lunny reviewed 2020-09-06 13:29:25 +00:00
Dismissed
dialects/postgres.go
@ -857,6 +857,10 @@ func (db *postgres) SQLType(c *schemas.Column) string {
res = schemas.Real
case schemas.TinyText, schemas.MediumText, schemas.LongText:
res = schemas.Text
case schemas.Char:
lunny commented 2020-09-06 13:29:25 +00:00
Owner
Copy Link

I don't think we should convert Char to Varchar since postgres support CHAR type. We just need

case schemas.NChar:
    res = schemas.Char
I don't think we should convert Char to Varchar since postgres support `CHAR` type. We just need ``` case schemas.NChar: res = schemas.Char ```
zeripath marked this conversation as resolved
zeripath added 2 commits 2020-09-06 16:32:11 +00:00
map "character" to Char for postgres 
Signed-off-by: Andrew Thornton <art27@cantab.net>
5729222a0e
zeripath commented 2020-09-06 16:33:49 +00:00
Author
Contributor
Copy Link

OK I've changed it instead. When dialect.GetColumns(...) reads a column with "character" it will get mapped to "CHAR" SQL type.

OK I've changed it instead. When `dialect.GetColumns(...)` reads a column with "character" it will get mapped to "CHAR" SQL type.
zeripath added 1 commit 2020-09-06 16:56:59 +00:00
prod CI
All checks were successful
continuous-integration/drone/pr Build is passing
Details
ee850f9407
lunny approved these changes 2020-09-08 01:36:36 +00:00
Dismissed
lunny merged commit b422930617 into master 2020-09-08 01:37:00 +00:00
lunny added the
kind
bug
 label 2020-09-08 01:37:16 +00:00
lunny added this to the 1.0.5 milestone 2020-09-08 01:37:22 +00:00
zeripath deleted branch fix-1782-MSSQL-nvarchar-fixes 2020-09-08 19:32:55 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
backport/done

   
backport/v1

   
blocked
  
db
oracle

  
db
sqlserver

  
duplicate
  
feature
cache

  
frontport/done

   
frontport/main

   
invalid
  
kind
breaking

  
kind
bug

  
kind
build

  
kind
dependencies

  
kind
docs

  
kind
driver

  
kind
enhancement

  
kind
feature

  
kind
performance

  
kind
proposal

  
kind
question

  
kind
refactor

  
kind
testing

  
need
feedback

  
need
test

  
proposal:accepted

   
RaspBerry Pi

   
regression

   
skip-changelog

   
upstream

   
wip

   
wontfix
No Label
backport/done
backport/v1
blocked
db
oracle
db
sqlserver
duplicate
feature
cache
frontport/done
frontport/main
invalid
kind
breaking
kind
bug
kind
build
kind
dependencies
kind
docs
kind
driver
kind
enhancement
kind
feature
kind
performance
kind
proposal
kind
question
kind
refactor
kind
testing
need
feedback
need
test
proposal:accepted
RaspBerry Pi
regression
skip-changelog
upstream
wip
wontfix
Milestone
Clear milestone
No items
No Milestone
1.0.5
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: xorm/xorm#1783
No description provided.
Delete Branch "zeripath/xorm:fix-1782-MSSQL-nvarchar-fixes"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?