Converts columns from NULL to NOT NULL DEFAULT #18871

KN4CK3R · 2022-02-23T20:53:32Z

This PR converts columns from NULL to NOT NULL DEFAULT. This should concern only the tests because the fixtures insert NULL values. The usual xorm.Inserts do not insert NULL values for int and bool.

models/migrations/v211.go

…l-fields

codecov-commenter · 2022-03-08T15:43:34Z

Codecov Report

Merging #18871 (de70352) into main (1546580) will decrease coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main   #18871      +/-   ##
==========================================
- Coverage   46.60%   46.58%   -0.03%     
==========================================
  Files         854      854              
  Lines      122891   122891              
==========================================
- Hits        57271    57243      -28     
- Misses      58701    58734      +33     
+ Partials     6919     6914       -5

Impacted Files	Coverage Δ
models/issue.go	`53.84% <ø> (ø)`
models/issue_label.go	`65.05% <ø> (ø)`
models/issue_milestone.go	`67.62% <ø> (ø)`
models/org_team.go	`54.33% <ø> (ø)`
models/repo/attachment.go	`50.33% <ø> (ø)`
models/repo/repo.go	`63.04% <ø> (ø)`
models/repo/topic.go	`61.53% <ø> (ø)`
models/user/user.go	`57.97% <ø> (ø)`
modules/queue/queue_bytefifo.go	`46.90% <0.00%> (-5.46%)`	⬇️
modules/charset/charset.go	`69.74% <0.00%> (-4.21%)`	⬇️
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1546580...de70352. Read the comment docs.

models/migrations/migrations.go

…l-fields

KN4CK3R · 2022-05-02T19:45:11Z

I checked the models in the migration just now. Changes are still valid.

models/migrations/v214.go

lunny · 2022-09-19T08:42:20Z

conflicted.

…l-fields

6543

blocking as recreating whole database is not feasable for big instances like...

gitea.com, codeberg.org, ...

lafriks · 2022-09-27T08:53:27Z

Yes this can be done using alter table set default and alter table set not null for all databases except sqlite3

…l-fields

wxiaoguang · 2022-10-13T04:07:34Z

blocking as recreating whole database is not feasable for big instances like...

gitea.com, codeberg.org, ...

I would against about this point. Gitea is not only for codeberg, it's for everyone.

In history, Gitea (gogs)'s code base is not ideal, there will be a lot of refactoring to make Gitea more "right". One instance should not be the blocker for a general purpose project.

For the people who need to customize Gitea a lot, they could alter the database without downtime ahead and skip the migration manually.

Update: this comment is a general thought, not for this PR. For this PR itself, I haven't fully understand the problem and have some worries and added a new comment.

lafriks · 2022-10-13T09:09:58Z

I would against about this point. Gitea is not only for codeberg, it's for everyone.

In history, Gitea (gogs)'s code base is not ideal, there will be a lot of refactoring to make Gitea more "right". One instance should not be the blocker for a general purpose project.

For the people who need to customize Gitea a lot, they could alter the database without downtime ahead and skip the migration manually.

But why do you think that there could not be other large private instances?

wxiaoguang · 2022-10-13T09:24:20Z

I would against about this point. Gitea is not only for codeberg, it's for everyone.
In history, Gitea (gogs)'s code base is not ideal, there will be a lot of refactoring to make Gitea more "right". One instance should not be the blocker for a general purpose project.
For the people who need to customize Gitea a lot, they could alter the database without downtime ahead and skip the migration manually.

But why do you think that there could not be other large private instances?

There could be, IMO they could either take a downtime or apply the changes manually?

wxiaoguang · 2022-10-16T15:50:40Z

Although I think it's good to make the database columns more strict and said "I couldn't agree with that 'database migrations should be blocked by large instances'", I just mean a general thought. Sorry that I didn't say clearly in first time.

