upstream/forgejo
1
0
Fork 0
mirror of https://codeberg.org/forgejo/forgejo.git synced 2026-02-20 02:10:04 -05:00
Code Issues Projects Releases Packages Wiki Activity Actions 4

[v14.0/forgejo] fix: use ALTER TABLE in SQLite DropTableColumns(), allowing unexpected database sources to work better in migrations (#10903)

Browse source 
**Backport:** https://codeberg.org/forgejo/forgejo/pulls/10888

The existing implementation of `DropTableColumns()` came from before SQLite had the ability to `ALTER TABLE ... DROP COLUMN ...`.  It works by parsing the table definition and rewriting it without the columns that are to be dropped, but it will fail to do this correctly if the table definition is not in the exact expected format.  In #10887, a database that had probably come through some migration tool was not exactly formatted the way Forgejo expected, resulting in a migration failure.

This replaces `DropTableColumns()`'s hacky SQLite implementation with a more straightforward implementation.  Affected indexes touching the target fields are dropped, then the field is dropped.

DROP COLUMN is supported on SQLite since [3.35.0, 2021-03-12](https://sqlite.org/releaselog/3_35_0.html).

## Checklist

The [contributor guide](https://forgejo.org/docs/next/contributor/) contains information that will be helpful to first time contributors. There also are a few [conditions for merging Pull Requests in Forgejo repositories](https://codeberg.org/forgejo/governance/src/branch/main/PullRequestsAgreement.md). You are also welcome to join the [Forgejo development chatroom](https://matrix.to/#/#forgejo-development:matrix.org).

### Tests

- Existing `test-sqlite-migration` coverage is relied upon for this change.  During development it was proven to exercise the affected code -- in other words, multiple iterations of changes were required due to it failing as I worked on it.
- No coverage is added for "database with unexpected schema definition format" as the trigger issue for this change though, a point that can be raised if someone believes it is worthwhile.
- I added test coverage for Go changes...
  - [ ] in their respective `*_test.go` for unit tests.
  - [ ] in the `tests/integration` directory if it involves interactions with a live Forgejo server.
- I added test coverage for JavaScript changes...
  - [ ] in `web_src/js/*.test.js` if it can be unit tested.
  - [ ] in `tests/e2e/*.test.e2e.js` if it requires interactions with a live Forgejo server (see also the [developer guide for JavaScript testing](https://codeberg.org/forgejo/forgejo/src/branch/forgejo/tests/e2e/README.md#end-to-end-tests)).

### Documentation

- [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change.
- [x] I did not document these changes and I do not expect someone else to do it.

### Release notes

- [x] This change will be noticed by a Forgejo user or admin (feature, bug fix, performance, etc.). I suggest to include a release note for this change.
- [ ] This change is not visible to a Forgejo user or admin (refactor, dependency upgrade, etc.). I think there is no need to add a release note for this change.

*The decision if the pull request will be shown in the release notes is up to the mergers / release team.*

The content of the `release-notes/<pull request number>.md` file will serve as the basis for the release notes. If the file does not exist, the title of the pull request will be used instead.

Co-authored-by: Mathieu Fenniak <mathieu@fenniak.net>
Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/10903
Reviewed-by: Mathieu Fenniak <mfenniak@noreply.codeberg.org>
Co-authored-by: forgejo-backport-action <forgejo-backport-action@noreply.codeberg.org>
Co-committed-by: forgejo-backport-action <forgejo-backport-action@noreply.codeberg.org>
This commit is contained in:
forgejo-backport-action 2026-01-17 22:34:10 +01:00 committed by Mathieu Fenniak
parent ca46a3f68b
commit 16f98ebaec
1 changed files with 13 additions and 64 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

77
models/gitea_migrations/base/db.go
View file

@ -7,7 +7,6 @@ import (
"errors"
"fmt"
"reflect"
"regexp"
"slices"
"strings"
@ -469,74 +468,24 @@ func DropTableColumns(sess *xorm.Session, tableName string, columnNames ...strin
if err != nil {
return err
}
if len(indexRes) != 1 {
continue
containsDroppedColumn := false
for _, r := range indexRes {
indexCol := string(r["name"])
if slices.Contains(columnNames, indexCol) {
containsDroppedColumn = true
break
}
}
indexColumn := string(indexRes[0]["name"])
for _, name := range columnNames {
if name == indexColumn {
_, err := sess.Exec(fmt.Sprintf("DROP INDEX `%s`", indexName))
if err != nil {
return err
}
if containsDroppedColumn {
if _, err := sess.Exec(fmt.Sprintf("DROP INDEX `%s`", indexName)); err != nil {
return err
}
}
}
// Here we need to get the columns from the original table
sql := fmt.Sprintf("SELECT sql FROM sqlite_master WHERE tbl_name='%s' and type='table'", tableName)
res, err := sess.Query(sql)
if err != nil {
return err
}
tableSQL := string(res[0]["sql"])
// Get the string offset for column definitions: `CREATE TABLE ( column-definitions... )`
columnDefinitionsIndex := strings.Index(tableSQL, "(")
if columnDefinitionsIndex < 0 {
return errors.New("couldn't find column definitions")
}
// Separate out the column definitions
tableSQL = tableSQL[columnDefinitionsIndex:]
// Remove the required columnNames
for _, name := range columnNames {
tableSQL = regexp.MustCompile(regexp.QuoteMeta("`"+name+"`")+"[^`,)]*?[,)]").ReplaceAllString(tableSQL, "")
}
// Ensure the query is ended properly
tableSQL = strings.TrimSpace(tableSQL)
if tableSQL[len(tableSQL)-1] != ')' {
if tableSQL[len(tableSQL)-1] == ',' {
tableSQL = tableSQL[:len(tableSQL)-1]
for _, col := range columnNames {
if _, err := sess.Exec(fmt.Sprintf("ALTER TABLE `%s` DROP COLUMN `%s`", tableName, col)); err != nil {
return fmt.Errorf("drop table `%s` column %s encountered error: %w", tableName, col, err)
}
tableSQL += ")"
}
// Find all the columns in the table
columns := regexp.MustCompile("`([^`]*)`").FindAllString(tableSQL, -1)
tableSQL = fmt.Sprintf("CREATE TABLE `new_%s_new` ", tableName) + tableSQL
if _, err := sess.Exec(tableSQL); err != nil {
return err
}
// Now restore the data
columnsSeparated := strings.Join(columns, ",")
insertSQL := fmt.Sprintf("INSERT INTO `new_%s_new` (%s) SELECT %s FROM %s", tableName, columnsSeparated, columnsSeparated, tableName)
if _, err := sess.Exec(insertSQL); err != nil {
return err
}
// Now drop the old table
if _, err := sess.Exec(fmt.Sprintf("DROP TABLE `%s`", tableName)); err != nil {
return err
}
// Rename the table
if _, err := sess.Exec(fmt.Sprintf("ALTER TABLE `new_%s_new` RENAME TO `%s`", tableName, tableName)); err != nil {
return err
}
case setting.Database.Type.IsPostgreSQL():