Add Entity.getOrCreate and Entity.getOrThrow API#567
Conversation
WalkthroughThis change introduces new entity retrieval methods— Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Handler as EntityHandlerContext
participant Store as EntityStore
Caller->>Handler: getOrThrow(id)
Handler->>Store: get(id)
alt Entity exists
Store-->>Handler: entity
Handler-->>Caller: entity
else Entity missing
Handler-->>Caller: throws error
end
Caller->>Handler: getOrCreate(entity)
Handler->>Store: get(entity.id)
alt Entity exists
Store-->>Handler: entity
Handler-->>Caller: entity
else Entity missing
Handler->>Store: set(entity)
Store-->>Handler: entity
Handler-->>Caller: entity
end
Suggested reviewers
Poem
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: 1
🧹 Nitpick comments (2)
codegenerator/cli/templates/static/codegen/src/UserContext.res (2)
111-121: Consider potential race condition in getOrCreate.The implementation is functionally correct, but there's a theoretical race condition between checking entity existence and creating it. In concurrent scenarios, multiple calls could attempt to create the same entity simultaneously.
This might be acceptable depending on your system's concurrency model and whether the underlying storage layer handles duplicate creation gracefully. Consider documenting this behavior or implementing atomic upsert semantics if needed.
215-236: Consider reducing code duplication between contexts.The
getOrThrowimplementation is duplicated between handler and loader contexts. While the logic is correct, you could extract this into a shared helper function to improve maintainability.+let makeGetOrThrow = (get, entityName) => (entityId, ~message=?) => + get(entityId)->Promise.thenResolve(entity => { + switch entity { + | Some(entity) => entity + | None => + Js.Exn.raiseError( + message->Belt.Option.getWithDefault( + `Entity '${entityName}' with ID '${entityId}' is expected to exist.`, + ), + ) + } + })Then use this helper in both contexts:
- getOrThrow: (entityId, ~message=?) => - get(entityId)->Promise.thenResolve(entity => { - switch entity { - | Some(entity) => entity - | None => - Js.Exn.raiseError( - message->Belt.Option.getWithDefault( - `Entity '${entityConfig.name}' with ID '${entityId}' is expected to exist.`, - ), - ) - } - }), + getOrThrow: makeGetOrThrow(get, entityConfig.name),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
codegenerator/cli/templates/dynamic/codegen/src/Types.res.hbs(2 hunks)codegenerator/cli/templates/dynamic/codegen/src/Types.ts.hbs(2 hunks)codegenerator/cli/templates/static/codegen/src/UserContext.res(3 hunks)scenarios/test_codegen/src/EventHandlers.ts(2 hunks)scenarios/test_codegen/test/EventHandler_test.ts(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
scenarios/test_codegen/test/EventHandler_test.ts
[error] 104-104: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.
(lint/suspicious/noFocusedTests)
[error] 131-131: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.
(lint/suspicious/noFocusedTests)
[error] 159-159: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.
(lint/suspicious/noFocusedTests)
[error] 187-187: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.
(lint/suspicious/noFocusedTests)
[error] 209-209: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.
(lint/suspicious/noFocusedTests)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build_and_test
🔇 Additional comments (8)
codegenerator/cli/templates/dynamic/codegen/src/Types.ts.hbs (1)
26-30: LGTM! Clean and well-designed API additions.The new
getOrThrowandgetOrCreatemethods are properly documented with clear JSDoc comments and follow consistent TypeScript patterns. The design decision to restrictgetOrCreatetoHandlerContextonly makes sense since loaders should not perform mutations.Also applies to: 54-63
codegenerator/cli/templates/dynamic/codegen/src/Types.res.hbs (1)
18-18: LGTM! Consistent type definitions across language bindings.The ReScript type definitions properly mirror the TypeScript interface with correct syntax for labeled arguments (
~message: string=?). The API consistency between TypeScript and ReScript ensures proper interoperability.Also applies to: 34-35
scenarios/test_codegen/test/EventHandler_test.ts (1)
104-229: Excellent test coverage for new entity methods.The test suite comprehensively covers all scenarios for the new
getOrCreateandgetOrThrowmethods:
- Entity creation when missing
- Entity loading when existing
- Successful retrieval with
getOrThrow- Error handling with default and custom messages
The test setup with mock database and entity assertions using
deepEqualis well-structured and validates the expected behavior correctly.🧰 Tools
🪛 Biome (1.9.4)
[error] 104-104: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.(lint/suspicious/noFocusedTests)
[error] 131-131: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.(lint/suspicious/noFocusedTests)
[error] 159-159: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.(lint/suspicious/noFocusedTests)
[error] 187-187: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.(lint/suspicious/noFocusedTests)
[error] 209-209: Don't focus the test.
The 'only' method is often used for debugging or during implementation. It should be removed before deploying to production.
Consider removing 'only' to ensure all tests are executed.
Unsafe fix: Remove focus from test.(lint/suspicious/noFocusedTests)
scenarios/test_codegen/src/EventHandlers.ts (3)
1-1: Good addition of required import.Adding
deepEqualimport supports the assertion logic used in the new event handler test cases.
431-438: Clean and focused helper functions.The
notifyandisLiveEventhelper functions are simple, well-named, and serve clear purposes in the event handling logic.
440-506: Excellent implementation testing the new entity methods.The event handler systematically tests all scenarios for the new
getOrCreateandgetOrThrowmethods:
- Proper type checking with
expectType- Data validation using
deepEqualassertions- Comprehensive coverage of success and error cases
- Clean switch statement structure that corresponds to test cases
The implementation demonstrates the intended usage patterns and validates the API behavior correctly.
codegenerator/cli/templates/static/codegen/src/UserContext.res (2)
67-85: LGTM: Good refactoring with helper functions.The extraction of
getandsethelper functions improves code organization and reduces duplication. The logic is correct and maintains the same functionality as before.
99-110: LGTM: getOrThrow implementation is correct.The
getOrThrowmethod properly handles the error case with a customizable error message and follows the expected semantics for this type of operation.
| | None => | ||
| Js.Exn.raiseError( | ||
| message->Belt.Option.getWithDefault( | ||
| `Entity '${params.entityConfig.name}' with ID '${entityId}' is expected to exist.`, |
There was a problem hiding this comment.
I think regardless of the message we should add entity name and id as a context.
There was a problem hiding this comment.
No easy way to pass it from here
| getOrThrow: (entityId, ~message=?) => | ||
| get(entityId)->Promise.thenResolve(entity => { | ||
| switch entity { | ||
| | Some(entity) => entity | ||
| | None => |
There was a problem hiding this comment.
How does this implementation account for the entity not existing on the first run of the loader?
There was a problem hiding this comment.
@JonoPrest I updated the PR to silently ignore the first loader run errors. I don't know why we didn't do it before. I firmly believe that the previous behavior is wrong, so I'll merge the PR without a rereview, but I'm open to feedback when you're back on Monday.
There was a problem hiding this comment.
Ok, something is wrong with thests, so you'll have a chance to review :)
JonoPrest
left a comment
JonoPrest
left a comment
There was a problem hiding this comment.
Looks great but please let me know if its handled that loader runs for the first time without the entity existing yet or if it will crash.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
codegenerator/cli/npm/envio/src/bindings/Promise.res (1)
40-43: Consider the implications of silent error suppression.The
silentCatchfunction suppresses promise rejections without any logging or handling, which could make debugging difficult if errors are inadvertently swallowed. While this appears intentional for the new batch processing flow, ensure this is only used where errors are definitively handled elsewhere.The use of
Obj.magicis inherently unsafe but appears necessary for the no-op function.Consider adding a comment explaining when
silentCatchshould be used:+// Only use silentCatch when errors are handled elsewhere in the pipeline let silentCatch = (promise: promise<'a>): promise<'a> => { catch(promise, noop) }scenarios/test_codegen/src/EventHandlers.ts (1)
431-431: Consider test isolation for the module-level counter.While this approach works for sequential test execution, consider resetting this counter before each test or using a more isolated approach to avoid potential issues with parallel test execution.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
codegenerator/cli/npm/envio/src/ErrorHandling.res(0 hunks)codegenerator/cli/npm/envio/src/bindings/Promise.res(1 hunks)codegenerator/cli/templates/dynamic/codegen/src/TestHelpers.res.hbs(4 hunks)codegenerator/cli/templates/static/codegen/src/EventProcessing.res(4 hunks)scenarios/test_codegen/src/EventHandlers.ts(2 hunks)scenarios/test_codegen/test/EventHandler_test.ts(2 hunks)scenarios/test_codegen/test/Mock_test.res(1 hunks)
💤 Files with no reviewable changes (1)
- codegenerator/cli/npm/envio/src/ErrorHandling.res
🧰 Additional context used
🧬 Code Graph Analysis (1)
scenarios/test_codegen/src/EventHandlers.ts (1)
codegenerator/cli/templates/static/codegen/index.js (1)
BigDecimal(7-7)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build_and_test
🔇 Additional comments (15)
scenarios/test_codegen/test/Mock_test.res (1)
13-26: Excellent refactor to batch processing with proper error handling.The consolidation from manual event iteration to
runBatchHandlersOrThrowaligns well with the new exception-based error handling pattern. The try-catch block properly handlesProcessingErrorexceptions while maintaining consistent error logging and propagation.codegenerator/cli/templates/dynamic/codegen/src/TestHelpers.res.hbs (2)
143-158: Well-designed refactor to exception-based batch processing.The transformation from conditional handler invocation to batch processing with exception handling significantly improves the code:
- Cleaner control flow: Exception-based error handling is more readable than nested conditionals
- Consistent error handling:
ProcessingErrorexceptions provide structured error information- Better performance: Batch processing is more efficient than individual event handling
- Improved maintainability: Unified error handling path reduces code duplication
The proper contextual logging with event-specific loggers ensures good debugging capabilities.
129-132:Details
✅ Verification successful
Verify the contract register function signature.
Ensure that
runContractRegistersOrThrowaccepts the expected parameter format with~reversedWithContractRegister.
🏁 Script executed:
#!/bin/bash # Verify the ChainFetcher.runContractRegistersOrThrow function signature ast-grep --pattern 'let runContractRegistersOrThrow = $$$'Length of output: 60
To locate the definition of
runContractRegistersOrThrow, let’s search across all Rescript source files:
🏁 Script executed:
#!/bin/bash # Search for runContractRegistersOrThrow in .res files to find its signature rg "runContractRegistersOrThrow" -n --glob "*.res"Length of output: 340
To confirm the exact parameters of
runContractRegistersOrThrow, let’s inspect its definition inChainFetcher.resaround line 311:
🏁 Script executed:
#!/bin/bash # Extract the function signature and surrounding context sed -n '300,330p' codegenerator/cli/templates/static/codegen/src/eventFetching/ChainFetcher.resLength of output: 1258
Signature Verified
The
runContractRegistersOrThrowfunction in
codegenerator/cli/templates/static/codegen/src/eventFetching/ChainFetcher.res
is declared as:let runContractRegistersOrThrow = async ( ~reversedWithContractRegister: array<Internal.eventItem>, ) => { … }This matches the call in your template (
~reversedWithContractRegister=[eventItem]). No changes required.scenarios/test_codegen/test/EventHandler_test.ts (6)
105-130: Excellent test coverage for getOrCreate functionality.The test properly verifies entity creation when the entity doesn't exist, with appropriate assertions on all expected fields including the default values for
updatesCountOnUserForTestingandaccountType.
132-158: Good isolation and verification of getOrCreate loading behavior.The test correctly:
- Pre-populates the mock database with an existing entity
- Verifies that
getOrCreateloads rather than creates- Ensures no modification of the existing entity
This validates the conditional logic in the
getOrCreateimplementation.
160-186: Solid verification of getOrThrow success path.The test appropriately validates that
getOrThrowreturns existing entities without modification, confirming the method works as expected when entities exist.
188-208: Proper error handling validation for getOrThrow.The test correctly uses
assert.rejectsto verify thatgetOrThrowthrows the expected error message when entities don't exist. This ensures the error handling semantics are working properly.
210-230: Good validation of custom error messaging.Testing custom error messages ensures the
getOrThrowmethod properly supports user-defined error messages, which is important for debugging and user experience.
232-252: Comprehensive edge case testing for loader failure scenarios.This test validates complex error handling where the first loader failure is ignored but subsequent failures abort processing. This ensures the robustness of the error handling pipeline in real-world scenarios.
scenarios/test_codegen/src/EventHandlers.ts (2)
1-1: LGTM! Good choice for deep equality assertions.Using
deepEqualfrom the assert module is appropriate for verifying entity data in tests.
433-566: Well-structured test implementation for the new entity retrieval methods.The test cases comprehensively cover the new
getOrCreateandgetOrThrowfunctionality:
- Loader failure handling is properly tested
- Type safety is verified with
expectType- Data integrity is checked with
deepEqual- All test scenarios are clearly separated and documented
codegenerator/cli/templates/static/codegen/src/EventProcessing.res (4)
95-95: Good error structure for event processing failures.The
ProcessingErrorexception provides excellent context for debugging by including the error message, original exception, and the event item that caused the failure.
97-166: Clean refactoring to exception-based error handling.The error handling provides clear, actionable messages that guide users to handle errors properly. The preservation of benchmarking logic and proper propagation of loader returns to handlers is well implemented.
196-227: Intentional error suppression is well-documented.The comments clearly explain why errors are silently caught during the first loader run. This design prevents stale data by re-running loaders before handlers, aligning with the test behavior in EventHandlers.ts.
275-349: Excellent simplification of error handling flow.The refactoring from result-based to exception-based error handling significantly improves readability while maintaining all functionality. The centralized error handling with proper context preservation makes debugging easier.
| try { | ||
| Some( | ||
| loader( | ||
| UserContext.getLoaderArgs({ | ||
| eventItem, | ||
| inMemoryStore, | ||
| loadLayer, | ||
| shouldGroup: true, | ||
| }), | ||
| )->Promise.silentCatch, | ||
| ) | ||
| } catch { |
There was a problem hiding this comment.
Do you need to try catch if you're running silent catch?
Or vice versa
There was a problem hiding this comment.
Yes, because if it throws before await, it'll be immediately thrown without creating a rejected promise.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
scenarios/erc20_multichain_factory/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (1)
codegenerator/cli/templates/static/codegen/src/EventProcessing.res(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build_and_test
🔇 Additional comments (5)
codegenerator/cli/templates/static/codegen/src/EventProcessing.res (5)
95-95: Well-structured exception type for debugging.The
ProcessingErrorexception includes comprehensive context (message, original exception, and event item) which will be valuable for error tracking and debugging.
97-166: Solid exception-based refactor with proper benchmarking.The refactored function correctly:
- Wraps async operations in try-catch blocks
- Provides descriptive error messages for debugging
- Preserves benchmarking logic after successful execution
- Maintains the same logical flow as the previous result-based approach
168-194: Consistent refactor maintaining original logic.The function correctly adapts to the new exception-based pattern while preserving all original functionality including sync state updates and raw event handling.
232-251: Appropriate sequential processing for handlers.Sequential handler execution ensures proper event ordering and allows exceptions to properly propagate, which is correct for the exception-based error handling pattern.
278-352:Details
❌ Incorrect review comment
Mixed error handling patterns need consistency.
The function mixes exception-based processing with result-based database operations, creating inconsistency:
- Lines 301-345: ProcessingError exceptions from event processing
- Lines 320-321: Database exceptions converted to Error results
- Lines 347-350: ProcessingError converted back to Error results
Consider standardizing the error handling approach - either handle database exceptions as ProcessingError or convert all exceptions to results at the same level.
Additionally, verify that all metrics and benchmarking data is properly preserved in both success and error scenarios.
No changes needed to error‐handling here
Both event‐processing errors and database exceptions are already converted into
Errorresults before the function returns—
- The
switch … | exception exn => …block aroundexecuteBatchcatches and wraps DB errors.- The outer
catch ProcessingErrorblock wraps any processing exceptions.Since every failure path yields an
Errorand every success path yieldsOk(), the error‐handling pattern is consistent.Likely an incorrect or invalid review comment.
| let runBatchLoadersOrThrow = async ( | ||
| eventBatch: array<Internal.eventItem>, | ||
| ~loadLayer, | ||
| ~inMemoryStore, | ||
| ) => { | ||
| // On the first run of loaders, we don't care about the result, | ||
| // whether it's an error or a return type. | ||
| // We'll rerun the loader again right before the handler run, | ||
| // to avoid having a stale data returned from the loader. | ||
| let _ = await Promise.all( | ||
| eventBatch->Array.keepMap(eventItem => { | ||
| switch eventItem.eventConfig { | ||
| | {loader: Some(loader)} => | ||
| try { | ||
| Some( | ||
| loader( | ||
| UserContext.getLoaderArgs({ | ||
| eventItem, | ||
| inMemoryStore, | ||
| loadLayer, | ||
| shouldGroup: true, | ||
| }), | ||
| // Must have Promise.catch as well as normal catch, | ||
| // because if user throws an error before await in the handler, | ||
| // it won't create a rejected promise | ||
| )->Promise.silentCatch, | ||
| ) | ||
| } catch { | ||
| | _ => None | ||
| } | ||
| }) | ||
| ->Promise.all | ||
| Ok() | ||
| }) | ||
| | _ => None | ||
| } | ||
| }), | ||
| ) | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the silent error suppression approach in batch loaders.
The dual error handling (try-catch + silentCatch) correctly addresses synchronous vs asynchronous errors. However, the silent suppression of all loader errors on the first run could mask important issues.
Consider logging suppressed errors for debugging purposes, even if they don't fail the batch processing.
🏁 Script executed:
#!/bin/bash
# Verify that Promise.silentCatch is properly implemented and doesn't completely hide errors
ast-grep --pattern 'silentCatch'Length of output: 32
I couldn’t locate any silentCatch definitions with the first AST query. Let’s try a broader grep search:
If this still returns nothing, please verify manually that Promise.silentCatch is implemented (and how it handles/logs errors), as we need confidence it won’t fully swallow critical loader failures.
🏁 Script executed:
#!/bin/bash
# Search for any usage or definition of silentCatch
rg -n 'Promise\.silentCatch'
rg -n 'silentCatch'Length of output: 379
Log suppressed errors in Promise.silentCatch for better visibility
Currently the first‐run loader errors are completely swallowed by silentCatch, making it hard to debug latent loader failures. To retain silent failure but still capture these errors:
• In codegenerator/cli/npm/envio/src/bindings/Promise.res (around line 41), enhance silentCatch to emit a log:
let silentCatch = (promise: promise<'a>): promise<'a> =>
- promise->Js.Promise.catch(_ => Js.Promise.resolve())
+ promise->Js.Promise.catch(err => {
+ /* debug: record suppressed error */
+ Js.log2("silentCatch suppressed error:", err);
+ Js.Promise.resolve()
+ })• Or, wrap the loader invocation in codegenerator/cli/templates/static/codegen/src/EventProcessing.res (line 221) to log per‐loader errors:
)->Promise.silentCatch,becomes
)->Promise.silentCatch
+ |> Js.Promise.tapError(err => {
+ Js.log2("First‐run loader error (suppressed):", err);
+ Js.Promise.resolve(None)
+ }),This preserves the intentional “ignore first run” semantics while surfacing suppressed errors for later troubleshooting.
🤖 Prompt for AI Agents
In codegenerator/cli/templates/static/codegen/src/EventProcessing.res around
lines 196 to 230, the current use of Promise.silentCatch in the first-run batch
loaders completely suppresses errors, which can hide important issues. To fix
this, modify the silentCatch implementation in
codegenerator/cli/npm/envio/src/bindings/Promise.res near line 41 to log
suppressed errors for visibility, or alternatively, wrap the loader invocation
at line 221 in EventProcessing.res with additional error logging that captures
and logs errors while still allowing the batch to continue silently. This
ensures errors are not lost but the first-run silent failure behavior is
preserved.
2 participants
Summary by CodeRabbit
getOrThrow(throws an error if the entity is missing) andgetOrCreate(creates the entity if it does not exist).getOrThrowandgetOrCreatemethods for entity handling, including error cases and custom error messages.