Skip to content

Expose content_version for optimistic locking on issue and PR edits#37035

Merged
wxiaoguang merged 5 commits intogo-gitea:mainfrom
myers:feat/issue-content-version
Mar 30, 2026
Merged

Expose content_version for optimistic locking on issue and PR edits#37035
wxiaoguang merged 5 commits intogo-gitea:mainfrom
myers:feat/issue-content-version

Conversation

@myers
Copy link
Copy Markdown
Contributor

@myers myers commented Mar 29, 2026 •
edited by silverwind
Loading

No one wants their work overwritten by accident. Allow the API to check before it writes.

Major Changes

  • Add content_version field to Issue and PullRequest API responses
  • Accept optional content_version in PATCH /repos/{owner}/{repo}/issues/{index} and PATCH /repos/{owner}/{repo}/pulls/{index} — returns 409 Conflict when stale, succeeds silently when omitted (backward compatible)
  • Pre-check content_version before any mutations to prevent partial writes (e.g. title updated but body rejected)
  • Breaking: content version conflicts now return 409 Conflict instead of 400 Bad Request

Written with Claude Code and Opus, reviewed by human.

@myers
Expose content_version for optimistic locking on issue and PR edits
df4fd7f 
Add content_version field to Issue and PullRequest API responses.
Accept optional content_version in edit endpoints — returns 409
Conflict when stale, succeeds silently when omitted.
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 29, 2026
@github-actions github-actions bot added modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code labels Mar 29, 2026
bircni
bircni suggested changes Mar 29, 2026
View reviewed changes
@GiteaBot GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Mar 29, 2026
myers and others added 3 commits March 30, 2026 03:25
@myers
Check content_version before any mutations to prevent partial writes
4d7c7b3 
When a PATCH updates both title and body with a stale content_version,
the title change would succeed before the body update returned 409.
Add an upfront version check before any mutations so the entire
request is rejected early if the version is stale.

The DB-level check in ChangeContent remains for concurrent request
safety. A full transaction wrapping all mutations would be the
complete fix, noted as a TODO.
@wxiaoguang
modern go
f5cbba9
@wxiaoguang
refactor tests
6bad3af
@wxiaoguang
Copy link
Copy Markdown
Contributor

wxiaoguang commented Mar 30, 2026

Overall looks good to me.

btw: next time you can ask your AI to use modern go like this: f5cbba9

@wxiaoguang wxiaoguang requested a review from bircni March 30, 2026 06:49
bircni
bircni approved these changes Mar 30, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@bircni bircni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm - I don't see why I would use this but ok

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged labels Mar 30, 2026
silverwind
silverwind reviewed Mar 30, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@silverwind silverwind left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Three things I noticed:

  1. Breaking change: 400 → 409 status code — The error response for ChangeContent version conflicts changed from http.StatusBadRequest to http.StatusConflict in both issue.go and pull.go. This affects all requests hitting the DB-level version check, not just those opting into the new content_version field. Any existing API client handling 400 for this error will break. While 409 is semantically more correct, this should be called out as a breaking API change.

  2. No tests for the pull request endpoint — testAPIIssueContentVersion only covers the issue edit path. The PR edit endpoint has identical content_version logic but no test coverage.

  3. Missing swagger description annotations — The new ContentVersion fields in the response structs (Issue, PullRequest) and edit option structs lack swagger description comments. Most other fields in these structs have doc comments describing them.

This comment was written by Claude Opus 4.6.

@silverwind
Copy link
Copy Markdown
Member

silverwind commented Mar 30, 2026

status code change I wouldn't consider breaking, so skip that one.

@wxiaoguang wxiaoguang enabled auto-merge (squash) March 30, 2026 13:19
@wxiaoguang wxiaoguang merged commit c31e0cf into go-gitea:main Mar 30, 2026
26 checks passed
@GiteaBot GiteaBot added this to the 1.26.0 milestone Mar 30, 2026
@silverwind
Copy link
Copy Markdown
Member

silverwind commented Mar 30, 2026 •
edited
Loading

Why merge with unaddressed comments? Do you reject them?

@wxiaoguang
Copy link
Copy Markdown
Contributor

wxiaoguang commented Mar 30, 2026

status code change I wouldn't consider breaking, so skip that one.

I thought you meant that there is no concern anymore.

To me:

  1. not breaking change
  2. quite trivial, testing it or not, no much difference
  3. also quite trivial, the field name is clear enough, even if you add comment like "the content version of the issue", it brings no useful inforamtion

@wxiaoguang
Copy link
Copy Markdown
Contributor

wxiaoguang commented Mar 30, 2026

quite trivial, testing it or not, no much difference

I will add some tests for PR edit

@silverwind
Copy link
Copy Markdown
Member

silverwind commented Mar 30, 2026

I meant skip 1, but do 2 and 3, sorry if it was unclear.

@wxiaoguang
Copy link
Copy Markdown
Contributor

wxiaoguang commented Mar 30, 2026

Maybe it's better to convert a "lgtm/done" PR to WIP when there are still concerns to address.

-> Add tests for pull request's content_version in API #37044

I am not sure how to do 3, feel free to edit the PR directly.

@silverwind
Copy link
Copy Markdown
Member

silverwind commented Mar 30, 2026 •
edited
Loading

Done in a7e5a90. I see we lack description for many API fields, this is something we can fix with with AI later.

zjjhot added a commit to zjjhot/gitea that referenced this pull request Mar 31, 2026
@zjjhot
Merge branch 'main' into mainzjj
c7cac10 
* main:
  Refactor issue sidebar and fix various problems (go-gitea#37045)
  Add tests for pull request's content_version in API (go-gitea#37044)
  Enable concurrent vitest execution (go-gitea#36998)
  Fix theme discovery and Vite dev server in dev mode (go-gitea#37033)
  bump snapcraft deps (go-gitea#37039)
  Expose content_version for optimistic locking on issue and PR edits (go-gitea#37035)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@silverwind silverwind silverwind left review comments

@wxiaoguang wxiaoguang wxiaoguang approved these changes

+1 more reviewer

@bircni bircni bircni approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/api This PR adds API routes or modifies them modifies/go Pull requests that update Go code

Projects

None yet

Milestone

1.26.0

Development

Successfully merging this pull request may close these issues.

5 participants

@myers @wxiaoguang @silverwind @bircni @GiteaBot