Fix invalid delete on rollback - #555
Conversation
WalkthroughThis update introduces additional filtering to SQL queries in entity history rollback and pruning functions, ensuring that rows with both Changes
Sequence Diagram(s)sequenceDiagram
participant TestSuite as Test Suite
participant DB as Database
participant EntityHistory as Entity History Logic
TestSuite->>DB: Reset and migrate database
TestSuite->>EntityHistory: Insert mock entities and history rows
EntityHistory->>DB: Insert entities/history (with possible chain_id=0)
TestSuite->>EntityHistory: Trigger rollback/prune operations
EntityHistory->>DB: Execute SQL with filtering (exclude chain_id=0 & timestamp=0)
DB-->>EntityHistory: Return filtered results
EntityHistory-->>TestSuite: Provide rollback/prune outcome
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
codegenerator/cli/templates/static/codegen/src/db/DbFunctionsImplementation.js (2)
477-482: Rollback-diff query repeats the filterThe filter is re-introduced here. Switching to the shared constant and row comparison keeps all three queries consistent and index-friendly.
409-414: 🛠️ Refactor suggestionSame predicate duplicated – DRY & indexability
Same remarks as above apply inside the
pruneStaleEntityHistoryCTE. Re-use the shared constant to avoid drifting implementations.
🧹 Nitpick comments (2)
scenarios/test_codegen/test/lib_tests/EntityHistory_test.res (2)
219-225: Extremely brittle string-equality assertionA one-to-one equality against the full
CREATE TABLEDDL will fail on harmless changes (e.g. re-ordering fields, formatting, or adding an index).
Consider asserting only the essential parts:let createQuery = … Assert.ok(createQuery->Js.String.includes("\"TestEntity_history\"")) Assert.ok(createQuery->Js.String.includes("\"serial\" SERIAL"))This preserves intent while avoiding noisy test failures.
718-735: Test suite is performing full DB migrations in every iterationRunning down/up migrations for each test is ~seconds per call and dominates CI time. Group the “setup schema” phase into a
beforeAlland use transactions/ROLLBACKinbeforeEachto restore a clean slate instantaneously:Async.beforeAll(async () => { /* run migrations once */ }) Async.beforeEach(async () => await Db.sql->Postgres.beginSql(sql => [sql`BEGIN`])) Async.afterEach(async () => await Db.sql->Postgres.unsafe("ROLLBACK"))This keeps isolation while slashing execution time.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
scenarios/test_codegen/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
codegenerator/cli/templates/static/codegen/src/db/DbFunctionsImplementation.js(20 hunks)scenarios/test_codegen/test/lib_tests/EntityHistory_test.res(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build_and_test
| -- Filter out rows with a chain_id of 0 since they are the copied history rows | ||
| -- check timestamp as well in case a future chain is added with id of 0 | ||
| AND NOT ( | ||
| entity_history_chain_id = 0 AND | ||
| entity_history_block_timestamp = 0 | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consolidate the “copied history” filter for maintainability and speed
The duplicated 4-line predicate appears in three separate queries. Besides the maintenance overhead, row-wise AND/NOT comparisons prevent Postgres from using multi-column indexes efficiently.
- AND NOT (
- entity_history_chain_id = 0 AND
- entity_history_block_timestamp = 0
- );
+ -- `(row) <> (0,0)` lets the planner leverage a composite index
+ AND (entity_history_chain_id, entity_history_block_timestamp) <> (0, 0);Consider extracting this into:
const copiedHistoryRowFilter = sql`
(entity_history_chain_id, entity_history_block_timestamp) <> (0, 0)
`;and re-using it in all queries.
This both DRYs the code and makes adding a composite index on (entity_history_chain_id, entity_history_block_timestamp) straight-forward.
🤖 Prompt for AI Agents
In
codegenerator/cli/templates/static/codegen/src/db/DbFunctionsImplementation.js
around lines 369 to 374, the filter excluding copied history rows is duplicated
across multiple queries using separate AND/NOT conditions, which hinders
maintainability and index usage. Refactor by defining a single reusable
constant, copiedHistoryRowFilter, that uses a tuple comparison to exclude rows
where (entity_history_chain_id, entity_history_block_timestamp) equals (0, 0).
Replace all occurrences of the original filter with this constant to DRY the
code and enable efficient multi-column indexing.
There was a problem hiding this comment.
Actually not a bad optimisation suggestion, but I think the query engine planner should be able to leverage a composite in both cases. Either way need to add a composite for this. Not sure it's necessary for a low amount of rows in a reorg threshold where this operates. So probably just a 👎🏼 from me
There was a problem hiding this comment.
@JonoPrest Fair point! You're absolutely right that with the low row counts in reorg thresholds, the performance difference would be negligible. The query planner should indeed handle both cases similarly, especially once you add that composite index. Thanks for the consideration and the clear reasoning - makes sense to keep it as is! 👍
| Async.it("Rollback ignores copied entities as an item in reorg threshold", async () => { | ||
| let rollbackDiff = await Db.sql->DbFunctions.EntityHistory.getRollbackDiff( | ||
| OrderedMultichain({ | ||
| reorgChainId: Mocks.GnosisBug.chain_id, | ||
| safeBlockNumber: 9, | ||
| safeBlockTimestamp: 9 * 5, | ||
| }), | ||
| ~entityMod=module(TestEntity), | ||
| ) | ||
|
|
||
| let expectedDiff: array<EntityHistory.historyRow<TestEntity.t>> = [ | ||
| { | ||
| current: {chain_id: 0, block_timestamp: 0, block_number: 0, log_index: 0}, | ||
| previous: %raw(`undefined`), | ||
| entityData: Set(Mocks.Entity.mockEntity1), | ||
| }, | ||
| { | ||
| current: {chain_id: 0, block_timestamp: 0, block_number: 0, log_index: 0}, | ||
| previous: %raw(`undefined`), | ||
| entityData: Set(Mocks.Entity.mockEntity5), | ||
| }, | ||
| ] | ||
|
|
||
| Assert.deepStrictEqual( | ||
| rollbackDiff, | ||
| expectedDiff, | ||
| ~message="Should rollback to the copied entity", | ||
| ) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Order-sensitive deep comparison may flicker
DbFunctions.EntityHistory.getRollbackDiff does not guarantee deterministic ordering of diff rows. deepStrictEqual on the raw array will intermittently fail if Postgres returns rows in a different order.
Instead, sort by entityData.id or current.serial before comparing:
let cmp = (a,b) => Js.String.compare(a.EntityHistory.entityData.getId, b.EntityHistory.entityData.getId)
Assert.deepStrictEqual(rollbackDiff->Js.Array2.sortInPlaceWith(cmp), expectedDiff->Js.Array2.sortInPlaceWith(cmp))🤖 Prompt for AI Agents
In scenarios/test_codegen/test/lib_tests/EntityHistory_test.res around lines 631
to 659, the test uses deepStrictEqual to compare rollbackDiff and expectedDiff
arrays directly, but the order of rows returned by getRollbackDiff is not
guaranteed, causing intermittent test failures. To fix this, sort both
rollbackDiff and expectedDiff arrays by a stable key such as entityData.id or
current.serial before comparing them with deepStrictEqual, ensuring
order-insensitive comparison.
There was a problem hiding this comment.
Should not be the case for this isolated test
There was a problem hiding this comment.
@JonoPrest You're absolutely right. In this isolated test scenario with controlled mock data and a clean database setup, the ordering should be deterministic. The test creates a specific controlled environment where the getRollbackDiff results should be predictable based on the exact mock entities and history rows being inserted.
Since this test runs in isolation with the beforeEach setup resetting the state and inserting specific controlled data, there shouldn't be any non-deterministic ordering issues. My suggestion was overly cautious for this particular test context.
/| /|
( :v: :v: )
|(_)(_)|
| |
\ /
\_ _/
^^^
✏️ Learnings added
Learnt from: JonoPrest
PR: enviodev/hyperindex#555
File: scenarios/test_codegen/test/lib_tests/EntityHistory_test.res:631-659
Timestamp: 2025-05-27T17:07:12.870Z
Learning: In isolated tests for EntityHistory rollbacks with controlled mock data setup, the ordering of getRollbackDiff results is deterministic and doesn't require sorting before comparison with deepStrictEqual.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| describe("Entity history rollbacks", () => { | ||
| Async.beforeEach(async () => { | ||
| try { | ||
| let _ = DbHelpers.resetPostgresClient() | ||
| let _ = await Migrations.runDownMigrations(~shouldExit=false) | ||
| let _ = await Migrations.createEnumIfNotExists(Db.sql, EntityHistory.RowAction.enum) | ||
| let _ = await Migrations.creatTableIfNotExists(Db.sql, TestEntity.table) | ||
| let _ = await Migrations.creatTableIfNotExists(Db.sql, TestEntity.entityHistory.table) | ||
|
|
||
| let _ = await Db.sql->Postgres.unsafe(TestEntity.entityHistory.createInsertFnQuery) | ||
|
|
||
| try await Db.sql->DbFunctionsEntities.batchSet(~entityMod=module(TestEntity))([ | ||
| Mocks.Entity.mockEntity1, | ||
| Mocks.Entity.mockEntity5, | ||
| ]) catch { | ||
| | exn => | ||
| Js.log2("batchSet mock entity exn", exn) | ||
| Assert.fail("Failed to set mock entity in table") | ||
| } | ||
|
|
||
| try await Db.sql->Postgres.beginSql( | ||
| sql => [ | ||
| TestEntity.entityHistory->EntityHistory.batchInsertRows( | ||
| ~sql, | ||
| ~rows=Mocks.GnosisBug.historyRows, | ||
| ), | ||
| ], | ||
| ) catch { | ||
| | exn => | ||
| Js.log2("insert mock rows exn", exn) | ||
| Assert.fail("Failed to insert mock rows") | ||
| } | ||
|
|
||
| let historyItems = { | ||
| let items = await Db.sql->getAllMockEntityHistory | ||
| items->S.parseJsonOrThrow(TestEntity.entityHistory.schemaRows) | ||
| } | ||
| Assert.equal(historyItems->Js.Array2.length, 4, ~message="Should have 4 history items") | ||
| Assert.ok( | ||
| historyItems->Belt.Array.some(item => item.current.chain_id == 0), | ||
| ~message="Should contain 2 copied items", | ||
| ) | ||
| } catch { | ||
| | exn => | ||
| Js.log2(" Entity history setup exn", exn) | ||
| Assert.fail("Failed setting up tables") | ||
| } | ||
| }) |
There was a problem hiding this comment.
beforeEach does not await DbHelpers.resetPostgresClient()
resetPostgresClient() is called without await, so subsequent statements can race with the connection teardown, leading to rare “connection already closed” failures on busy CI runners.
- try {
- let _ = DbHelpers.resetPostgresClient()
+ try {
+ await DbHelpers.resetPostgresClient()Ensuring the promise resolves keeps the setup deterministic.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| describe("Entity history rollbacks", () => { | |
| Async.beforeEach(async () => { | |
| try { | |
| let _ = DbHelpers.resetPostgresClient() | |
| let _ = await Migrations.runDownMigrations(~shouldExit=false) | |
| let _ = await Migrations.createEnumIfNotExists(Db.sql, EntityHistory.RowAction.enum) | |
| let _ = await Migrations.creatTableIfNotExists(Db.sql, TestEntity.table) | |
| let _ = await Migrations.creatTableIfNotExists(Db.sql, TestEntity.entityHistory.table) | |
| let _ = await Db.sql->Postgres.unsafe(TestEntity.entityHistory.createInsertFnQuery) | |
| try await Db.sql->DbFunctionsEntities.batchSet(~entityMod=module(TestEntity))([ | |
| Mocks.Entity.mockEntity1, | |
| Mocks.Entity.mockEntity5, | |
| ]) catch { | |
| | exn => | |
| Js.log2("batchSet mock entity exn", exn) | |
| Assert.fail("Failed to set mock entity in table") | |
| } | |
| try await Db.sql->Postgres.beginSql( | |
| sql => [ | |
| TestEntity.entityHistory->EntityHistory.batchInsertRows( | |
| ~sql, | |
| ~rows=Mocks.GnosisBug.historyRows, | |
| ), | |
| ], | |
| ) catch { | |
| | exn => | |
| Js.log2("insert mock rows exn", exn) | |
| Assert.fail("Failed to insert mock rows") | |
| } | |
| let historyItems = { | |
| let items = await Db.sql->getAllMockEntityHistory | |
| items->S.parseJsonOrThrow(TestEntity.entityHistory.schemaRows) | |
| } | |
| Assert.equal(historyItems->Js.Array2.length, 4, ~message="Should have 4 history items") | |
| Assert.ok( | |
| historyItems->Belt.Array.some(item => item.current.chain_id == 0), | |
| ~message="Should contain 2 copied items", | |
| ) | |
| } catch { | |
| | exn => | |
| Js.log2(" Entity history setup exn", exn) | |
| Assert.fail("Failed setting up tables") | |
| } | |
| }) | |
| describe("Entity history rollbacks", () => { | |
| Async.beforeEach(async () => { | |
| try { | |
| await DbHelpers.resetPostgresClient() | |
| let _ = await Migrations.runDownMigrations(~shouldExit=false) | |
| let _ = await Migrations.createEnumIfNotExists(Db.sql, EntityHistory.RowAction.enum) | |
| let _ = await Migrations.creatTableIfNotExists(Db.sql, TestEntity.table) | |
| let _ = await Migrations.creatTableIfNotExists(Db.sql, TestEntity.entityHistory.table) | |
| let _ = await Db.sql->Postgres.unsafe(TestEntity.entityHistory.createInsertFnQuery) | |
| try await Db.sql->DbFunctionsEntities.batchSet(~entityMod=module(TestEntity))([ | |
| Mocks.Entity.mockEntity1, | |
| Mocks.Entity.mockEntity5, | |
| ]) catch { | |
| | exn => | |
| Js.log2("batchSet mock entity exn", exn) | |
| Assert.fail("Failed to set mock entity in table") | |
| } | |
| try await Db.sql->Postgres.beginSql( | |
| sql => [ | |
| TestEntity.entityHistory->EntityHistory.batchInsertRows( | |
| ~sql, | |
| ~rows=Mocks.GnosisBug.historyRows, | |
| ), | |
| ], | |
| ) catch { | |
| | exn => | |
| Js.log2("insert mock rows exn", exn) | |
| Assert.fail("Failed to insert mock rows") | |
| } | |
| let historyItems = { | |
| let items = await Db.sql->getAllMockEntityHistory | |
| items->S.parseJsonOrThrow(TestEntity.entityHistory.schemaRows) | |
| } | |
| Assert.equal(historyItems->Js.Array2.length, 4, ~message="Should have 4 history items") | |
| Assert.ok( | |
| historyItems->Belt.Array.some(item => item.current.chain_id == 0), | |
| ~message="Should contain 2 copied items", | |
| ) | |
| } catch { | |
| | exn => | |
| Js.log2(" Entity history setup exn", exn) | |
| Assert.fail("Failed setting up tables") | |
| } | |
| }) |
🤖 Prompt for AI Agents
In scenarios/test_codegen/test/lib_tests/EntityHistory_test.res around lines 582
to 629, the call to DbHelpers.resetPostgresClient() inside the beforeEach hook
is missing an await, causing potential race conditions with connection teardown.
Fix this by adding await before DbHelpers.resetPostgresClient() to ensure the
promise resolves before proceeding with subsequent setup steps.
There was a problem hiding this comment.
🧩 Analysis chain
I need to investigate the DbHelpers.resetPostgresClient() function to understand why my analysis was incorrect.
🏁 Script executed:
#!/bin/bash
# Find and examine the DbHelpers.resetPostgresClient function implementation
ast-grep --pattern 'resetPostgresClient($_) {
$$$
}'
Length of output: 56
🏁 Script executed:
#!/bin/bash
# Search for resetPostgresClient function definition
rg "resetPostgresClient" -A 10 -B 2
Length of output: 6496
🏁 Script executed:
#!/bin/bash
# Find DbHelpers module files
fd "DbHelpers" --type f
Length of output: 127
@JonoPrest You're absolutely right - my analysis was incorrect.
Looking at the implementation, DbHelpers.resetPostgresClient has the signature unit => unit, making it a synchronous function that doesn't return a promise:
let resetPostgresClient: unit => unit = () => {
// This is a hack to reset the postgres client between tests. postgres.js seems to cache some types, and if tests clear the DB you need to also reset sql.
%raw("require('../../generated/src/db/Db.bs.js').sql = require('postgres')(_sqlConfig)")
}The function performs a synchronous JavaScript operation to reassign the SQL client, so adding await would be incorrect. I apologize for the false positive.
🏁 Script executed:
#!/bin/bash
# Let's examine the function signature more clearly
cat scenarios/test_codegen/test/helpers/DbHelpers.res
Length of output: 541
@JonoPrest You're absolutely right - my analysis was incorrect.
Looking at the implementation, DbHelpers.resetPostgresClient has the signature unit => unit, making it a synchronous function that doesn't return a promise:
let resetPostgresClient: unit => unit = () => {
// This is a hack to reset the postgres client between tests. postgres.js seems to cache some types, and if tests clear the DB you need to also reset sql.
%raw("require('../../generated/src/db/Db.bs.js').sql = require('postgres')(_sqlConfig)")
}The function performs a synchronous JavaScript operation to reassign the SQL client, so adding await would be incorrect. I apologize for the false positive.
✏️ Learnings added
Learnt from: JonoPrest
PR: enviodev/hyperindex#555
File: scenarios/test_codegen/test/lib_tests/EntityHistory_test.res:582-629
Timestamp: 2025-05-27T17:07:04.658Z
Learning: In ReScript test files using DbHelpers.resetPostgresClient(), this function is synchronous with signature `unit => unit` and should not be awaited. It performs a raw JavaScript reassignment of the postgres client and does not return a promise.
Learnt from: JonoPrest
PR: enviodev/hyperindex#555
File: scenarios/test_codegen/test/lib_tests/EntityHistory_test.res:582-629
Timestamp: 2025-05-27T17:07:04.658Z
Learning: In ReScript test files using DbHelpers.resetPostgresClient(), this function is synchronous with signature `unit => unit` and should not be awaited. It performs a raw JavaScript reassignment of the postgres client and does not return a promise.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
DZakh
left a comment
DZakh
left a comment
There was a problem hiding this comment.
Nice 🥳 I'm still reviewing, but approve before hand, if I won't internet later
2 participants
The issue was in multiple copies of an different entities into entity history within a batch and these copies were not filtered out of the diffing queries.
I've created a sequence diagram to explain the case and I've added tests to validate that it now works.
Summary by CodeRabbit