[Kysely] Switch lint enforcement from Sequelize to Kysely and added transaction wrapper enforcement#436
Open
juanmrad wants to merge 4 commits into
Open
[Kysely] Switch lint enforcement from Sequelize to Kysely and added transaction wrapper enforcement#436juanmrad wants to merge 4 commits into
juanmrad wants to merge 4 commits into
Conversation
juanmrad
requested review from
cassidyjames,
dom-notion,
julietshen,
pawiecz and
vinaysrao1
as code owners
`fc.date()` without `noInvalidDate` can draw `Date(NaN)`, and calling
`.toISOString()` on an invalid date throws `RangeError: Invalid time value`.
This caused intermittent CI failures in
`extractItemDataValues.test.ts > getFieldValueOrValues > should return arrays
for array type fields`, since that test reaches `DateStringArbitrary` via
`ArrayFieldWithValueArbitrary` for the DATETIME scalar.
Passing `{ noInvalidDate: true }` (available since fast-check 3.13.0)
constrains the arbitrary to valid dates only, eliminating the flake.
There was a problem hiding this comment.
Pull request overview
This PR shifts transaction enforcement from legacy Sequelize patterns to Kysely by introducing/expanding a Kysely transaction-with-retry wrapper and updating direct Kysely transaction callers to use it.
Changes:
- Adds isolation-level support and tests for
makeKyselyTransactionWithRetry. - Replaces direct
kysely.transaction()usage across services/jobs with the wrapper. - Updates lint restrictions and removes/refreshes Sequelize-related comments and docs.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
server/.env.example |
Removes obsolete Sequelize pool documentation. |
server/.eslintrc.cjs |
Replaces Sequelize transaction restriction with raw Kysely transaction restriction. |
server/graphql/datasources/LocationBankApi.ts |
Uses transaction retry wrapper for delete flow. |
server/graphql/datasources/RuleApi.ts |
Uses transaction retry wrapper for rule update/delete flows. |
server/iocContainer/index.ts |
Refreshes KyselyPg service comments. |
server/rule_engine/RuleEngine.ts |
Generalizes connection timeout comment. |
server/services/manualReviewToolService/modules/AppealsJobRouting.ts |
Uses transaction retry wrapper in appeals routing mutations. |
server/services/manualReviewToolService/modules/JobRouting.ts |
Uses transaction retry wrapper in routing mutations. |
server/services/manualReviewToolService/modules/QueueOperations.ts |
Uses transaction retry wrapper in queue mutations. |
server/services/moderationConfigService/modules/ItemTypeOperations.ts |
Uses transaction retry wrapper with isolation options. |
server/services/moderationConfigService/modules/UserStrikeOperations.ts |
Uses transaction retry wrapper for bulk threshold replacement. |
server/services/moderationConfigService/types/conditionResults.ts |
Simplifies condition result documentation. |
server/services/notificationsService/notificationsService.ts |
Removes Sequelize-specific comment wording. |
server/services/reportingService/ReportingRules.ts |
Uses transaction retry wrapper in reporting rule mutations. |
server/services/ruleHistoryService/ruleHistoryService.ts |
Refreshes read-only view documentation. |
server/test/arbitraries/ContentType.ts |
Prevents invalid dates in date-string arbitrary generation. |
server/utils/kyselyTransactionWithRetry.test.ts |
Adds unit coverage for retry behavior and isolation options. |
server/utils/kyselyTransactionWithRetry.ts |
Adds overloads/options and isolation-level support to the retry wrapper. |
server/utils/url.ts |
Simplifies URL validation default comment. |
server/workers_jobs/RefreshMRTDecisionsMaterializedViewJob.ts |
Uses transaction retry wrapper in materialized view refresh job. |
Comments suppressed due to low confidence (1)
server/utils/kyselyTransactionWithRetry.test.ts:124
- This test says it covers errors thrown by the transaction callback, but the fake Kysely throws
appErrorfrom the per-attempt behavior beforecb({})is invoked, so the callback-error path remains untested. Move the throw into the callback or adjust the setup/name so the test exercises the behavior it claims to cover.
test('does not retry on plain (non-pg) errors thrown by the callback', async () => {
const appError = new Error('business logic error');
const { fakeKysely, getAttemptCount } = makeFakeKysely([
throwing(appError),
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Document retry-safe callback caveat on `makeKyselyTransactionWithRetry` (docblock + lint message): on a 40001 the whole callback is re-run, so non-DB side effects must be idempotent or deferred until commit. - Extend wrapper tests to model both pre- and post-callback failures and add coverage for the realistic commit-time 40001 (callback ran twice). - `ItemTypeOperations.getItemTypesForAction`: route the inner item-type reads through `trx` so they share the `repeatable read` snapshot with the initial action lookup. - `ReportingRules.updateReportingRule`: fallback association reads now key on the rule's `id` (not `orgId`) and go through `trx`. - `ReportingRules.deleteReportingRule`: actually delete the rule row and all join rows, scoped by `org_id`. - `JobRouting.deleteRoutingRule` / `AppealsJobRouting.deleteAppealsRoutingRule`: return the real outcome via `numDeletedRows`; drop the inner try/catch that was masking serialization failures from the retry wrapper.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughIntroduces makeKyselyTransactionWithRetry (with isolation-level support), adds tests and an ESLint rule banning raw kysely.transaction() usage, and migrates many write transactions across services to use the retry wrapper; several delete APIs now accept orgId and return Promise indicating single-row deletion success. ChangesTransaction Retry Infrastructure & Service Migrations
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
server/rule_engine/RuleEngine.ts (1)
322-329: ⚡ Quick winAdd error logging to support debugging the connection-pool timeout.
While the comment indicates you're debugging the root cause of the connection-pool acquire timeout, the error is currently swallowed without any logging. This makes investigation significantly harder. Consider logging the error to aid debugging while still preventing the process from crashing.
📊 Suggested improvement to add error logging
).catch((error) => { // This query sometimes fails from a connection-pool acquire timeout. // While we're debugging the root cause further, swallow the error // rather than crashing the process. + this.tracer.logError(error, { + context: 'recordRuleActionLimitUsage failed', + ruleIds: actionableRules.map((it) => it.id), + }); })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/rule_engine/RuleEngine.ts` around lines 322 - 329, The catch currently swallows errors from the recordRuleActionLimitUsage call; change it to log the error (including the caught error object and context like the actionable rule IDs from actionableRules.map(it => it.id) and the variable name updateRuleActionCountsPromise) before swallowing so you still avoid crashing; use the instance logger (e.g., this.logger.error) if available, otherwise console.error, and include a clear message plus the error stack/details and the list of rule ids for debugging.server/utils/url.ts (1)
30-33: ⚡ Quick winExtract duplicated default options to eliminate drift risk.
The default
UrlValidationOptionsare duplicated betweenvalidateUrlandvalidateUrlOrNull, requiring manual synchronization. Extract these to a shared constant to apply DRY and remove the maintenance burden.♻️ Proposed refactor to eliminate duplication
+const DEFAULT_URL_VALIDATION_OPTIONS: UrlValidationOptions = { + allowedSchemes: ['http', 'https'], + blockedHostnames: defaultBlockedHostnames(), +}; + export function validateUrl( value: string, - // If you update these opts make sure to update validateUrlOrNull's opts as - // well - opts: UrlValidationOptions = { - allowedSchemes: ['http', 'https'], - blockedHostnames: defaultBlockedHostnames(), - }, + opts: UrlValidationOptions = DEFAULT_URL_VALIDATION_OPTIONS, ) {export function validateUrlOrNull( value?: string, - // Keep this default in sync with `validateUrl`'s default. - opts: UrlValidationOptions = { - allowedSchemes: ['http', 'https'], - blockedHostnames: defaultBlockedHostnames(), - }, + opts: UrlValidationOptions = DEFAULT_URL_VALIDATION_OPTIONS, ) {Also applies to: 74-77
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/utils/url.ts` around lines 30 - 33, The default UrlValidationOptions object is duplicated in validateUrl and validateUrlOrNull; extract a single shared constant (e.g., DEFAULT_URL_VALIDATION_OPTIONS) that contains allowedSchemes and blockedHostnames (using defaultBlockedHostnames()) and replace the inline opts defaults in both validateUrl and validateUrlOrNull to reference this constant so both functions use the same source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/services/manualReviewToolService/modules/AppealsJobRouting.ts`:
- Around line 94-153: After successful mutations that change routing rules
(e.g., in the create flow shown inside transactionWithRetry where you insert
into manual_review_tool.appeals_routing_rules and
manual_review_tool.appeals_routing_rules_to_item_types, and any update/delete
flows including where `#reorderOneRoutingRule` is called), invalidate the
in-memory appealsRoutingRulesCache entry for the org (use orgId as the cache
key) immediately after the DB commit/transaction completes but before returning
to callers; apply the same cache-clear step to the other mutation handlers
referenced (the update and delete flows noted around the other diffs) so
getQueueIdForJob reads fresh rules.
- Around line 261-269: The deleteAppealsRoutingRule method currently deletes
solely by id and must be scoped to the tenant; change the method signature to
accept input { id: string; orgId: string } and in the transactionWithRetry call
add the tenant filter to the query (i.e., include .where('org_id','=', orgId) in
the same deleteFrom('manual_review_tool.appeals_routing_rules') chain alongside
the existing .where('id','=', id)), keeping the existing result.numDeletedRows
=== 1n return semantics so only a row belonging to that org is removable.
In `@server/services/manualReviewToolService/modules/JobRouting.ts`:
- Around line 153-210: After successful writes that modify routing rules (e.g.,
the create block inside transactionWithRetry shown here, and the update/delete
mutation handlers at the other ranges), invalidate the in-memory cache that
getQueueIdForJob reads by calling the routingRulesCache invalidation method
(e.g., this.routingRulesCache.clear() or this.routingRulesCache.invalidate())
after the DB transaction completes but before returning; add the same
cache-clear call in the update and delete code paths (and any path that calls
`#reorderOneRoutingRule`) so routingRulesCache is refreshed immediately after
changes.
- Around line 317-325: The deleteRoutingRule method currently deletes a
routing_rules row by id only, letting a foreign orgId remove another tenant's
rule; update deleteRoutingRule to accept orgId in its input and apply the same
tenant filter used in updateRoutingRule (i.e., add .where('org_id','=', orgId)
to the trx.deleteFrom('manual_review_tool.routing_rules') query inside
transactionWithRetry), and return the boolean based on result.numDeletedRows ===
1n as before.
In `@server/services/reportingService/ReportingRules.ts`:
- Around line 323-347: The current flow deletes join rows before verifying
ownership, risking cross-org data loss and swallowing DB errors; inside the
method that calls transactionWithRetry (the delete routine in
ReportingRules.ts), first SELECT the reporting_rules row by id within the
transaction to confirm org_id === orgId (or return false if not found/doesn't
match), then run the three deletes from reporting_rules_to_item_types,
reporting_rules_to_actions, reporting_rules_to_policies and finally delete the
parent reporting_rules row; remove the broad .catch((_error) => false) so
unexpected DB errors propagate (only return false for the explicit
not-found/ownership mismatch case).
---
Nitpick comments:
In `@server/rule_engine/RuleEngine.ts`:
- Around line 322-329: The catch currently swallows errors from the
recordRuleActionLimitUsage call; change it to log the error (including the
caught error object and context like the actionable rule IDs from
actionableRules.map(it => it.id) and the variable name
updateRuleActionCountsPromise) before swallowing so you still avoid crashing;
use the instance logger (e.g., this.logger.error) if available, otherwise
console.error, and include a clear message plus the error stack/details and the
list of rule ids for debugging.
In `@server/utils/url.ts`:
- Around line 30-33: The default UrlValidationOptions object is duplicated in
validateUrl and validateUrlOrNull; extract a single shared constant (e.g.,
DEFAULT_URL_VALIDATION_OPTIONS) that contains allowedSchemes and
blockedHostnames (using defaultBlockedHostnames()) and replace the inline opts
defaults in both validateUrl and validateUrlOrNull to reference this constant so
both functions use the same source of truth.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 12c118a5-0c19-472f-a804-79b8a262f961
📒 Files selected for processing (20)
server/.env.exampleserver/.eslintrc.cjsserver/graphql/datasources/LocationBankApi.tsserver/graphql/datasources/RuleApi.tsserver/iocContainer/index.tsserver/rule_engine/RuleEngine.tsserver/services/manualReviewToolService/modules/AppealsJobRouting.tsserver/services/manualReviewToolService/modules/JobRouting.tsserver/services/manualReviewToolService/modules/QueueOperations.tsserver/services/moderationConfigService/modules/ItemTypeOperations.tsserver/services/moderationConfigService/modules/UserStrikeOperations.tsserver/services/moderationConfigService/types/conditionResults.tsserver/services/notificationsService/notificationsService.tsserver/services/reportingService/ReportingRules.tsserver/services/ruleHistoryService/ruleHistoryService.tsserver/test/arbitraries/ContentType.tsserver/utils/kyselyTransactionWithRetry.test.tsserver/utils/kyselyTransactionWithRetry.tsserver/utils/url.tsserver/workers_jobs/RefreshMRTDecisionsMaterializedViewJob.ts
💤 Files with no reviewable changes (1)
- server/.env.example
CodeRabbit and Copilot flagged several cross-tenant data-loss and
result-discarding bugs in delete flows. Addressing them here:
- `ReportingRules.deleteReportingRule`: verify ownership in-txn before
touching any join rows (the join tables have no org column, so a
foreign-org id would otherwise wipe that other org's joins before the
scoped parent delete no-ops). Drop the broad `.catch(() => false)` so
unexpected DB errors propagate; only the verified not-found case
returns false.
- `QueueOperations.deleteManualReviewQueue` (+ the
`ForTestsDO_NOT_USE` variant): serialize the deletes and bail before
touching `users_and_accessible_queues` when the org-scoped queue
delete matches 0 rows. The previous `Promise.all` ran the unscoped
join delete concurrently, so a foreign queueId would wipe another
org's access rows even when the parent delete no-ops.
- `RuleApi.deleteRule`: propagate the boolean from `kyselyDeleteRule`
(which already returns false on org-mismatch) instead of returning
`true` whenever no exception is thrown.
- `JobRouting.deleteRoutingRule` / `AppealsJobRouting.deleteAppealsRoutingRule`:
thread `orgId` through the service wrapper and the GraphQL resolver,
add `where('org_id', '=', orgId)` to match the update path. Updated
the test callers accordingly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context & Requests for Reviewers
Removes dead lint rule for
unmanagedSequelizeTransactionSelectorand adds a new one for KyselymakeKyselyTransactionWithRetry. As part of this I also moved all direct callers of kyely execute to use the wrapped Transaction query.This has no real behavior change, but moves towards better lint rules and code standards when working with kysely.
Summary by CodeRabbit
Bug Fixes
Improvements
Tests