Refactor load layer to use storage as the main interface for reading data - #631
Conversation
WalkthroughThis change refactors the loading and persistence infrastructure, introducing explicit separation between load manager and storage interfaces, and enhances effect caching support. It updates type definitions, restructures function signatures, removes indirection layers, and standardizes naming conventions for SQL query generation. Test and mock infrastructure are also updated to match the new abstractions. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant EventProcessing
participant LoadManager
participant Storage
User->>EventProcessing: processEventBatch(...)
EventProcessing->>LoadManager: runBatchHandlersOrThrow(~loadManager, ~storage, ...)
LoadManager->>Storage: loadByIdsOrThrow / loadByFieldOrThrow(...)
Storage-->>LoadManager: Promise<entities>
LoadManager-->>EventProcessing: entities
EventProcessing-->>User: Processing complete
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
🧰 Additional context used📓 Path-based instructions (2)`**/*.res`: Never use `[| item |]` to create an array. Use `[ item ]` instead. M...
📄 Source: CodeRabbit Inference Engine (.cursor/rules/rescript.mdc) List of files the instruction was applied to:
`**/*.{res,resi}`: ReScript has record types which require a type definition bef...
📄 Source: CodeRabbit Inference Engine (.cursor/rules/rescript.mdc) List of files the instruction was applied to:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (3)
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 (
|
| type t = { | ||
| loadManager: LoadManager.t, | ||
| loadEntitiesByIds: ( | ||
| array<Types.id>, | ||
| ~entityConfig: Internal.entityConfig, | ||
| ) => promise<array<Internal.entity>>, | ||
| loadEntitiesByField: ( | ||
| ~operator: TableIndices.Operator.t, | ||
| ~entityConfig: Internal.entityConfig, | ||
| ~fieldName: string, | ||
| ~fieldValue: fieldValue, | ||
| ~fieldValueSchema: S.t<fieldValue>, | ||
| ~logger: Pino.t=?, | ||
| ) => promise<array<Internal.entity>>, | ||
| } | ||
|
|
||
| let make = (~loadEntitiesByIds, ~loadEntitiesByField) => { | ||
| { | ||
| loadManager: LoadManager.make(), | ||
| loadEntitiesByIds, | ||
| loadEntitiesByField, | ||
| } | ||
| } | ||
|
|
||
| // Ideally it shouldn't be here, but it'll make writing tests easier, | ||
| // until we have a proper mocking solution. | ||
| let makeWithDbConnection = (~persistence=Config.codegenPersistence) => { | ||
| let storage = Persistence.getInitializedStorageOrThrow(persistence) | ||
| { | ||
| loadManager: LoadManager.make(), | ||
| loadEntitiesByIds: (ids, ~entityConfig) => | ||
| storage.loadByIdsOrThrow( | ||
| ~table=entityConfig.table, | ||
| ~rowsSchema=entityConfig.rowsSchema, | ||
| ~ids, | ||
| ), | ||
| loadEntitiesByField: DbFunctionsEntities.makeWhereQuery(Db.sql), | ||
| } | ||
| } |
There was a problem hiding this comment.
This was bad and it was removed.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
codegenerator/cli/templates/dynamic/codegen/src/TestHelpers_MockDb.res.hbs (2)
417-465: Add TODO comments for unimplemented storage methods.Several methods throw "Not used yet" errors without explanation. Consider adding TODO comments or implementing these methods to avoid confusion during testing.
and makeMockStorage = (mockDb: t): Persistence.storage => { { - isInitialized: () => Js.Exn.raiseError("Not used yet"), + // TODO: Implement when needed for testing initialization state + isInitialized: () => Js.Exn.raiseError("Not implemented yet"), - initialize: (~entities as _=?, ~generalTables as _=?, ~enums as _=?) => - Js.Exn.raiseError("Not used yet"), + // TODO: Implement when needed for testing initialization flow + initialize: (~entities as _=?, ~generalTables as _=?, ~enums as _=?) => + Js.Exn.raiseError("Not implemented yet"), - loadEffectCaches: () => Js.Exn.raiseError("Not used yet"), + // TODO: Implement when effect caching is fully functional + loadEffectCaches: () => Js.Exn.raiseError("Not implemented yet"),
441-462: Consider performance implications of index evaluation.The current implementation iterates through all entities for field-based queries. While acceptable for testing, consider adding a comment about the O(n) performance characteristic.
For larger test datasets, you might want to consider maintaining indexes for frequently queried fields to improve lookup performance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
codegenerator/cli/npm/envio/index.d.ts(1 hunks)codegenerator/cli/npm/envio/src/Envio.gen.ts(1 hunks)codegenerator/cli/npm/envio/src/Envio.res(2 hunks)codegenerator/cli/npm/envio/src/Internal.res(1 hunks)codegenerator/cli/npm/envio/src/Persistence.res(4 hunks)codegenerator/cli/npm/envio/src/PgStorage.res(17 hunks)codegenerator/cli/templates/dynamic/codegen/src/TestHelpers_MockDb.res.hbs(7 hunks)codegenerator/cli/templates/static/codegen/src/EventProcessing.res(11 hunks)codegenerator/cli/templates/static/codegen/src/IO.res(3 hunks)codegenerator/cli/templates/static/codegen/src/InMemoryStore.res(5 hunks)codegenerator/cli/templates/static/codegen/src/Index.res(3 hunks)codegenerator/cli/templates/static/codegen/src/LoadLayer.res(9 hunks)codegenerator/cli/templates/static/codegen/src/LoadLayer.resi(3 hunks)codegenerator/cli/templates/static/codegen/src/UserContext.res(11 hunks)codegenerator/cli/templates/static/codegen/src/db/DbFunctionsEntities.res(1 hunks)codegenerator/cli/templates/static/codegen/src/db/DbFunctionsImplementation.js(0 hunks)codegenerator/cli/templates/static/codegen/src/db/TablesStatic.res(1 hunks)codegenerator/cli/templates/static/codegen/src/globalState/GlobalState.res(4 hunks)scenarios/test_codegen/test/E2EEthNode_test.res(1 hunks)scenarios/test_codegen/test/Integration_ts_helpers.res(0 hunks)scenarios/test_codegen/test/LoadLayer_test.res(21 hunks)scenarios/test_codegen/test/Mock_test.res(1 hunks)scenarios/test_codegen/test/SerDe_Test.res(1 hunks)scenarios/test_codegen/test/helpers/Mock.res(1 hunks)scenarios/test_codegen/test/lib_tests/EntityHistory_test.res(2 hunks)scenarios/test_codegen/test/lib_tests/Persistence_test.res(4 hunks)scenarios/test_codegen/test/lib_tests/PgStorage_test.res(6 hunks)scenarios/test_codegen/test/rollback/Rollback_test.res(2 hunks)scenarios/test_codegen/test/schema_types/BigDecimal_test.res(2 hunks)scenarios/test_codegen/test/schema_types/Timestamp_test.res(2 hunks)
💤 Files with no reviewable changes (2)
- scenarios/test_codegen/test/Integration_ts_helpers.res
- codegenerator/cli/templates/static/codegen/src/db/DbFunctionsImplementation.js
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.res`: Never use `[| item |]` to create an array. Use `[ item ]` instead. M...
**/*.res: Never use[| item |]to create an array. Use[ item ]instead.
Must always use=for setting value to a field. Use:=only for ref values created usingreffunction.
It's also possible to define an inline object, it'll have quoted fields in this case.
Never use %raw to access object fields if you know the type.
📄 Source: CodeRabbit Inference Engine (.cursor/rules/rescript.mdc)
List of files the instruction was applied to:
scenarios/test_codegen/test/rollback/Rollback_test.rescodegenerator/cli/templates/static/codegen/src/db/TablesStatic.resscenarios/test_codegen/test/SerDe_Test.resscenarios/test_codegen/test/schema_types/BigDecimal_test.resscenarios/test_codegen/test/schema_types/Timestamp_test.resscenarios/test_codegen/test/Mock_test.resscenarios/test_codegen/test/lib_tests/EntityHistory_test.rescodegenerator/cli/templates/static/codegen/src/IO.rescodegenerator/cli/npm/envio/src/Internal.rescodegenerator/cli/npm/envio/src/Envio.resscenarios/test_codegen/test/E2EEthNode_test.rescodegenerator/cli/templates/static/codegen/src/db/DbFunctionsEntities.resscenarios/test_codegen/test/lib_tests/PgStorage_test.resscenarios/test_codegen/test/lib_tests/Persistence_test.rescodegenerator/cli/templates/static/codegen/src/Index.rescodegenerator/cli/templates/static/codegen/src/globalState/GlobalState.resscenarios/test_codegen/test/LoadLayer_test.rescodegenerator/cli/templates/static/codegen/src/UserContext.rescodegenerator/cli/templates/static/codegen/src/InMemoryStore.rescodegenerator/cli/templates/static/codegen/src/EventProcessing.resscenarios/test_codegen/test/helpers/Mock.rescodegenerator/cli/templates/static/codegen/src/LoadLayer.rescodegenerator/cli/npm/envio/src/Persistence.rescodegenerator/cli/npm/envio/src/PgStorage.res
`**/*.{res,resi}`: ReScript has record types which require a type definition bef...
**/*.{res,resi}: ReScript has record types which require a type definition beforehand. You can access record fields by dot likefoo.myField.
Use records when working with structured data, and objects to conveniently pass payload data between functions.
📄 Source: CodeRabbit Inference Engine (.cursor/rules/rescript.mdc)
List of files the instruction was applied to:
scenarios/test_codegen/test/rollback/Rollback_test.rescodegenerator/cli/templates/static/codegen/src/db/TablesStatic.resscenarios/test_codegen/test/SerDe_Test.resscenarios/test_codegen/test/schema_types/BigDecimal_test.resscenarios/test_codegen/test/schema_types/Timestamp_test.resscenarios/test_codegen/test/Mock_test.resscenarios/test_codegen/test/lib_tests/EntityHistory_test.rescodegenerator/cli/templates/static/codegen/src/IO.rescodegenerator/cli/npm/envio/src/Internal.rescodegenerator/cli/npm/envio/src/Envio.resscenarios/test_codegen/test/E2EEthNode_test.rescodegenerator/cli/templates/static/codegen/src/db/DbFunctionsEntities.resscenarios/test_codegen/test/lib_tests/PgStorage_test.resscenarios/test_codegen/test/lib_tests/Persistence_test.rescodegenerator/cli/templates/static/codegen/src/Index.rescodegenerator/cli/templates/static/codegen/src/globalState/GlobalState.resscenarios/test_codegen/test/LoadLayer_test.rescodegenerator/cli/templates/static/codegen/src/UserContext.rescodegenerator/cli/templates/static/codegen/src/InMemoryStore.rescodegenerator/cli/templates/static/codegen/src/EventProcessing.resscenarios/test_codegen/test/helpers/Mock.rescodegenerator/cli/templates/static/codegen/src/LoadLayer.rescodegenerator/cli/npm/envio/src/Persistence.rescodegenerator/cli/templates/static/codegen/src/LoadLayer.resicodegenerator/cli/npm/envio/src/PgStorage.res
🧠 Learnings (10)
scenarios/test_codegen/test/SerDe_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.878Z
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.
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.878Z
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.
codegenerator/cli/templates/static/codegen/src/IO.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.878Z
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.
codegenerator/cli/templates/static/codegen/src/db/DbFunctionsEntities.res (2)
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.699Z
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.699Z
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.
scenarios/test_codegen/test/lib_tests/PgStorage_test.res (3)
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.699Z
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.699Z
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:631-659
Timestamp: 2025-05-27T17:07:12.878Z
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.
scenarios/test_codegen/test/lib_tests/Persistence_test.res (3)
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.878Z
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.
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.699Z
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.699Z
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.
scenarios/test_codegen/test/LoadLayer_test.res (3)
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.878Z
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.
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.699Z
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.699Z
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.
codegenerator/cli/templates/static/codegen/src/InMemoryStore.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.878Z
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.
codegenerator/cli/templates/dynamic/codegen/src/TestHelpers_MockDb.res.hbs (3)
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.878Z
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.
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.699Z
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.699Z
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.
codegenerator/cli/npm/envio/src/PgStorage.res (2)
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.699Z
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.699Z
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.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_and_test
🔇 Additional comments (61)
codegenerator/cli/templates/static/codegen/src/db/TablesStatic.res (1)
247-247: LGTM! Clean type casting implementation.The external function using
%identityprovides a type-safe, zero-cost conversion fromDynamicContractRegistry.ttoInternal.entity, which aligns well with the refactoring to centralize data handling through the Storage abstraction.codegenerator/cli/npm/envio/src/Internal.res (1)
200-200: LGTM! Clean addition for effect caching support.The
cache: boolfield is a straightforward addition that supports the new effect caching functionality. The field placement and typing follow ReScript conventions correctly.scenarios/test_codegen/test/E2EEthNode_test.res (1)
73-73: LGTM! Simplified test setup aligns with refactoring goals.The removal of
loadLayercreation and the simplifiedGlobalState.makecall effectively moves complexity from test setup into the library, which improves testability and aligns with the PR's goal of centralizing data operations through the Storage abstraction.codegenerator/cli/npm/envio/index.d.ts (1)
108-109: LGTM! Well-documented optional property for effect caching.The addition of the optional
cache?: booleanproperty is properly typed and documented. The interface extension cleanly supports the new effect caching functionality while maintaining backward compatibility.codegenerator/cli/npm/envio/src/Envio.gen.ts (1)
24-26: LGTM! Generated TypeScript properly reflects ReScript implementation.The addition of the optional
cache?: booleanproperty to theeffectOptionstype is correctly generated and properly documented. The generated TypeScript maintains consistency with the manual TypeScript declarations and the underlying ReScript implementation.scenarios/test_codegen/test/SerDe_Test.res (2)
118-118: LGTM: Function name updated for consistency.The function name change from
makeCreateTableSqltomakeCreateTableQueryaligns with the broader refactoring to standardize SQL generation function naming conventions.
123-123: LGTM: Function name updated for consistency.The function name change from
makeInsertUnnestSetSqltomakeInsertUnnestSetQuerymaintains consistency with the updated naming convention for SQL generation functions.scenarios/test_codegen/test/rollback/Rollback_test.res (2)
144-144: LGTM: Removed loadLayer parameter as part of refactoring.The removal of the
~loadLayerparameter fromGlobalState.make()aligns with the refactoring to eliminate theloadLayerabstraction in favor of explicitloadManagerandstorageparameters. This change is consistent with the broader architectural improvements.
210-210: LGTM: Consistent removal of loadLayer parameter.The removal of the
~loadLayerparameter maintains consistency with the refactoring changes throughout the codebase.scenarios/test_codegen/test/Mock_test.res (2)
11-12: LGTM: Updated to use explicit loadManager and storage.The replacement of
LoadLayer.makeWithDbConnection()with explicitLoadManager.make()andConfig.codegenPersistence.storagealigns with the refactoring to remove theloadLayerabstraction and use separateloadManagerandstoragecomponents.
17-18: LGTM: Function call updated to use new parameters.The update to pass
~loadManagerand~storageinstead of~loadLayeris consistent with the refactored function signature and the broader architectural changes.scenarios/test_codegen/test/schema_types/BigDecimal_test.res (3)
38-39: LGTM: Consistent refactoring to explicit components.The replacement of
LoadLayer.makeWithDbConnection()withLoadManager.make()andConfig.codegenPersistence.storagefollows the same pattern as other test files and aligns with the architectural refactoring.
45-46: LGTM: Updated loader context parameters.The update to pass
~loadManagerand~storagetoUserContext.getLoaderContextis consistent with the refactored function signature.
58-59: LGTM: Updated handler context parameters.The update to pass
~loadManagerand~storagetoUserContext.getHandlerContextmaintains consistency with the refactored interfaces.scenarios/test_codegen/test/lib_tests/EntityHistory_test.res (2)
6-7: LGTM: Improved comment clarity.The updated comment provides better clarity about the purpose of mandatory tables in Envio-managed schemas and specifically highlights the role of
event_sync_statein identifying Envio-controlled schemas.
230-230: LGTM: Function name updated for consistency.The function name change from
makeCreateTableSqltomakeCreateTableQuerymaintains consistency with the updated naming convention for SQL generation functions across the codebase.codegenerator/cli/npm/envio/src/Envio.res (2)
25-26: LGTM: Effect caching support added correctly.The optional
cachefield follows proper ReScript conventions for optional record fields.
53-53: LGTM: Safe handling of optional cache field.The use of
Belt.Option.getWithDefault(false)correctly handles the optional cache field, providing a sensible default value when not specified.scenarios/test_codegen/test/schema_types/Timestamp_test.res (2)
30-31: LGTM: Refactoring aligns with architectural changes.The transition from
LoadLayer.makeWithDbConnection()to separateLoadManager.make()andstoragevariables is consistent with the PR's objective to use storage as the main interface.
37-38: LGTM: Context construction updated consistently.Both loader and handler contexts are updated to use the new
loadManagerandstorageparameters, maintaining consistency across the codebase.Also applies to: 49-50
codegenerator/cli/templates/static/codegen/src/IO.res (3)
57-58: LGTM: Improved abstraction with accessor function.The change from direct dictionary access to using
getInMemTableaccessor function improves abstraction and type safety for in-memory entity table access.
137-138: LGTM: Consistent use of accessor function.The refactoring to use
getInMemTableis applied consistently across functions that access in-memory tables.
276-276: LGTM: Rollback function updated consistently.The rollback functionality also uses the new accessor pattern, maintaining consistency with the refactoring.
scenarios/test_codegen/test/lib_tests/PgStorage_test.res (2)
4-4: LGTM: Consistent naming convention update.The renaming from
*Sqlto*Queryis applied consistently across all test functions and variables, aligning with the implementation changes in PgStorage.res.Also applies to: 8-8, 15-15, 25-25, 32-32, 40-40, 44-44, 47-47, 54-54, 57-57, 62-62, 66-66, 69-69, 76-76, 79-79, 86-86, 90-90
239-239: LGTM: Comprehensive test suite naming update.The extensive renaming maintains consistency with the new naming convention while preserving all test logic and expected SQL strings.
Also applies to: 243-243, 246-246, 256-256, 259-259, 267-267, 271-271, 274-274, 284-284, 287-287, 295-295, 299-299, 306-306, 309-309, 316-316, 323-323, 327-327, 328-328, 335-335, 339-339, 346-346, 350-350, 351-351, 361-361, 368-368, 372-372, 373-373
codegenerator/cli/templates/static/codegen/src/db/DbFunctionsEntities.res (1)
22-22: LGTM: Query-by-condition logic removed as part of architectural refactor.The removal of query-by-condition functions aligns with the broader refactoring to use storage as the main interface for data loading operations. The remaining
batchDeletefunctionality is appropriately preserved.codegenerator/cli/templates/static/codegen/src/globalState/GlobalState.res (4)
60-60: LGTM! Field rename aligns with LoadLayer abstraction removal.The field rename from
loadLayertoloadManageris consistent with the architectural refactoring described in the PR objectives.
67-67: LGTM! Function signature simplified appropriately.Removing the
loadLayerparameter aligns with the internal initialization approach.
87-87: LGTM! Internal initialization follows the new pattern.The internal initialization of
loadManagerviaLoadManager.make()is consistent with the refactoring to remove theLoadLayerabstraction.
961-961: LGTM! Parameter update aligns with the refactoring.Passing
loadManagertoEventProcessing.processEventBatchis consistent with the removal of theLoadLayerabstraction.codegenerator/cli/templates/static/codegen/src/Index.res (3)
167-167: LGTM! Type definition simplified appropriately.The simplified
argstype definition improves readability and maintainability.
181-184: LGTM! Code formatting improved.The improved formatting of the
Pervasives.maxcall enhances readability.
351-351: LGTM! Function call updated correctly.The simplified
GlobalState.makecall aligns with the removedloadLayerparameter in the function signature.scenarios/test_codegen/test/lib_tests/Persistence_test.res (4)
5-5: LGTM! Mock storage approach improved.The new
Mock.Storage.makewith explicit capabilities provides better test clarity and maintainability compared to the previous approach.
88-88: LGTM! Test assertion updated for new persistence interface.The addition of
effectCachesin theReadystate assertion correctly reflects the expanded persistence layer interface.
124-124: LGTM! Mock capabilities updated for new storage interface.Adding
#loadEffectCachescapability correctly reflects the new storage interface method for loading effect cache metadata.
151-159: LGTM! Test assertions verify new effect cache loading behavior.The updated assertions correctly validate that
loadEffectCachesis called exactly once when storage is already initialized, which aligns with the new persistence layer behavior.codegenerator/cli/templates/static/codegen/src/InMemoryStore.res (5)
21-22: LGTM! Interface improved with string-based entity names.The change from
~entityConfigto~entityName: stringimproves the interface abstraction by removing the need to pass entire config objects.
31-31: LGTM! Error message updated consistently.The error message update to use
entityNameis consistent with the function signature change and maintains clear error reporting.
87-87: LGTM! Function adapted to new interface correctly.The update to pass
~entityName=entityConfig.namecorrectly adapts to the new string-based interface while maintaining functionality.
98-98: LGTM! Improved abstraction with getInMemTable.Using
getInMemTableinstead of directEntityTables.getimproves abstraction and follows the established pattern.
128-131: LGTM! Explicit entity casting ensures type safety.The addition of
TablesStatic.DynamicContractRegistry.castToInternalcasting ensures entities are stored in the correct internal form, improving type safety.scenarios/test_codegen/test/LoadLayer_test.res (4)
5-7: LGTM! Mock setup improved with explicit capabilities.The new
Mock.Storage.makeapproach with explicit capabilities (#loadByIdsOrThrow) and separateloadManagerinstantiation provides better test clarity and aligns with the new architecture.
10-18: LGTM! Function call updated to use explicit parameters.The updated
LoadLayer.loadByIdcall with explicit~loadManagerand~storageparameters correctly implements the new architecture while maintaining the same functionality.
24-31: LGTM! Assertion updated for new storage interface.The updated assertion using
storageMock.loadByIdsOrThrowCallscorrectly validates the new storage interface calls with proper field validation (ids,tableName).
348-372: LGTM! LoadLayer.loadByField calls updated consistently.The updated
LoadLayer.loadByFieldcalls with explicit~loadManagerand~storageparameters follow the same pattern as theloadByIdupdates and correctly implement the new architecture.codegenerator/cli/templates/static/codegen/src/UserContext.res (1)
17-24: LGTM! Clean separation of concerns.The refactoring from a single
loadLayerto explicitloadManagerandstorageparameters improves modularity and makes the dependencies more explicit.codegenerator/cli/templates/static/codegen/src/EventProcessing.res (1)
300-301: Good approach for obtaining storage instance.Getting storage from
config.persistencemaintains backward compatibility while supporting the new architecture.codegenerator/cli/templates/dynamic/codegen/src/TestHelpers_MockDb.res.hbs (1)
188-195: LGTM! Improved abstraction with string-based entity lookups.Using entity names as strings instead of entity configs provides better flexibility and decoupling.
codegenerator/cli/npm/envio/src/Persistence.res (2)
125-149: Well-structured initialization with proper race condition handling.The effect cache loading logic correctly handles initialization states and race conditions, ensuring caches are loaded only once during non-clean runs.
9-17: Dismiss thread-safety warning: single‐threaded, sequential accessIn this CLI/Node context all
effectCacheinstances are created and consumed through a single‐threaded event loop (the only async call is awaited before use), so there’s no concurrent mutation at runtime. You can safely leave the mutable fields as is.Likely an incorrect or invalid review comment.
scenarios/test_codegen/test/helpers/Mock.res (1)
36-80: Excellent mock design with selective method implementation.The new Storage mock provides comprehensive call tracking and selective method implementation, which greatly improves testability.
codegenerator/cli/templates/static/codegen/src/LoadLayer.res (3)
3-55: LGTM! Clean refactoring to explicit dependencies.The function correctly replaces the
loadLayerabstraction with explicitloadManagerandstorageparameters, maintaining all functionality including error handling.
57-89: LGTM! Straightforward parameter update.The function correctly accepts the explicit
loadManagerparameter. No storage parameter is needed since this function handles effects without database interaction.
91-177: Well-executed refactoring with improved error context.The function correctly implements the new explicit parameter pattern. The operator string conversion for cache keys improves readability, and the enhanced error logging with additional context (operator, table, field, value) will aid in debugging.
codegenerator/cli/templates/static/codegen/src/LoadLayer.resi (1)
1-31: Interface correctly updated to match implementation.The removal of the abstract
ttype and factory functions, along with the updated function signatures to accept explicitloadManagerandstorageparameters, properly reflects the simplified architecture.codegenerator/cli/npm/envio/src/PgStorage.res (5)
1-5: Excellent naming improvements for clarity.The consistent renaming from
*Sqlto*Querybetter reflects that these functions generate SQL query strings. The cache object key update from "sql" to "query" maintains this consistency.Also applies to: 7-13, 23-23, 55-168, 236-252, 311-321
344-351: Clean helper for schema introspection.Good addition of the typed
schemaTableNameand corresponding query function for safe schema introspection.
389-408: Effect cache discovery looks good.The function correctly identifies effect cache tables by prefix. Since this returns caches with
size: 0andtable: None, I assume this is part of the "preliminary traces of effect caching" mentioned in the PR description?Are there plans to populate the size and table fields in a follow-up PR?
444-489: Robust implementation of field-based loading.Excellent error handling with descriptive messages for both serialization and query execution failures. The operator type conversion is clean and type-safe.
362-380: Excellent safety improvements in initialization.The enhanced validation preventing accidental data loss is a great addition. The error message is particularly well-crafted with clear, actionable resolution steps.
2 participants
The goal of the PR is to simplify the development and testing of the persistence layer. It moves more code from codegen to the library by starting to fully utilize the newly created Storage abstraction (as an injectable interface)
There are also some traces of effect caching, but it's not usable yet.
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests