feat: add forShare and skipLocked selection modifiers to knex loader - #705
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #705 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 109 109
Lines 18093 18169 +76
Branches 1546 1549 +3
=========================================
+ Hits 18093 18169 +76
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| /** | ||
| * Lock the selected rows for update using `SELECT ... FOR UPDATE`. The lock is held until the | ||
| * end of the transaction, so the query must be executed in a transactional query context. | ||
| * Mutually exclusive with `forShare`. |
There was a problem hiding this comment.
Should we throw an error if forShare is set? I think it's not a bad idea to enforce these requirements outside the validator.
There was a problem hiding this comment.
We do in EntityKnexDataManager, yep!
Edit: misread your comment. I think so far these are roughly the builder pattern, though not a true build(). I'm undecided. Most builders throw upon build() which in this case is executeAsync(), which is what runs the validation.
There was a problem hiding this comment.
I think I'll keep it as-is for now, but we can see how it feels and add the validation if we see places it'd be better to throw earlier.
3 participants
Why
Similar to #704, other common reasons Expo engineers drop down to raw knex is that they want to use forShare and skipLocked during a transaction. This adds the functionality to entity.
How
Told claude: "add skipLocked and forShare as well"
I read through the code manually.
Test Plan
Run added tests.