For this PR I have some worries (I haven't thought about these problems carefully, just questions):

Does it need a full test for all databases to make sure the migration works well?
Does the x.Exec("UPDATE ... need to be merged into one UPDATE SET col1=...,col2=... to speed up for large tables?
Does the DEFAULT false or UPDATE SET col=false(default) work for all databases? IIRC XORM does some tricks for bool/true/false

One more question, if this PR is mainly for tests and it's difficult to make the migration clear or too much time-consuming, could there be an alternative approach to only make tests correct while keeping the database structure unchanged?

KN4CK3R · 2022-10-16T16:10:19Z

Does it need a full test for all databases to make sure the migration works well?

How would a "full test" look like?

Does the x.Exec("UPDATE ... need to be merged into one UPDATE SET col1=...,col2=... to speed up for large tables?

That's "not" possible because of the WHERE statement (should not update col1 if col2 is null). A different solution could be col1 = ISNULL(...) but I don't know if that's better or supported.

Does the DEFAULT false or UPDATE SET col=false(default) work for all databases? IIRC XORM does some tricks for bool/true/false

We already have lots of DEFAULT false/true statements so I think that works?!

One more question, if this PR is mainly for tests and it's difficult to make the migration clear, could there be an alternative approach to only make tests correct while keeping the database structure unchanged?

Sure, we would have to add all fields to the fixtures (or enforce default values somehow). Then there would be no change to the database which is a pro or a con...?

wxiaoguang · 2022-10-16T16:20:04Z

Does it need a full test for all databases to make sure the migration works well?

How would a "full test" look like?

Just a question, not sure whether "tests/integration/migration-test" covers all cases.

Does the DEFAULT false or UPDATE SET col=false(default) work for all databases? IIRC XORM does some tricks for bool/true/false

We already have lots of DEFAULT false/true statements so I think that works?!

I recalled some reviews mentioned that "do not write col=true/false" because it would cause broken SQL, I am not sure whether it's a real problem, so just ask the question. Update: checked with XORM code, it will change the column.Default to dialect values. So no problem

One more question, if this PR is mainly for tests and it's difficult to make the migration clear, could there be an alternative approach to only make tests correct while keeping the database structure unchanged?

Sure, we would have to add all fields to the fixtures (or enforce default values somehow). Then there would be no change to the database which is a pro or a con...?

Just a thought, maybe there could be some "default fixtures" during testing, which contains all default values for the fields, then all loaded fixtures first get filled by default fixtures, then load the real fixture data.

Update: Pros: no changes to database. Cons: may lead to inconsistent test results ..... So maybe it's not a good idea ... in the end the database should have strict structure .......

lunny · 2022-10-16T16:35:54Z

@lunny Looks like xorm.ColumnString generates an invalid command for mssql. In this case it's ALTER TABLE [label] ALTER COLUMN [num_issues] INT DEFAULT 0 NOT NULL.

Is this a bug of xorm?

wxiaoguang · 2022-10-16T16:35:58Z

Does the x.Exec("UPDATE ... need to be merged into one UPDATE SET col1=...,col2=... to speed up for large tables?

That's "not" possible because of the WHERE statement (should not update col1 if col2 is null). A different solution could be col1 = ISNULL(...) but I don't know if that's better or supported.

Since most columns do not have an index, so most UPDATE will be a full-table scan. So the all-in-one UPDATE might be: UPDATE ... SET col1=COALESCE(col1, DefaultValue), ... (without the WHERE)

lunny · 2022-10-16T16:39:37Z

It's safer to update all NULL values to non NULL values before change the columns to NOT NULL, otherwise it will fail when upgrading.

wxiaoguang · 2022-10-16T16:41:35Z

It's safer to update all NULL values to non NULL values before change the columns to NOT NULL, otherwise it will fail when upgrading.

Yup, that's what this PR has done, first UPDATE, then modifyColumns

wxiaoguang · 2022-10-16T17:10:32Z

Hmm ..... after some more thinking, my personal conclusion at the moment is:

I support making database structure strict if it's very very necessary.
For tests, the fixtures could be fixed by adding missing fields (although it needs some work)
If this PR is only for test purpose, maybe we can leave it until there is a more strong requirement for it, because this PR only adds "NOT NULL" to some columns. If we take it, in the future, maybe another similar PR will appear to add more "NOT NULL" for other columns, which is pretty annoying (as a non-native speaker, I can not find another word for it 😁 ) ....

So I would propose to leave this PR as it is, fix the fixtures for Add some api integration tests #18872 manually. I could also help if it's needed.

depends on #18871 Added some api integration tests to help testing of #18798. Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com> Co-authored-by: wxiaoguang <wxiaoguang@gmail.com> Co-authored-by: zeripath <art27@cantab.net> Co-authored-by: techknowlogick <techknowlogick@gitea.io>

puni9869 · 2023-09-01T05:58:21Z

@KN4CK3R are you currently working on this? we can close this PR.

KN4CK3R · 2023-09-01T06:03:51Z

Not working on this because it was finished back then. It's still relevant but not an easy migration for existing large instances.

puni9869 · 2023-09-01T06:10:38Z

Any forward plans for this. As piece by piece migration. There are lots of migration we need

Converts columns from NULL to NOT NULL DEFAULT.

eb3ec0f

KN4CK3R added the type/testing label Feb 23, 2022

KN4CK3R mentioned this pull request Feb 23, 2022

Add some api integration tests #18872

Merged

Fixed unit tests.

b8dd4f2

singuliere reviewed Feb 24, 2022

View reviewed changes

models/migrations/v211.go Outdated Show resolved Hide resolved

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 24, 2022

Merge branch 'main' of https://github.com/go-gitea/gitea into fix-nul…

3688b8d

…l-fields

KN4CK3R mentioned this pull request Mar 1, 2022

Send 404 on /{org}.gpg #18959

Merged

Merge branch 'main' into fix-null-fields

de70352

6543 reviewed Mar 8, 2022

View reviewed changes

models/migrations/migrations.go Outdated Show resolved Hide resolved

KN4CK3R added 2 commits March 23, 2022 19:26

Merge branch 'main' of https://github.com/go-gitea/gitea into fix-nul…

0f7fe3a

…l-fields

Merge branch 'main' of https://github.com/go-gitea/gitea into fix-nul…

fa3065e

…l-fields

KN4CK3R mentioned this pull request Sep 18, 2022

Public activity in user profile not shown #21180

Open

lafriks reviewed Sep 18, 2022

View reviewed changes

models/migrations/v214.go Outdated Show resolved Hide resolved

6543 reviewed Sep 18, 2022

View reviewed changes

models/migrations/v214.go Outdated Show resolved Hide resolved

KN4CK3R and others added 8 commits September 20, 2022 10:15

Merge branch 'main' of https://github.com/go-gitea/gitea into fix-nul…

df7fcbf

…l-fields

Add more NOT NULLs.

40bb73b

lint

881278f

Set value for is_mirror.

c241285

Merge branch 'main' of https://github.com/go-gitea/gitea into fix-nul…

334e494

…l-fields

Fix WebAuthnCredential table name.

28bff39

Check for errors.

9ca0bf6

Merge branch 'main' into fix-null-fields

22321e4

6543 requested changes Sep 27, 2022

View reviewed changes

KN4CK3R added 2 commits September 27, 2022 10:04

Use modifyColumns.

9548f35

Merge branch 'main' of https://github.com/go-gitea/gitea into fix-nul…

f9074b6

…l-fields

KN4CK3R added 4 commits October 7, 2022 12:32

Use table recreate for MSSQL.

e3e00dc

Merge branch 'main' into fix-null-fields

b738ad0

Merge branch 'main' into fix-null-fields

ceb577e

Merge branch 'main' into fix-null-fields

79adee3

Merge branch 'main' into fix-null-fields

5ca1d59

6543 approved these changes Oct 16, 2022

View reviewed changes

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 Oct 16, 2022

6543 added this to the 1.18.0 milestone Oct 16, 2022

Merge branch 'main' into fix-null-fields

61c2ea6

wxiaoguang modified the milestones: 1.18.0, 1.x.x Oct 17, 2022

lunny removed this from the 1.x.x milestone Jan 18, 2023

pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 10, 2023

KN4CK3R mentioned this pull request Nov 17, 2023

Support repo license #24872

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Converts columns from NULL to NOT NULL DEFAULT #18871

Converts columns from NULL to NOT NULL DEFAULT #18871

KN4CK3R commented Feb 23, 2022 •

edited

Loading

codecov-commenter commented Mar 8, 2022

KN4CK3R commented May 2, 2022

lunny commented Sep 19, 2022

6543 left a comment

lafriks commented Sep 27, 2022

wxiaoguang commented Oct 13, 2022 •

edited

Loading

lafriks commented Oct 13, 2022

wxiaoguang commented Oct 13, 2022

wxiaoguang commented Oct 16, 2022 •

edited

Loading

KN4CK3R commented Oct 16, 2022

wxiaoguang commented Oct 16, 2022 •

edited

Loading

lunny commented Oct 16, 2022

wxiaoguang commented Oct 16, 2022 •

edited

Loading

lunny commented Oct 16, 2022

wxiaoguang commented Oct 16, 2022

wxiaoguang commented Oct 16, 2022 •

edited

Loading

puni9869 commented Sep 1, 2023

KN4CK3R commented Sep 1, 2023

puni9869 commented Sep 1, 2023 •

edited

Loading

Converts columns from NULL to NOT NULL DEFAULT #18871

Are you sure you want to change the base?

Converts columns from NULL to NOT NULL DEFAULT #18871

Conversation

KN4CK3R commented Feb 23, 2022 • edited Loading

codecov-commenter commented Mar 8, 2022

Codecov Report

KN4CK3R commented May 2, 2022

lunny commented Sep 19, 2022

6543 left a comment

Choose a reason for hiding this comment

lafriks commented Sep 27, 2022

wxiaoguang commented Oct 13, 2022 • edited Loading

lafriks commented Oct 13, 2022

wxiaoguang commented Oct 13, 2022

wxiaoguang commented Oct 16, 2022 • edited Loading

KN4CK3R commented Oct 16, 2022

wxiaoguang commented Oct 16, 2022 • edited Loading

lunny commented Oct 16, 2022

wxiaoguang commented Oct 16, 2022 • edited Loading

lunny commented Oct 16, 2022

wxiaoguang commented Oct 16, 2022

wxiaoguang commented Oct 16, 2022 • edited Loading

puni9869 commented Sep 1, 2023

KN4CK3R commented Sep 1, 2023

puni9869 commented Sep 1, 2023 • edited Loading

KN4CK3R commented Feb 23, 2022 •

edited

Loading

wxiaoguang commented Oct 13, 2022 •

edited

Loading

wxiaoguang commented Oct 16, 2022 •

edited

Loading

wxiaoguang commented Oct 16, 2022 •

edited

Loading

wxiaoguang commented Oct 16, 2022 •

edited

Loading

wxiaoguang commented Oct 16, 2022 •

edited

Loading

puni9869 commented Sep 1, 2023 •

edited

Loading