Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Support converting varchar to nvarchar for mssql database #24105

Merged
merged 4 commits into from
Apr 17, 2023
Merged

Support converting varchar to nvarchar for mssql database #24105

merged 4 commits into from
Apr 17, 2023

Conversation

lunny
Copy link
Member

@lunny lunny commented Apr 13, 2023 •
edited by delvh
Loading

In #12269, all string fields of struct will generate a NVARCHAR column in database, but for those Gitea instances installed before that PR, users have to convert columns themselves.

In this PR, we update the ./gitea admin convert commands to support both MySQL and MSSQL database converting. Previously, it only supported converting utf8 -> utf8mb4 for MySQL.
Now, it will check the database types.
If it's MSSQL, it will convert VARCHAR -> NVARCHAR as well.

@lunny lunny added topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile outdated/backport/v1.19 This PR should be backported to Gitea 1.19 labels Apr 13, 2023
@lunny lunny added this to the 1.20.0 milestone Apr 13, 2023
@codecov-commenter
Copy link

codecov-commenter commented Apr 13, 2023 •
edited
Loading

Codecov Report

Merging #24105 (4a68cca) into main (f521e88) will decrease coverage by 0.03%.
The diff coverage is 39.70%.

@@            Coverage Diff             @@
##             main   #24105      +/-   ##
==========================================
- Coverage   47.14%   47.12%   -0.03%     
==========================================
  Files        1149     1162      +13     
  Lines      151446   153830    +2384     
==========================================
+ Hits        71397    72489    +1092     
- Misses      71611    72821    +1210     
- Partials     8438     8520      +82
Impacted Files Coverage Δ
cmd/convert.go 0.00% <0.00%> (ø)
cmd/dump.go 0.64% <0.00%> (-0.03%) ⬇️
cmd/embedded.go 0.00% <0.00%> (ø)
cmd/mailer.go 0.00% <0.00%> (ø)
cmd/manager.go 0.00% <0.00%> (ø)
cmd/manager_logging.go 0.00% <0.00%> (ø)
cmd/migrate_storage.go 5.76% <0.00%> (-0.12%) ⬇️
cmd/restore_repo.go 0.00% <0.00%> (ø)
cmd/web.go 0.00% <0.00%> (ø)
models/actions/run.go 1.63% <0.00%> (-0.10%) ⬇️
... and 58 more

... and 164 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 13, 2023
wolfogre
wolfogre approved these changes Apr 14, 2023
View reviewed changes
cmd/convert.go Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Apr 14, 2023
@lunny @wolfogre
Update cmd/convert.go
f135fad 
Co-authored-by: Jason Song <i@wolfogre.com>
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Apr 15, 2023
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Apr 17, 2023
@lunny lunny merged commit cda84be into go-gitea:main Apr 17, 2023
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Apr 17, 2023
@GiteaBot GiteaBot mentioned this pull request Apr 17, 2023
Merged
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Apr 17, 2023
@lunny @GiteaBot
Support converting varchar to nvarchar for mssql database (go-gitea#2…
1756854 
…4105)
@GiteaBot GiteaBot added the backport/done All backports for this PR have been created label Apr 17, 2023
@lunny lunny deleted the lunny/convert_varchar_nvarchar branch April 17, 2023 13:29
techknowlogick pushed a commit that referenced this pull request Apr 17, 2023
@GiteaBot @lunny
Support converting varchar to nvarchar for mssql database (#24105) (#…
c4f569b 
…24168)

Backport #24105 by @lunny

In #12269, all string fields of struct will generate a NVARCHAR column
in database, but for those Gitea instances installed before that PR,
users have to convert columns themselves.

In this PR, we update the `./gitea admin convert` commands to support
both MySQL and MSSQL database converting. Previously, it only supported
converting `utf8 -> utf8mb4` for MySQL.
Now, it will check the database types.
If it's MSSQL, it will convert `VARCHAR -> NVARCHAR` as well.

Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
zjjhot added a commit to zjjhot/gitea that referenced this pull request Apr 18, 2023
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
bab3026 
* giteaofficial/main: (25 commits)
  zh-cn support on doc pages (go-gitea#24166)
  [skip ci] Updated translations via Crowdin
  Use double quotes consistently in en-US (go-gitea#24141)
  Use correct locale key for forks page (go-gitea#24172)
  Improve Wiki TOC (go-gitea#24137)
  Localize activity heatmap (except tooltip) (go-gitea#24131)
  Support triggering workflows by wiki related events (go-gitea#24119)
  add CLI command to register runner tokens (go-gitea#23762)
  Add new user types `reserved`, `bot`, and `remote` (go-gitea#24026)
  Fix Org edit page bugs: renaming detection, maxlength (go-gitea#24161)
  Make HAS_GO a simply expanded variable (go-gitea#24169)
  Support converting varchar to nvarchar for mssql database (go-gitea#24105)
  Fix math and mermaid rendering bugs (go-gitea#24049)
  Refactor locale number (go-gitea#24134)
  [skip ci] Updated translations via Crowdin
  Use 1.18's aria role for dropdown menus (go-gitea#24144)
  Set EasyMDE heading font-size to the same size as the resulting markdown (go-gitea#24151)
  Fix 2-dot direct compare to use the right base commit (go-gitea#24133)
  Add migration to fix external unit access mode of owner/admin team (go-gitea#24117)
  Remove untranslatable `on_date` key (go-gitea#24106)
  ...
@edwardforgacs
Copy link

edwardforgacs commented May 3, 2023 •
edited
Loading

Not sure how thoroughly this was tested on SQL Server, but I got a lot of errors with the included tool and it appears to have some serious shortcomings (VARCHAR(MAX) doesn't work, and it doesn't quote the names). SQL Server also users "ALTER COLUMN" not "MODIFY". This is my modified query although I am still having errors due to unique index dependencies.

Not a major issue for us at this stage, I just thought it should be run as it was mentioned on the blog.

SELECT 'ALTER TABLE [' + OBJECT_NAME(SC.object_id) + '] ALTER COLUMN [' + SC.name + '] NVARCHAR(' +
CASE WHEN SC.max_length = -1 THEN 'MAX' ELSE CONVERT(VARCHAR(5), SC.max_length) END
+ ')'

FROM SYS.columns SC
JOIN SYS.types ST
ON SC.system_type_id = ST.system_type_id
AND SC.user_type_id = ST.user_type_id
WHERE ST.name ='varchar'

@lunny lunny mentioned this pull request May 3, 2023
Open
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Jul 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@yardenshoham yardenshoham yardenshoham approved these changes

@wolfogre wolfogre wolfogre approved these changes

@silverwind silverwind silverwind approved these changes

Assignees
No one assigned
Labels
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. outdated/backport/v1.19 This PR should be backported to Gitea 1.19 topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile
Projects
None yet
Milestone
1.20.0
Development

Successfully merging this pull request may close these issues.
7 participants
@lunny @codecov-commenter @edwardforgacs @silverwind @wolfogre @yardenshoham @GiteaBot