feat(op-work-packages): update path with optimistic locking#67
Conversation
Add the WorkPackage update write path on top of the #66 CreateService. - UpdateWorkPackage — a sparse patch write-model (Deserialize); each Some field is applied, None leaves the value untouched. (Un-setting a nullable FK to NULL is a documented follow-up — needs Option<Option<_>>.) - WorkPackageStore::update — persist changes to an existing row. - WorkPackageService::update(id, expected_lock_version, changes) — optimistic locking: NotFound if absent, Conflict{expected,actual} if the caller's version is stale, Validation if the patch would blank the subject; on success applies the patch, bumps lock_version, refreshes updated_at, persists, returns the entity. - WorkPackageError::Conflict variant. - MemoryWorkPackageStore::update. 4 new update tests (apply+bump, stale-version conflict, NotFound, blank- subject rejection) -> 16 tests pass. fmt clean; op-work-packages clippy-clean (0 warnings in this crate; transitive op-core -D-warnings lints pre-existing). Follow-up: emit a change event on create/update so the composition layer can journal it (op-work-packages <-> op-journals), without coupling the crate to op-journals.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 77e89ae4c0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| wp.lock_version += 1; | ||
| wp.updated_at = Utc::now(); | ||
| self.store.update(&wp).await?; |
There was a problem hiding this comment.
When two updates for the same work package run concurrently with the same expected_lock_version, both can read the old row in find(), pass the check above, then call store.update() and overwrite each other while both return success. Because the version predicate is not part of the write (and the store API only receives the already-bumped row), optimistic locking does not actually prevent lost updates for any asynchronous/DB-backed store or parallel in-memory access; the compare against the expected version needs to happen inside the store update/WHERE clause.
Useful? React with 👍 / 👎.
2 participants
What
The WorkPackage update write path on top of the #66
CreateService, with OpenProject-style optimistic locking (lock_version).UpdateWorkPackage— a sparse patch write-model (Deserialize); eachSomefield is applied,Noneleaves the current value untouched.WorkPackageService::update(id, expected_lock_version, changes)— the guarded write:NotFoundif the id is absent,Conflict { expected, actual }if the caller'slock_versionis stale (someone wrote first),Validationif the patch would blank the subject,lock_version→ refreshupdated_at→ persist → return the entity.WorkPackageStore::update+ theMemoryWorkPackageStoreimpl;WorkPackageError::Conflictvariant.Tests
4 new update tests (apply + version bump, stale-version →
Conflict,NotFound, blank-subject rejection) → 16 tests pass total.Gate
fmt --checkclean;op-work-packagesclippy-clean (0 warnings in this crate). As before,-D warningstransitively surfaces the pre-existingop-corelints already onmain— untouched here.Notes
NULL(e.g. clearing an assignee) is a documented follow-up — theNone-means-"no change" convention needs anOption<Option<_>>to also express "set to null".op-work-packages↔op-journals) without coupling this crate toop-journals.Branch note
Follows the merged #66; branch restarted from the new
main(832004a) per the merged-branch rule, single follow-up commit, no overlap with the walledodoo-rs-transcodebranch.🤖 Generated with Claude Code
Generated by Claude Code