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.

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

Increase size of the language column in language_stat #12396

Merged
merged 4 commits into from
Aug 4, 2020

Conversation

zeripath
Copy link
Contributor

@zeripath zeripath commented Aug 1, 2020

In #12379 it was discovered that enry v2 has a maximum language length
of 34 characters which is larger than the 30 previously provided.

This PR updates the language column to 50.

Fix #12379
Fix #13013

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

@zeripath zeripath added this to the 1.13.0 milestone Aug 1, 2020
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 1, 2020
@zeripath zeripath force-pushed the fix-12379-language_stats_50 branch from ed1cfd4 to a8d1265 Compare August 1, 2020 17:40
@zeripath
Copy link
Contributor Author

zeripath commented Aug 2, 2020

So actually thinking on, because sqlite doesn't need to change schema - varchar is mapped to text - I think we might be able to get away with just using ALTER TABLE in this case.

I'll keep this code though and add a doctor command that will allow you to recreate tables from the schema - that will finally allow us to be able to get rid of the weird warning messages about defaults having changed.

models/migrations/v102.go Outdated Show resolved Hide resolved
@zeripath zeripath force-pushed the fix-12379-language_stats_50 branch from 7bb96a0 to f6a4452 Compare August 2, 2020 10:50
In go-gitea#12379 it was discovered that enry v2 has a maximum language length
of 34 characters which is larger than the 30 previously provided.

This PR updates the language column to 50.

Fix go-gitea#12379

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath zeripath force-pushed the fix-12379-language_stats_50 branch from f6a4452 to 437ab15 Compare August 2, 2020 10:50
Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath
Copy link
Contributor Author

zeripath commented Aug 3, 2020

Finally I think I've solved (again) all the issues with the different dialects.

@zeripath zeripath requested a review from lunny August 3, 2020 07:47
Copy link
Member

@lafriks lafriks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice if this code could be moved to function like rename column but otherwise looks good

@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 Aug 3, 2020
@zeripath
Copy link
Contributor Author

zeripath commented Aug 3, 2020

@lafriks unfortunately a general solution is not very easy at all. In particular SQLite poses a particular problem here.

SQLite doesn't have a mechanism for changing the type of a column and you can't create a column with a unique not null constraint on an already existing table. So you're limited to copying the table and adding the column.

In reality the simplest thing to do is to recreate the table in its entirety from bean definition - I've provided that as another PR.

A general method that keeps the previous columns would be possible but I suspect that it may not be worth it.

@lunny
Copy link
Member

lunny commented Aug 4, 2020

Otherwise LGTM

@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 Aug 4, 2020
@zeripath
Copy link
Contributor Author

zeripath commented Aug 4, 2020

make lg-tm work

@zeripath zeripath merged commit 502e38c into go-gitea:master Aug 4, 2020
@zeripath zeripath deleted the fix-12379-language_stats_50 branch August 4, 2020 13:54
@silverwind
Copy link
Member

silverwind commented Aug 4, 2020

Hmm it seems this migration is giving me some trouble against MariaDB:

routers/init.go:71:initDBEngine() [I] ORM engine initialization attempt #3/10...
...rm/session_schema.go:25:Ping() [I] PING DATABASE mysql
...ations/migrations.go:310:Migrate() [I] Migration[145]: Increase Language field to 50 in LanguageStats
routers/init.go:77:initDBEngine() [E] ORM engine initialization attempt #3/10 failed. Error: migrate: do migrate: Error 1062: Duplicate entry 'JavaScript' for key 'UQE_language_stat_s'
routers/init.go:78:initDBEngine() [I] Backing off for 3 seconds
mysql  Ver 15.1 Distrib 10.4.13-MariaDB, for Linux (x86_64) using readline 5.1

@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
5 participants