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

Fix dropTableColumns sqlite implementation #7710

Merged
merged 8 commits into from Aug 5, 2019

Conversation

@zeripath
Copy link
Contributor

commented Aug 1, 2019

The implementation of dropTableColumns for sqlite3 doesn't work if the last column is removed. This PR fixes this.

Fix #7694
Fix #7698

@zeripath zeripath added this to the 1.10.0 milestone Aug 1, 2019

@sapk
sapk approved these changes Aug 1, 2019

@GiteaBot GiteaBot added the lgtm/need 1 label Aug 1, 2019

@sapk

This comment has been minimized.

Copy link
Member

commented Aug 1, 2019

It would be good to have a unit test for that. At least testing the query generation and validate what it should look like.

@lunny

This comment has been minimized.

Copy link
Member

commented Aug 2, 2019

@zeripath I agree with @sapk we need unit tests on this change.

@codecov-io

This comment has been minimized.

Copy link

commented Aug 2, 2019

Codecov Report

Merging #7710 into master will increase coverage by 0.03%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7710      +/-   ##
==========================================
+ Coverage   41.27%   41.31%   +0.03%     
==========================================
  Files         473      473              
  Lines       63885    63828      -57     
==========================================
+ Hits        26371    26372       +1     
+ Misses      34076    34019      -57     
+ Partials     3438     3437       -1
Impacted Files Coverage Δ
models/migrations/v78.go 0% <0%> (ø) ⬆️
models/migrations/migrations.go 1.3% <0%> (-0.03%) ⬇️
models/migrations/v85.go 0% <0%> (ø) ⬆️
routers/repo/view.go 42.23% <0%> (-1.02%) ⬇️
models/repo_list.go 73.09% <0%> (+1.01%) ⬆️
modules/process/manager.go 81.15% <0%> (+4.34%) ⬆️

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 7ad6710...62f970b. Read the comment docs.

@zeripath

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2019

Writing a test for this apart from adding a migration that would trigger it is not possible.

We cannot run this as a unit test because we need the various dbs, so we have to run it as an integration test.

Now integration tests are run separately from the unit tests. However, neither models.x nor migrations.dropTableColumns are exported to the integrations tests and cannot be simply testshim exported as the integrations test don't run in the same testing context and therefore these shims are not available.

zeripath added 4 commits Aug 2, 2019
@lafriks

This comment has been minimized.

Copy link
Member

commented Aug 2, 2019

I think we can skip tests for this as it will be quite hard to do that

@zeripath

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2019

Ok so by changing the v78 migration this code will run - but not the failing migration. I can create a 1.8.3 sqlite migration test if that would help get this merged though.

@sapk

This comment has been minimized.

Copy link
Member

commented Aug 4, 2019

@zeripath if tests are too complicated or kind of a trick. Just drop them. We will figure this out later.

@zeripath

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

Yeah they're not really possible. The only test is by doing a migration - I'm confused as to why our migration tests didn't highlight the problem with the code in the first place though.

zeripath added 2 commits Aug 5, 2019
@zeripath

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

So I can't get a migration from 1.8.3 to fail, so I've added a migration from 1.3.3 for sqlite which fails without this PR. Thus proving this works.

@lafriks
lafriks approved these changes Aug 5, 2019

@GiteaBot GiteaBot added lgtm/done and removed lgtm/need 1 labels Aug 5, 2019

@zeripath zeripath merged commit 026696b into go-gitea:master Aug 5, 2019

2 checks passed

approvals/lgtm this commit looks good
continuous-integration/drone/pr Build is passing
Details
@lafriks

This comment has been minimized.

Copy link
Member

commented Aug 5, 2019

Please send backport :)

@zeripath zeripath deleted the zeripath:fix-dropTableColumns branch Aug 5, 2019

zeripath added a commit to zeripath/gitea that referenced this pull request Aug 5, 2019
Fix dropTableColumns sqlite implementation (go-gitea#7710)
* Fix dropTableColumns sqlite implementation

* use droptables and its index dropping support in v78 and v85

* golang-ci fixes

* Add migration from gitea 1.3.3 for sqlite which reveals the droptables bug - thus showing this works
lafriks added a commit that referenced this pull request Aug 6, 2019
Fix dropTableColumns sqlite implementation (#7710) (#7765)
* Fix dropTableColumns sqlite implementation

* use droptables and its index dropping support in v78 and v85

* golang-ci fixes

* Add migration from gitea 1.3.3 for sqlite which reveals the droptables bug - thus showing this works

@lunny lunny added the backport/done label Aug 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
6 participants
You can’t perform that action at this time.