Skip to content

fix(migrations): preserve unique constraints in v334 sync#37743

Merged
bircni merged 2 commits into
go-gitea:mainfrom
silverwind:fix/v334-ignore-constrains
May 17, 2026
Merged

fix(migrations): preserve unique constraints in v334 sync#37743
bircni merged 2 commits into
go-gitea:mainfrom
silverwind:fix/v334-ignore-constrains

Conversation

@silverwind
Copy link
Copy Markdown
Member

@silverwind silverwind commented May 17, 2026

The truncated ActionRunner struct in AddCancellingSupportToActionRunner declares only the new HasCancellingSupport column. When xorm's SyncWithOptions compares it against the live action_runner table, every index/constraint absent from the local struct is a candidate for removal.

Walking xorm v1.3.11 sync.go:250-266:

  • IndexType indices skip the drop when IgnoreIndices || IgnoreDropIndices — already covered.
  • UniqueType indices skip the drop only when IgnoreConstrains — not set in feat: execute post run cleanup when workflow is cancelled #37275, so the existing UNIQUE on token_hash (and any other uniques) would be dropped on upgrade.

Adding IgnoreConstrains: true matches v333's pattern and preserves the existing unique constraints. Spotted by @wxiaoguang in #37275 (comment).

This PR was written with the help of Claude Opus 4.7

@silverwind @claude
fix(migrations): preserve unique constraints in v334 sync
689718c 
The partial ActionRunner struct in AddCancellingSupportToActionRunner
omits the UNIQUE on token_hash, so xorm's SyncWithOptions would drop
that constraint to make the live table match. Set IgnoreConstrains so
existing unique constraints are left alone, matching v333.

Co-Authored-By: Claude (Opus 4.7) <noreply@anthropic.com>
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 17, 2026
@silverwind silverwind mentioned this pull request May 17, 2026
Merged
@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 May 17, 2026
@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 May 17, 2026
@wxiaoguang
Copy link
Copy Markdown
Contributor

wxiaoguang commented May 17, 2026

@lunny what's your plan to avoid such problems in the future? I have pointed it out at least 3 times.

@bircni bircni added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label May 17, 2026
@bircni bircni enabled auto-merge (squash) May 17, 2026 15:41
@bircni
Copy link
Copy Markdown
Member

bircni commented May 17, 2026

@lunny what's your plan to avoid such problems in the future? I have pointed it out at least 3 times.

maybe adding a lint for it or any other checks?

@bircni bircni merged commit d9149d8 into go-gitea:main May 17, 2026
21 checks passed
@GiteaBot GiteaBot added this to the 1.27.0 milestone May 17, 2026
@lunny
Copy link
Copy Markdown
Member

lunny commented May 17, 2026

@lunny what's your plan to avoid such problems in the future? I have pointed it out at least 3 times.

It might be a new sync function like Sync(struct, is_drop_index, is_drop_unqie) could avoid some possible wrong usage.

@lunny
Copy link
Copy Markdown
Member

lunny commented May 17, 2026

@lunny what's your plan to avoid such problems in the future? I have pointed it out at least 3 times.

maybe adding a lint for it or any other checks?

It's difficult to know it want to create a new table or add a new column.

@wxiaoguang wxiaoguang deleted the fix/v334-ignore-constrains branch May 17, 2026 17:38
@wxiaoguang
Copy link
Copy Markdown
Contributor

wxiaoguang commented May 17, 2026

@lunny what's your plan to avoid such problems in the future? I have pointed it out at least 3 times.

maybe adding a lint for it or any other checks?

It's difficult to know it want to create a new table or add a new column.

No, the callers all clearly know

@lunny what's your plan to avoid such problems in the future? I have pointed it out at least 3 times.

It might be a new sync function like Sync(struct, is_drop_index, is_drop_unqie) could avoid some possible wrong usage.

It's just a new dirty patch, just more messy.

@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label May 17, 2026
@silverwind
Copy link
Copy Markdown
Member Author

silverwind commented May 17, 2026

It's difficult to know it want to create a new table or add a new column.

With #37668 it will be possible to write golangci-lint plugins with full AST access. BTW still looking for approval there.

@wxiaoguang
Copy link
Copy Markdown
Contributor

wxiaoguang commented May 17, 2026

it will be possible to write golangci-lint plugins with full AST access.

Even if you have full AST access, what can you do for it? What are the rules in your mind to "lint" for these cases?

@silverwind
Copy link
Copy Markdown
Member Author

silverwind commented May 17, 2026

Right, the lint does not see the DB structure. The better fix would probably be to have xorm have safer defaults, should check how other ORMs do it.

@lunny lunny added type/bug skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. labels May 17, 2026
silverwind added a commit to silverwind/gitea that referenced this pull request May 17, 2026
@silverwind
Merge remote-tracking branch 'origin/main' into lint-shell
56767d4 
* origin/main:
  fix: Allow direct commits for unprotected files with push restrictions (go-gitea#37657)
  chore: Conventional adjustments (go-gitea#37677)
  chore(db): introduce db.Session and db.EngineMigration interfaces (go-gitea#37746)
  fix(migrations): preserve unique constraints in v334 sync (go-gitea#37743)
  feat(web): also display PR counts in repo list (go-gitea#37739)
silverwind added a commit to silverwind/gitea that referenced this pull request May 18, 2026
@silverwind
Merge remote-tracking branch 'origin/main' into ci-cache-improvements
751acd4 
* origin/main: (39 commits)
  fix: Add missed token scope checking (go-gitea#37735)
  chore: Use giteabot instead of backporter (go-gitea#37422)
  fix: Allow direct commits for unprotected files with push restrictions (go-gitea#37657)
  chore: Conventional adjustments (go-gitea#37677)
  chore(db): introduce db.Session and db.EngineMigration interfaces (go-gitea#37746)
  fix(migrations): preserve unique constraints in v334 sync (go-gitea#37743)
  feat(web): also display PR counts in repo list (go-gitea#37739)
  feat: execute post run cleanup when workflow is cancelled (go-gitea#37275)
  fix(actions): wrong assumption that run id always >= job id (go-gitea#37737)
  fix(icon): use repo-forked icon to display forks count (go-gitea#37731)
  fix(oauth): strengthen PKCE validation and refresh token replay protection (go-gitea#37706)
  fix(web): enforce token scopes on raw, media, and attachment downloads (go-gitea#37698)
  feat: Add bypass allowlist for branch protection (go-gitea#36514)
  refactor(glob): use strings.Builder for regexp compilation (go-gitea#37730)
  feat(oauth): Support AWS Cognito OAuth2 provider (go-gitea#37607)
  feat: Add default PR branch update style setting (go-gitea#37410)
  refactor: move `workflowpattern` into `modules/actions` (go-gitea#37717)
  ci: add `zizmor` to `lint-actions` (go-gitea#37720)
  chore(doctor): remove four obsolete doctor check implementations (go-gitea#37728)
  chore(renovate): enable dockerfile manager (go-gitea#37719)
  ...

# Conflicts:
#	modules/globallock/locker_test.go
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lunny lunny lunny approved these changes

@bircni bircni bircni approved these changes

Assignees

No one assigned

Labels

lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/bug

Projects

None yet

Milestone

1.27.0

Development

Successfully merging this pull request may close these issues.

5 participants

@silverwind @wxiaoguang @bircni @lunny @GiteaBot