Librarify persistence layer#558
Conversation
WalkthroughThis update introduces a new persistence abstraction for managing database and in-memory state, refactors enum and entity configuration handling, and centralizes initialization logic for storage and migrations. It adds PostgreSQL schema generation utilities, restructures configuration and migration flows, and updates related tests. Several obsolete or redundant functions and files are removed. Changes
Sequence Diagram(s)sequenceDiagram
participant Main
participant Persistence
participant Storage
participant Callback
Main->>Persistence: init(~skipIsInitializedCheck, ~reset)
alt Already Initializing
Persistence-->>Main: Await existing promise
else Ready and not reset
Persistence-->>Main: Return (no-op)
else Needs Initialization
Persistence->>Storage: isInitialized()
alt Already initialized
Persistence-->>Main: Set status to Ready(cleanRun=false)
else Not initialized or reset
Persistence->>Storage: initialize(~entities, ~staticTables, ~enums, ~cleanRun)
alt onStorageInitialize exists
Persistence->>Callback: onStorageInitialize()
end
Persistence-->>Main: Set status to Ready(cleanRun=true)
end
end
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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: 7
🧹 Nitpick comments (10)
codegenerator/cli/templates/static/codegen/src/Config.res (1)
81-88: Excellent architectural improvement with persistence abstraction.The new
codegenPersistenceconstruction centralizes all persistence concerns:
- User entities and dynamic contract registry entities
- All enums with proper internal configuration casting
- PostgreSQL storage backend integration
- Clean separation between configuration and storage concerns
This represents a significant improvement in code organization and maintainability.
Note: The
Utils.magiccast for enums on line 86 may be necessary due to type system limitations, but consider adding a comment explaining this type conversion.codegenerator/cli/npm/envio/src/Persistence.res (1)
54-56: Avoid%raw("null")for safer, typed initialisationUsing a raw
nullcan lead to runtimeundefined is not a functionerrors if the promise is resolved beforeresolveRefis mutated. Prefer() => ()as a typed no-op placeholder (as shown in the diff above).codegenerator/cli/templates/static/codegen/src/db/DbFunctionsEntities.res (2)
75-86:WeakMapkeying by record may miss cache hits
Internal.entityConfigis a record; every call site that constructs an identical record literal produces a fresh JS object.
BecauseWeakMapkeys are compared by reference, two logically identical configs will not hit the cache unless the same instance is reused.If the codebase recreates configs (e.g. via
Entities.entityModToInternal), the cache degenerates and the batch‐set query is rebuilt for every call.Possible remedies:
- Store the generated function inside the
entityConfigitself, or- Use a stable primitive key (e.g. table name) for the
WeakMap.
109-146: SanitisefieldNamebefore passing to external SQL helpers
fieldNameis interpolated into log params and forwarded to JS externs.
Although callers are expected to use compile-time constants, adding a simple whitelist check (againstentityConfig.table.columnNames) will guard against accidental injection when new code paths are added.codegenerator/cli/templates/static/codegen/src/db/DbFunctions.res (2)
408-423: Unbounded parallel DB calls – consider throttling
allEntities->Belt.Array.map(async ...) |> Promise.allfires one concurrent query per entity.
With many entities this can:
- Exhaust the Postgres connection pool
- Cause large memory spikes on the JS side
- Reduce overall throughput due to contention
If the entity count can grow, introduce a small concurrency limiter:
let parallelMap = Concurrency.mapWithLimit(~limit=8) await allEntities ->parallelMap(async entityConfig => { /* body unchanged */ }) ->Promise.all(or reuse an existing rate-limiter util).
549-567: Minor:awaiton plain arrays is unnecessary
Entities.allEntitiesis already an array; the leadingawaitis a no-op and may mislead future readers into thinking it is a promise. Removing it improves readability with zero functional impact.codegenerator/cli/templates/dynamic/codegen/src/TestHelpers_MockDb.res.hbs (2)
296-299:loggerparameter is shadowed and never used
~logger as _=?discards the supplied logger. Either remove the argument or forward it to child calls so users can debug failed entity loads.- (ids, ~entityConfig, ~logger as _=?) => { + (ids, ~entityConfig, ~logger=?) => { let operations = mockDb->getEntityOperations(~entityConfig) - ids->Array.keepMap(id => operations.get(id))->Promise.resolve + ids + ->Array.keepMap(id => operations.get(id)) + ->Promise.resolve + ->Promise.tap(_ => + switch logger { + | Some(l) => l->Pino.debug("Loaded " ++ string_of_int(Array.length(ids)) ++ " entities") + | None => () + } + ) }
312-318: Unusual partial-application pattern harms readability
mockDbTable->InMemoryTable.Entity.hasIndex(~fieldName, ~operator))(fieldValueHash)nests a second pair of parentheses, implying thathasIndexreturns a function. This isn’t obvious to a reader skimming the code.A flatter style improves clarity:
- if (mockDbTable->InMemoryTable.Entity.hasIndex(~fieldName, ~operator))(fieldValueHash) { + let hasIdx = + mockDbTable->InMemoryTable.Entity.hasIndex(~fieldName, ~operator, fieldValueHash) + if hasIdx {Consider refactoring
hasIndexso the full arity is explicit; this yields the same perf but better comprehension.codegenerator/cli/templates/static/codegen/src/InMemoryStore.res (1)
21-35: Return type relies on unsafe cast; can we eliminateUtils.magic?
EntityTables.getdown-castsInMemoryTable.Entity.t<Entities.internalEntity>toInMemoryTable.Entity.t<entity>viaUtils.magic. If callers pass anentityConfigthat does not correspond to'entity, we’ll corrupt memory safety and get runtime exceptions later.A safer design is to return the untyped table and let callers perform the cast in one place, or add a phantom type field to
entityConfigso the compiler can verify the mapping.If keeping the cast, at least document the invariant above the function to alert future maintainers.
codegenerator/cli/templates/static/codegen/src/LoadLayer.resi (1)
8-14: Interface doc blocks still referenceentityModWhile the types were migrated to
entityConfig, the surrounding docs/examples (and possibly downstream tutorials) still mentionentityMod. This can confuse integrators reading the.resi.Please update the docstrings to reflect the new API surface.
📜 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 (28)
.cursor/rules/rescript.mdc(1 hunks)codegenerator/cli/npm/envio/src/Internal.res(1 hunks)codegenerator/cli/npm/envio/src/Logging.res(2 hunks)codegenerator/cli/npm/envio/src/Persistence.res(1 hunks)codegenerator/cli/npm/envio/src/PgStorage.res(1 hunks)codegenerator/cli/templates/dynamic/codegen/src/RegisterHandlers.res.hbs(0 hunks)codegenerator/cli/templates/dynamic/codegen/src/TestHelpers_MockDb.res.hbs(4 hunks)codegenerator/cli/templates/dynamic/codegen/src/db/Entities.res.hbs(2 hunks)codegenerator/cli/templates/static/codegen/src/Config.res(3 hunks)codegenerator/cli/templates/static/codegen/src/Env.res(1 hunks)codegenerator/cli/templates/static/codegen/src/IO.res(8 hunks)codegenerator/cli/templates/static/codegen/src/InMemoryStore.res(5 hunks)codegenerator/cli/templates/static/codegen/src/Index.res(1 hunks)codegenerator/cli/templates/static/codegen/src/LoadLayer.res(6 hunks)codegenerator/cli/templates/static/codegen/src/LoadLayer.resi(1 hunks)codegenerator/cli/templates/static/codegen/src/UserContext.res(9 hunks)codegenerator/cli/templates/static/codegen/src/db/Db.res(1 hunks)codegenerator/cli/templates/static/codegen/src/db/DbFunctions.res(8 hunks)codegenerator/cli/templates/static/codegen/src/db/DbFunctionsEntities.res(4 hunks)codegenerator/cli/templates/static/codegen/src/db/Migrations.res(1 hunks)codegenerator/cli/templates/static/codegen/src/globalState/GlobalState.res(1 hunks)scenarios/test_codegen/test/LoadLayer_test.res(18 hunks)scenarios/test_codegen/test/Mock_test.res(3 hunks)scenarios/test_codegen/test/SerDe_Test.res(6 hunks)scenarios/test_codegen/test/helpers/Mock.res(2 hunks)scenarios/test_codegen/test/lib_tests/EntityHistory_test.res(13 hunks)scenarios/test_codegen/test/schema_types/BigDecimal_test.res(1 hunks)scenarios/test_codegen/test/schema_types/Timestamp_test.res(1 hunks)
💤 Files with no reviewable changes (1)
- codegenerator/cli/templates/dynamic/codegen/src/RegisterHandlers.res.hbs
🧰 Additional context used
🧠 Learnings (1)
scenarios/test_codegen/test/lib_tests/EntityHistory_test.res (1)
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.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build_and_test
🔇 Additional comments (52)
codegenerator/cli/templates/static/codegen/src/Env.res (1)
102-104: LGTM! Clean separation of logger creation and setting.The refactoring properly separates logger creation (
makeLogger) from logger assignment (setLogger), improving the separation of concerns and making the code more modular..cursor/rules/rescript.mdc (1)
1-10: Excellent addition of development tooling configuration.This Cursor rules file will help maintain consistency across the codebase by:
- Ensuring developers reference current ReScript 11 documentation
- Preventing confusion by avoiding ReasonML syntax suggestions
- Enforcing modern array syntax (
[ item ]vs[| item |])- Clarifying field assignment conventions (
=vs:=for refs)This type of tooling configuration is valuable for team development and code quality.
codegenerator/cli/templates/static/codegen/src/globalState/GlobalState.res (1)
802-808: LGTM! Consistent with the entity handling refactoring.The change from
entityModtoentityConfigwith direct access toentityConfig.nameis consistent with the broader architectural refactor moving from module-based entity handling to explicit configuration records. This approach:
- Eliminates the need for module destructuring (
module Entity = entityMod)- Provides direct access to entity metadata via
entityConfig.name- Maintains the same functionality while using a cleaner data structure
The change aligns well with the introduction of
Internal.entityConfigrecords throughout the codebase.scenarios/test_codegen/test/LoadLayer_test.res (2)
10-10: LGTM: Consistent API migration from entityMod to entityConfig.The systematic replacement of
~entityMod=module(Entities.User)with~entityConfig=module(Entities.User)->Entities.entityModToInternalacross all LoadLayer function calls is well-executed and maintains consistency throughout the test suite.Also applies to: 40-40, 77-77, 108-108, 156-156, 197-197, 238-238, 290-290, 334-334, 345-345, 405-405, 417-417, 501-501
206-206: LGTM: Proper entity casting in assertions.The updates to cast entities using
Entities.User.castToInternalin assertions correctly align with the new internal entity representation pattern. This ensures test assertions work properly with the refactored persistence layer.Also applies to: 257-257, 427-432, 455-456, 532-532
scenarios/test_codegen/test/schema_types/Timestamp_test.res (1)
22-24: LGTM: Correct migration to entityConfig-based persistence.The update to
DbFunctionsEntities.batchSetproperly adopts the new API pattern:
- Uses
~entityConfigwithEntities.entityModToInternaltransformation- Casts entity to internal representation via
castToInternalbefore persistenceThis aligns perfectly with the DI refactor objectives.
scenarios/test_codegen/test/schema_types/BigDecimal_test.res (1)
27-35: LGTM: Consistent entityConfig migration for multiple entities.The migration correctly handles multiple entities:
- Proper use of
~entityConfigparameter withEntities.entityModToInternal- Both entities correctly cast to internal representation using
castToInternal- Array syntax properly maintained
This demonstrates the refactor works well for batch operations.
codegenerator/cli/templates/static/codegen/src/db/Db.res (3)
19-21: LGTM: Simplified entity table access pattern.The direct access to
entityConfig.tableremoves unnecessary module aliasing and aligns with the new entityConfig-based architecture. This simplifies the code while maintaining the same functionality.
26-27: LGTM: Consistent entityHistory handling.The changes properly adapt entity history processing to use
entityConfig.entityHistorydirectly, maintaining consistency with the table access pattern above.
19-30:Details
❓ Verification inconclusive
Verify integration with Entities.allEntities structure.
Ensure that
Entities.allEntitiesreturns the appropriateentityConfigrecords that contain the expectedtableandentityHistoryfields. The refactor appears sound, but the integration point should be verified.
🏁 Script executed:
#!/bin/bash # Description: Verify that Entities.allEntities provides entityConfig records with required fields # Expected: Find definitions showing entityConfig type and allEntities structure # Search for entityConfig type definition ast-grep --pattern 'type entityConfig = $_' # Search for allEntities definition ast-grep --pattern 'let allEntities = $_' # Search for entityModToInternal function ast-grep --pattern 'let entityModToInternal = $_'Length of output: 141
🏁 Script executed:
#!/bin/bash # Broaden the search to locate entityConfig, allEntities, and entityHistory definitions # Search for entityConfig occurrences with context rg --color=always -n -C2 'entityConfig' . # Search for the definition of allEntities rg --color=always -n -C2 'let allEntities' . # Search for any EntityConfig type declaration (case‐insensitive) rg --color=always -i -n -C2 'type.*entityconfig' . # Search for entityHistory occurrences with context rg --color=always -n -C2 'entityHistory' .Length of output: 79220
Verify
Entities.allEntitiesRecord ShapeEnsure that your generated
Entitiesmodule definesallEntitiesas an array ofentityConfigrecords whose shape matches every consumer in the CLI templates:
- In
codegenerator/cli/templates/static/codegen/src/db/Db.res(lines 19–30), eachentityConfigmust have:
.table: Table.table.entityHistory: EntityHistory.t<…>with a.tablefield- In
DbFunctionsEntities.res, theentityConfigis deconstructed for.table,.schema, and.rowsSchema- In LoadLayer & UserContext templates,
entityConfigis passed to functions expecting:
.name(for cache keys).entityHistory.schemaRows(for parsing history diffs)Please review the definition in
codegenerator/cli/templates/dynamic/codegen/src/db/Entities.res.hbs
(or the generatedEntities.res) to confirm that theentityConfigtype includes all required fields and thatallEntitiesis correctly built from it.scenarios/test_codegen/test/SerDe_Test.res (6)
57-59: LGTM! Consistent with entity configuration refactoring.The change from
~entityMod=module(...)to~entityConfig=module(...)->Entities.entityModToInternalfollows the expected pattern for the persistence layer DI refactoring.
61-63: LGTM! Consistent entity configuration pattern.The batchRead function correctly follows the same refactoring pattern as batchSet.
75-75: LGTM! Proper internal casting for database operations.Converting entities to their internal representation using
castToInternalbefore database insertion is the correct approach for the new persistence layer.
85-85: LGTM! Assertion correctly compares internal representations.Since database operations now use internal entity representations, comparing the read entity against the internally cast version is the correct approach.
155-157: LGTM! Consistent entity configuration pattern in second test.The second test case correctly follows the same refactoring pattern for entity configuration.
159-161: LGTM! Consistent application of refactoring patterns.All remaining changes in the second test case correctly follow the same entity configuration and internal casting patterns established in the first test case.
Also applies to: 176-176, 186-187
codegenerator/cli/npm/envio/src/Internal.res (2)
163-169: LGTM! Well-designed entity configuration record.The
entityConfigrecord provides a clean abstraction that encapsulates all necessary entity metadata including schema, table reference, and history tracking. This is a good foundation for the DI refactoring.
170-171: LGTM! Proper enum type abstraction.The abstract
enumtype provides type safety, and theenumConfigalias creates a consistent interface for enum configuration that aligns with the entity configuration approach.codegenerator/cli/npm/envio/src/Logging.res (2)
29-73: LGTM! Improved separation of concerns in logger creation.Renaming
setLoggertomakeLoggerand making it return a logger instance instead of setting it directly improves the functional design by separating logger creation from assignment. The logger creation logic remains intact and correct.
75-77: LGTM! Clean logger assignment interface.The new
setLoggerfunction provides a clean interface for logger assignment, completing the separation between logger creation and assignment responsibilities.scenarios/test_codegen/test/Mock_test.res (3)
48-56: LGTM! Consistent entity configuration refactoring pattern.The changes follow the established pattern of using
entityConfigwithentityModToInternaltransformation and applyingcastToInternalto entity data before database operations.
76-78: LGTM! In-memory store operations updated consistently.The in-memory store operations correctly use the new entity configuration approach, maintaining consistency with database operations.
91-91: LGTM! Test assertions correctly use internal entity representations.The test assertions properly compare against internally cast entities, ensuring consistency with how entities are stored and retrieved in the new system.
Also applies to: 99-99, 107-107
codegenerator/cli/templates/static/codegen/src/db/Migrations.res (1)
130-144: LGTM! Improved schema management logic.The refactored schema management provides better control over database initialization:
- Conditional schema dropping based on
resetparameter- Explicit separation of DROP CASCADE and CREATE operations
- Clear user permission grants for both postgres and public
This approach is more explicit and safer for different migration scenarios.
scenarios/test_codegen/test/helpers/Mock.res (2)
4-6: LGTM! Consistent entity configuration refactor.The systematic replacement of
entityModwithentityConfigand use ofEntities.entityModToInternalaligns perfectly with the persistence layer refactor objectives.
57-84: Excellent consistency in parameter updates.The LoadLayer module updates correctly transition from module-based to configuration-based entity handling:
- Parameter renaming from
~entityModto~entityConfig- Direct property access with
entityConfig.name- Maintains same functionality with improved type clarity
scenarios/test_codegen/test/lib_tests/EntityHistory_test.res (3)
58-58: Good addition of type-safe entity casting.The
castToInternalexternal function provides a clean way to convert fromTestEntity.ttoInternal.entityusing identity casting, maintaining type safety in the test suite.
595-600: Excellent test adaptation to new entity configuration.The test updates correctly use the new entity configuration approach:
entityConfigderived frommodule(TestEntity)->Entities.entityModToInternal- Entities cast to internal representation using
TestEntity.castToInternal- Maintains test logic while improving type consistency
642-643: Consistent entity configuration usage throughout tests.All test functions properly updated to use
entityConfigparameter, ensuring the test suite fully exercises the new persistence layer interface.codegenerator/cli/templates/static/codegen/src/Config.res (2)
97-97: Clean transition from entity array to persistence abstraction.Replacing the
entitiesfield withpersistence: Persistence.tprovides a much cleaner interface that encapsulates entity management, enum configurations, and storage details in a single abstraction.
106-106: Good default parameter approach for backward compatibility.Using
~persistence=codegenPersistenceas the default parameter maintains backward compatibility while enabling the new persistence abstraction. This allows for gradual migration and testing of the new architecture.codegenerator/cli/templates/static/codegen/src/UserContext.res (4)
63-99: LGTM! Clean refactoring from entityMod to entityConfig.The function signature and implementation have been correctly updated to use
Internal.entityConfiginstead of the module-based approach. The entity access patterns are consistent throughout the function.
114-118: LGTM! Consistent entity lookup refactoring.The switch from
entityModtoentityConfigin the entity lookup logic is correctly implemented, maintaining the same error handling pattern.
133-143: LGTM! Proper type annotation update.The
loaderEntityContextParamstype has been correctly updated to useInternal.entityConfig, and the error message formatting consistently usesentityConfig.name.
216-224: LGTM! Consistent entity context creation.The loader context creation has been properly updated to pass
entityConfiginstead of the module-based approach, maintaining the same functionality.codegenerator/cli/templates/static/codegen/src/IO.res (4)
51-58: LGTM! Correct function signature update.The
executeSetEntityWithHistoryfunction signature has been properly updated to useInternal.entityConfig, and the entity table access is consistent with the new approach.
110-117: LGTM! Consistent database function calls.The database function calls have been correctly updated to pass
entityConfiginstead of the module-based approach, maintaining the same functionality for both batch operations.
179-181: LGTM! Clean entity iteration refactoring.The entity iteration has been properly updated to use
Entities.allEntitieswithentityConfig, simplifying the code while maintaining the same functionality.
238-257: LGTM! Rollback logic correctly updated.The rollback logic has been consistently updated to use
entityConfigfor entity operations, database queries, and dictionary access. The entity name access pattern usingentityConfig.nameis correct throughout.codegenerator/cli/templates/dynamic/codegen/src/db/Entities.res.hbs (4)
14-16: LGTM! Well-designed external function declarations.The external functions provide clean conversion between module types and internal entity configurations. The use of
%identityis appropriate since these are compile-time type conversions.
110-111: LGTM! Consistent castToInternal function addition.Adding
castToInternalto each entity module provides a clean way to convert entities to their internal representation, maintaining type safety.
114-124: LGTM! Logical entity array restructuring.The separation of user entities from static entities (like
DynamicContractRegistry) is a good architectural decision. The concatenation approach maintains all entities while allowing for different handling of user vs. system entities.
125-130: LGTM! Correct dictionary mapping update.The dictionary mapping has been properly updated to use
entityConfig.nameinstead of module destructuring, which is consistent with the new entity configuration approach.codegenerator/cli/templates/static/codegen/src/InMemoryStore.res (1)
13-18:EntityTables.makeshould guard against duplicateentityConfig.nameentriesWhen iterating
entities, duplicate names will silently overwrite the first table, leading to subtle data-loss at runtime. A quick check avoids this trap:- entities->Belt.Array.forEach(entityConfig => { - init->Js.Dict.set((entityConfig.name :> string), InMemoryTable.Entity.make()) - }) + entities->Belt.Array.forEach(entityConfig => { + let key = (entityConfig.name :> string) + if Js.Dict.has(init, key) { + UndefinedEntity({entityName: key}) + ->ErrorHandling.mkLogAndRaise(~msg="Duplicate entity name detected during store init") + } + init->Js.Dict.set(key, InMemoryTable.Entity.make()) + })codegenerator/cli/templates/static/codegen/src/LoadLayer.res (8)
9-11: LGTM! Function type definitions properly refactored.The function type definitions have been consistently updated to use
~entityConfig: Internal.entityConfiginstead of the previous module-based approach, and return types correctly changed topromise<array<Internal.entity>>. This maintains type safety while moving to the new dependency injection pattern.Also applies to: 14-19
34-37: Function implementation correctly updated for new signature.The
makeWithDbConnectionfunction properly passes theentityConfigparameter to the underlying database functions, maintaining consistency with the refactored function signatures.
42-49: loadById function signature and implementation correctly refactored.The function signature properly accepts
~entityConfig: Internal.entityConfigand the implementation correctly usesentityConfig.namefor key generation and passes the config toInMemoryStore.getInMemTable. This maintains the same functionality with the new architecture.
56-56: Consistent parameter passing in loadEntitiesByIds call.The
~entityConfigparameter is correctly passed to theloadEntitiesByIdsfunction call, maintaining consistency with the refactored function signature.
82-82: Direct function call approach is consistent with new architecture.The change from a curried/module-based approach to direct function calls (
inMemTable->InMemoryTable.Entity.getUnsafe) aligns with the broader refactoring to use explicit configuration records instead of modules. This maintains the same functionality while simplifying the interface.
123-135: loadByField function signature and key generation properly updated.The function signature correctly uses
~entityConfig: Internal.entityConfigand the key generation properly referencesentityConfig.name. TheInMemoryStore.getInMemTablecall is also updated consistently.
154-154: Consistent entityConfig parameter usage in loadEntitiesByField.The
~entityConfigparameter is correctly passed to theloadEntitiesByFieldfunction call, maintaining consistency throughout the refactoring.
183-183: Direct function call maintains functionality with new architecture.Similar to line 82, this change from module-based currying to direct function calls (
InMemoryTable.Entity.getUnsafeOnIndex) is consistent with the overall refactoring approach and maintains the same functionality.
| let setLogger = l => { | ||
| logger := Some(l) | ||
| } |
There was a problem hiding this comment.
Make it easier to mock in the future.
Enabler for testing logs.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
codegenerator/cli/npm/envio/src/Persistence.res (1)
68-111:⚠️ Potential issueCritical: Promise resolution issue still partially exists.
The promise stored in
Initializingstate is not resolved in all code paths:
- Line 92: When storage is already initialized,
resolveRef.contents()is not called- Lines 108-110: In the error handler,
resolveRef.contents()is not called before re-raisingThis will cause any concurrent calls awaiting the promise to hang indefinitely.
Apply this fix to ensure the promise is always resolved:
let init = async ( persistence, ~skipIsInitializedCheck=false, ~reset=false, ) => { try { let shouldRun = switch persistence.storageStatus { | Unknown => true | Initializing(promise) => { await promise reset } | Ready(_) => reset } if shouldRun { let resolveRef = ref(%raw(`null`)) let promise = Promise.make((resolve, _) => { resolveRef := resolve }) persistence.storageStatus = Initializing(promise) - if !(reset || skipIsInitializedCheck) && (await persistence.storage.isInitialized()) { - persistence.storageStatus = Ready({cleanRun: false}) - } else { - let _ = await persistence.storage.initialize( - ~entities=persistence.allEntities, - ~staticTables=persistence.staticTables, - ~enums=persistence.allEnums, - ~cleanRun=reset || !skipIsInitializedCheck, - ) - persistence.storageStatus = Ready({cleanRun: true}) - switch persistence.onStorageInitialize { - | Some(onStorageInitialize) => await onStorageInitialize() - | None => () + try { + if !(reset || skipIsInitializedCheck) && (await persistence.storage.isInitialized()) { + persistence.storageStatus = Ready({cleanRun: false}) + } else { + let _ = await persistence.storage.initialize( + ~entities=persistence.allEntities, + ~staticTables=persistence.staticTables, + ~enums=persistence.allEnums, + ~cleanRun=reset || !skipIsInitializedCheck, + ) + persistence.storageStatus = Ready({cleanRun: true}) + switch persistence.onStorageInitialize { + | Some(onStorageInitialize) => await onStorageInitialize() + | None => () + } } + resolveRef.contents() + } catch { + | exn => + resolveRef.contents() + raise(exn) } - resolveRef.contents() } } catch { | exn => exn->ErrorHandling.mkLogAndRaise(~msg=`EE800: Failed to initialize the indexer storage.`) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
codegenerator/cli/npm/envio/src/Enum.res(0 hunks)codegenerator/cli/npm/envio/src/Internal.res(1 hunks)codegenerator/cli/npm/envio/src/Persistence.res(1 hunks)codegenerator/cli/npm/envio/src/PgStorage.res(1 hunks)codegenerator/cli/npm/envio/src/db/EntityHistory.res(3 hunks)codegenerator/cli/src/config_parsing/postgres_types.rs(1 hunks)codegenerator/cli/src/rescript_types.rs(1 hunks)codegenerator/cli/templates/dynamic/codegen/src/db/Enums.res.hbs(3 hunks)codegenerator/cli/templates/static/codegen/src/Config.res(3 hunks)codegenerator/cli/templates/static/codegen/src/Index.res(1 hunks)codegenerator/cli/templates/static/codegen/src/db/Migrations.res(1 hunks)codegenerator/cli/templates/static/codegen/src/db/TablesStatic.res(3 hunks)scenarios/erc20_multichain_factory/test/TestDeleteEntity.res(0 hunks)scenarios/erc20_multichain_factory/test/TestWhereQuery.res(0 hunks)scenarios/test_codegen/test/SerDe_Test.res(1 hunks)scenarios/test_codegen/test/lib_tests/EntityHistory_test.res(5 hunks)scenarios/test_codegen/test/lib_tests/Persistence_test.res(1 hunks)scenarios/test_codegen/test/lib_tests/PgStorage_test.res(1 hunks)scenarios/test_codegen/test/rollback/Rollback_test.res(0 hunks)
💤 Files with no reviewable changes (4)
- scenarios/erc20_multichain_factory/test/TestWhereQuery.res
- scenarios/erc20_multichain_factory/test/TestDeleteEntity.res
- scenarios/test_codegen/test/rollback/Rollback_test.res
- codegenerator/cli/npm/envio/src/Enum.res
✅ Files skipped from review due to trivial changes (3)
- codegenerator/cli/src/rescript_types.rs
- scenarios/test_codegen/test/SerDe_Test.res
- codegenerator/cli/src/config_parsing/postgres_types.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- codegenerator/cli/templates/static/codegen/src/Index.res
- scenarios/test_codegen/test/lib_tests/EntityHistory_test.res
- codegenerator/cli/npm/envio/src/Internal.res
- codegenerator/cli/templates/static/codegen/src/Config.res
- codegenerator/cli/npm/envio/src/PgStorage.res
🔇 Additional comments (17)
codegenerator/cli/templates/static/codegen/src/db/TablesStatic.res (3)
34-39: LGTM: Formatting improvement for readability.The multi-line formatting of the
mkFieldcall enhances readability without changing functionality.
247-248: Approve enum configuration update.The change from
Enums.ContractType.enumtoEnums.ContractType.configis consistent with the enum refactoring pattern throughout the codebase.
227-227:Details
✅ Verification successful
Verify enum schema migration across codebase.
The change from
Enums.ContractType.enum.schematoEnums.ContractType.config.schemaaligns with the enum refactoring. Ensure all references to enum schemas have been consistently updated.
🏁 Script executed:
#!/bin/bash # Description: Verify all enum schema references have been updated consistently # Expected: No remaining references to .enum.schema pattern echo "Searching for any remaining .enum.schema references..." rg '\.enum\.schema' --type=res --type=hbs echo "Verifying .config.schema pattern is used consistently..." rg '\.config\.schema' --type=res --type=hbs -A 2Length of output: 392
🏁 Script executed:
#!/bin/bash # Re-run without unsupported --type filter, scoping to .res and .hbs files echo "Searching for any remaining '.enum.schema' references across .res and .hbs files..." rg '\.enum\.schema' -g '*.res' -g '*.hbs' echo "" echo "Verifying '.config.schema' usage across .res and .hbs files (with context)..." rg '\.config\.schema' -g '*.res' -g '*.hbs' -A2Length of output: 1003
All enum schema references have been updated consistently
Ran a global search across
.resand.hbsfiles and confirmed:
- No remaining occurrences of
Enums.*.enum.schema- All schema references now use
Enums.*.config.schema(e.g.,TablesStatic.res)No further changes are needed.
codegenerator/cli/npm/envio/src/db/EntityHistory.res (1)
91-91: Approve direct property access pattern.The changes from
RowAction.enum.schematoRowAction.schemaandRowAction.enum.nametoRowAction.nameare consistent with the enum refactoring where properties are now accessed directly from the module.Also applies to: 231-231
codegenerator/cli/templates/dynamic/codegen/src/db/Enums.res.hbs (2)
14-14: Approve enum configuration refactoring.The replacement of
Enum.make(~name, ~variants)withInternal.makeEnumConfig(~name, ~variants)and the binding change fromenumtoconfigrepresents a systematic refactoring of the enum system. The new pattern appears more flexible and centralized.Also applies to: 32-32, 49-49
53-59:Details
✅ Verification successful
Verify allEnums transformation logic.
The
allEnumsarray now usesInternal.fromGenericEnumConfigto transform enum configurations. Ensure this transformation maintains the expected array structure and that consuming code has been updated accordingly.
🏁 Script executed:
#!/bin/bash # Description: Verify allEnums usage and Internal.fromGenericEnumConfig function # Expected: Code consuming allEnums should work with the new structure echo "Searching for allEnums usage..." rg "allEnums" -A 3 -B 1 echo "Checking Internal.fromGenericEnumConfig definition..." rg "fromGenericEnumConfig" -A 5Length of output: 12002
Transformation logic verified for allEnums
Internal.fromGenericEnumConfig is defined as an identity function, so the array structure remains unchanged. All consuming code and tests (Persistence, PgStorage, EntityHistory) have been updated to use it and pass the expected enumConfig shape. No further changes needed.
codegenerator/cli/templates/static/codegen/src/db/Migrations.res (1)
29-39:Details
❓ Verification inconclusive
Significant simplification of migration logic - verify completeness.
The migration logic has been drastically simplified to delegate to
Persistence.init. While this centralization improves maintainability, ensure that:
- All previous migration functionality is preserved in the persistence layer
- Error handling provides sufficient diagnostic information
- The
skipIsInitializedCheck=trueflag is intentional and documentedConsider adding more specific error logging to help diagnose migration failures:
let exitCode = try { await Config.codegenPersistence->Persistence.init(~skipIsInitializedCheck=true, ~reset) Success } catch { - | _ => Failure + | exn => + exn->ErrorHandling.make(~msg="Migration failed during persistence initialization")->ErrorHandling.log + Failure }
🏁 Script executed:
#!/bin/bash # Description: Verify that Persistence.init covers all migration functionality # Expected: Persistence module should handle schema creation, table setup, etc. echo "Checking Persistence.init implementation..." rg "let init" -A 20 --type=res | grep -E "(init|schema|table|migration)" echo "Verifying Config.codegenPersistence definition..." rg "codegenPersistence" -A 5 -B 2Length of output: 2411
🏁 Script executed:
#!/bin/bash # Locate Persistence module file(s) echo "Locating Persistence.res files..." fd -e res -t f | grep -i Persistence.res || echo "No Persistence.res found via fd" echo # Find module definitions named Persistence echo "Searching for 'module Persistence' in .res files..." rg -n "module Persistence" -g "*.res" || echo "No module Persistence definitions found" echo # Inspect the init function within Persistence files echo "Inspecting 'let init' implementation in Persistence files..." rg -n "let init" -A 10 -g "*Persistence.res" || echo "No init functions found in Persistence.res"Length of output: 1520
#!/bin/bash # Extract the full `init` implementation in Persistence.res for review echo "Retrieving Persistence.init from envio module..." rg -n "let init = async" -A 200 -g "codegenerator/cli/npm/envio/src/Persistence.res" || \ rg -n "let init =" -A 200 -g "codegenerator/cli/npm/envio/src/Persistence.res"
Verify Persistence.init covers all migration steps and enhances error diagnostics.
Before merging, please ensure that the new centralized
Persistence.initcall inMigrations.restruly implements every prior migration step (schema creation, table setup, upserts, version tracking) and that theskipIsInitializedCheck=trueflag is intentional and documented. Also consider improving failure diagnostics by logging the caught exception:• In
codegenerator/cli/templates/static/codegen/src/db/Migrations.res
- Confirm
Persistence.initincodegenerator/cli/npm/envio/src/Persistence.resreplicates the old manual SQL behavior (upserts, migrations, schema/version checks).- Document the rationale for
~skipIsInitializedCheck=true.- Enhance error handling:
let exitCode = try { await Config.codegenPersistence->Persistence.init(~skipIsInitializedCheck=true, ~reset) Success } catch { - | _ => Failure + | exn => + exn + ->ErrorHandling.make(~msg="Migration failed during persistence init") + ->ErrorHandling.log; + Failure }• Verify that any upstream code relying on detailed migration errors isn’t affected and update docs/tests if needed.
scenarios/test_codegen/test/lib_tests/PgStorage_test.res (4)
4-38: Well-structured tests for index creation SQL.The test suite properly covers both single-field and composite index creation scenarios with appropriate assertions.
40-66: Good edge case coverage for table indices.The tests appropriately handle both entities with indices and those without, ensuring correct SQL generation in both cases.
89-101: Duplicate test case needs clarification.This test case titled "Should handle default values" generates identical SQL to the first test case for entity A. If this is meant to test default value handling, it should use an entity that actually has fields with default values other than the standard
db_write_timestamp.
104-285: Comprehensive test coverage for schema initialization.Excellent test suite that covers all major initialization scenarios including clean runs, conditional initialization, minimal configurations, and single entity setups. The expected SQL outputs are well-formatted and accurate.
scenarios/test_codegen/test/lib_tests/Persistence_test.res (4)
3-52: Excellent mock implementation for async storage interface.The mock design allows precise control over async behavior and comprehensive tracking of method calls, making it ideal for testing the persistence layer's initialization logic.
54-174: Thorough test of persistence initialization lifecycle.This test comprehensively covers the initialization flow, including default entity inclusion, status transitions, idempotency, and reset behavior. The manual promise resolution approach effectively tests the async state machine.
176-210: Good coverage of the already-initialized scenario.The test properly verifies that initialization is skipped when storage reports it's already initialized, and handles concurrent init calls correctly.
212-244: Important backward compatibility test.This test ensures that the
skipIsInitializedCheckflag maintains backward compatibility for the "envio local db-up" command by bypassing the initialization check.codegenerator/cli/npm/envio/src/Persistence.res (2)
8-36: Well-designed persistence abstraction types.The storage interface is minimal and focused, the status type properly models async states, and the main type encapsulates all necessary configuration. Good use of mutable field for status tracking.
38-66: Well-implemented factory function.The
makefunction properly combines user-provided entities and enums with internal ones, and correctly initializes all fields including the optional callback.
| let variants = [SET, DELETE] | ||
| let name = "ENTITY_HISTORY_ROW_ACTION" | ||
| let enum = Enum.make(~name, ~variants) | ||
| let schema = S.enum(variants) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify RowAction module consistency with new enum pattern.
The direct S.enum(variants) usage suggests the RowAction module now defines its schema directly rather than through an enum object. Ensure this aligns with how RowAction is defined elsewhere in the codebase.
🏁 Script executed:
#!/bin/bash
# Description: Verify RowAction module definition and usage consistency
# Expected: RowAction should be defined using Internal.makeEnumConfig pattern
echo "Searching for RowAction module definition..."
rg "module RowAction" -A 10
echo "Checking if RowAction uses the new enum config pattern..."
rg "RowAction.*Internal\.makeEnumConfig" -A 3Length of output: 1129
Ensure RowAction uses the new enum‐config pattern
The RowAction module still calls
let schema = S.enum(variants)but should be defined via Internal.makeEnumConfig in line with other enums.
Locations to update:
- File:
codegenerator/cli/npm/envio/src/db/EntityHistory.res- In the
module RowAction = { … }block, replacewith something like:let schema = S.enum(variants)
(or the pattern your codebase uses for enum configuration)let config = Internal.makeEnumConfig({ name, variants }) let schema = S.config(config)
- In the
🤖 Prompt for AI Agents
In codegenerator/cli/npm/envio/src/db/EntityHistory.res at line 7 within the
module RowAction, replace the current enum schema definition using
S.enum(variants) with the new pattern that uses Internal.makeEnumConfig.
Specifically, create a config using Internal.makeEnumConfig with the name and
variants, then define the schema as S.config(config) to align with the updated
enum configuration approach used elsewhere in the codebase.
| ~entities=persistence.allEntities, | ||
| ~staticTables=persistence.staticTables, | ||
| ~enums=persistence.allEnums, | ||
| ~cleanRun=reset || !skipIsInitializedCheck, |
There was a problem hiding this comment.
This is a little bit of a mess, but I hope we'll get rid of the upserting behaviour at some point, and it'll get simpler.
JonoPrest
left a comment
JonoPrest
left a comment
There was a problem hiding this comment.
Awesome, very happy about the decoupling 🙏🏼
2 participants
And add DI support
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests
Chores
Revert