mirror of
https://codeberg.org/forgejo/forgejo.git
synced 2026-05-12 22:10:25 +00:00
16f98ebaec
**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> |
||
|---|---|---|
| .. | ||
| base | ||
| fixtures | ||
| test | ||
| v1_6 | ||
| v1_7 | ||
| v1_8 | ||
| v1_9 | ||
| v1_10 | ||
| v1_11 | ||
| v1_12 | ||
| v1_13 | ||
| v1_14 | ||
| v1_15 | ||
| v1_16 | ||
| v1_17 | ||
| v1_18 | ||
| v1_19 | ||
| v1_20 | ||
| v1_21 | ||
| v1_22 | ||
| v1_23 | ||
| main_test.go | ||
| migrations.go | ||
| migrations_test.go | ||