feat: add compensation feature to deno context and utils #945
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive compensation mechanism across multiple components of the system. The changes span protocol definitions, Rust and TypeScript converters, runtime contexts, and test workflows. The core addition is a new Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 4
🧹 Nitpick comments (4)
src/typegate/src/runtimes/substantial/deno_context.ts (3)
140-140: Simplify the conditional checkSince
compensationStackis always initialized as an array, the checkcompensationStack &&is redundant. You can simplify the condition to:Apply this diff:
- if (compensationStack && compensationStack.length) { + if (compensationStack.length) {🧰 Tools
🪛 Biome (1.9.4)
[error] 140-140: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
143-143: Simplify the compensation function callYou can simplify
await Promise.resolve(compensationFn())toawait compensationFn(). Theawaitoperator already handles both synchronous and asynchronous functions.Apply this diff:
- const result = await Promise.resolve(compensationFn()); + const result = await compensationFn();
509-513: Consider adding input validation torandom(a, b)Currently, the
randommethod does not validate ifais less than or equal tob. Adding input validation can prevent potential errors whena > b.You might consider adding a check like:
if (a > b) { throw new Error('Invalid arguments: a must be less than or equal to b'); }.helix/languages.toml (1)
17-24: Consider enabling unstable APIs.Since Deno often requires unstable APIs for certain features, consider adding the
unstableflag to the configuration:[language-server.deno-lsp.config.deno] enable = true +unstable = true suggest = { completeFunctionCalls = false, imports = { hosts = { "https://deno.land" = true } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.helix/languages.toml(1 hunks)src/substantial/protocol/events.proto(2 hunks)src/substantial/src/converters.rs(3 hunks)src/substantial/src/protocol/events.rs(11 hunks)src/typegate/engine/runtime.d.ts(4 hunks)src/typegate/src/runtimes/substantial/deno_context.ts(10 hunks)tests/runtimes/substantial/common.ts(4 hunks)tests/runtimes/substantial/imports/common_types.ts(2 hunks)tests/runtimes/substantial/substantial.py(2 hunks)tests/runtimes/substantial/workflows/workflow.ts(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/typegate/src/runtimes/substantial/deno_context.ts
[error] 140-140: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: lint-compat (macos-14, aarch64-apple-darwin, false)
- GitHub Check: bulid-docker (linux/amd64, custom-ubuntu-large)
- GitHub Check: lint-compat (macos-13, x86_64-apple-darwin, false)
- GitHub Check: test-full
- GitHub Check: pre-commit
🔇 Additional comments (12)
src/substantial/src/converters.rs (2)
70-74:Compensatevariant is correctly definedThe new
Compensatevariant inOperationEventis well-defined with appropriate fields, enhancing the handling of compensation events.
351-367:TryFrom<Operation>implementation correctly handlesCompensateThe conversion logic for the
Compensateevent is properly implemented, ensuring all fields are accurately converted and serialized.tests/runtimes/substantial/imports/common_types.ts (3)
51-51: Addedutilsproperty toContextinterfaceThe
utilsproperty is appropriately added to theContextinterface, providing utility methods for workflows.
83-84: IncludedcompensateWithinSaveOptioninterfaceThe optional
compensateWithfunction is correctly added toSaveOption, allowing for compensation logic in save operations.
86-89: Defined theUtilsinterfaceThe
Utilsinterface is properly defined with the expected methodsnow,random, anduuid4, enhancing utility functionality.src/substantial/protocol/events.proto (2)
34-38: Consider using a JSON string for compensation_result.Similar to
SaveResolved.json_result, consider documenting that thecompensation_resultfield should contain a JSON-encoded string for consistency.
67-67: LGTM!The new field
compensationis correctly added to theEventmessage with an appropriate field number, and it's not part of theoneofgroup which allows it to coexist with other event types..helix/languages.toml (1)
1-14: LGTM!The JavaScript and TypeScript language configurations are properly set up with Deno integration, including all necessary fields like shebangs, roots, file types, and language servers.
src/substantial/src/protocol/events.rs (1)
Line range hint
902-2426: LGTM! Generated code matches protocol definitions.The generated Rust code correctly implements:
- The new
Compensationmessage with all fields- The
Eventmessage modifications- All necessary traits and methods
The file is properly marked as generated and should not be manually edited.
tests/runtimes/substantial/substantial.py (2)
34-34: LGTM! Import added for compensation feature.The compensation example is correctly added to the list of imports.
88-95: LGTM! Compensation endpoint properly configured.The compensation endpoint is well-structured with:
- Proper type definition for the account parameter
- Correct workflow name mapping
src/typegate/engine/runtime.d.ts (1)
325-330: LGTM! Compensation event type properly defined.The Compensate event type is well-structured with all necessary fields:
- save_id for tracking the operation
- error for failure details
- compensation_result for tracking the compensation outcome
| if (option?.retry?.compensationOnfristFail) { | ||
| await this.#triggerCompensation(id, err); | ||
| throw err; | ||
| } |
There was a problem hiding this comment.
Fix the typo in 'compensationOnfristFail' to 'compensationOnFirstFail'
There is a typo in the property name compensationOnfristFail. It should be compensationOnFirstFail throughout the code to reflect the correct spelling.
Apply this diff to correct the typo:
- compensationOnfristFail
+ compensationOnFirstFailAlso applies to: 385-385
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/runtimes/substantial/common.ts(2 hunks)tests/runtimes/substantial/workflows/workflow.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: test-full
- GitHub Check: lint-compat (macos-14, aarch64-apple-darwin, false)
- GitHub Check: lint-compat (macos-13, x86_64-apple-darwin, false)
- GitHub Check: bulid-docker (linux/amd64, custom-ubuntu-large)
- GitHub Check: pre-commit
🔇 Additional comments (3)
tests/runtimes/substantial/workflows/workflow.ts (2)
176-181:⚠️ Potential issueFix compensation amount mismatch.
The compensation amount should match the debit amount. Currently, it debits 2 but credits 4.
Apply this diff:
await ctx.save(() => { - debitAccount(2); + const debitAmount = 2; + debitAccount(account, debitAmount); risky_transaction(); }, { - compensateWith: () => creditAccount(4), + compensateWith: () => creditAccount(account, debitAmount), });Likely invalid or redundant comment.
156-166: 🛠️ Refactor suggestionMove helper functions outside for reusability and testing.
The helper functions should be moved outside the main function to improve reusability and testability.
Apply this diff:
+const debitAccount = (account: number, value: number) => { + if (account < value) { + throw new Error(`Insufficient balance: ${account} < ${value}`); + } + return account - value; +}; + +const creditAccount = (account: number, value: number) => { + return account + value; +}; + +const risky_transaction = () => { + throw Error("Transaction Failed"); +}; + export async function compensation(ctx: Context) { const { account } = ctx.kwargs; - - const debitAccount = (value: number) => { - return account - value; - }; - - const creditAccount = (value: number) => { - return account + value; - }; - - const risky_transaction = () => { - throw Error("Transaction Failed"); - };Likely invalid or redundant comment.
tests/runtimes/substantial/common.ts (1)
639-680: LGTM!The implementation of the filter test case is correct and comprehensive.
tests/runtimes/substantial/common.ts
Outdated
| const account_balance = 1000; | ||
|
|
||
| await t.should("compensate account balance", async () => { | ||
| await gql` | ||
| mutation { | ||
| start_compensation(kwargs: { account: $account_balance }) | ||
| } | ||
| ` | ||
| .withVars({ account_balance }) | ||
| .on(e); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve test coverage for compensation workflow.
The current test only verifies the initial state. Consider adding:
- Verification of compensation workflow execution
- Verification of intermediate states and error handling
Add the following test cases:
// Add after line 168
await t.should("verify compensation workflow execution", async () => {
await gql`
query {
results(name: "compensation") {
completed {
runs {
result {
status
value
}
}
}
}
}
`
.expectData({
results: {
completed: {
runs: [{
result: {
status: "COMPLETED_WITH_ERROR",
value: "Transaction Failed"
}
}]
}
}
})
.on(e);
});
await t.should("verify account state after compensation", async () => {
await gql`
query {
results(name: "compensation") {
completed {
runs {
events {
type
compensation {
save_id
error
compensation_result
}
}
}
}
}
}
`
.expectBody((body) => {
const events = body.data.results.completed.runs[0].events;
const compensationEvents = events.filter(
(e: any) => e.type === "compensation"
);
assertEquals(compensationEvents.length > 0, true);
assertEquals(
compensationEvents.every((e: any) => e.compensation.compensation_result === 1000),
true,
"Account balance not restored after compensation"
);
})
.on(e);
});
.helix/languages.toml
Outdated
| [[language]] | ||
| name = "javascript" | ||
| shebangs = ["deno"] | ||
| roots = ["deno.json", "deno.jsonc"] | ||
| file-types = ["js"] | ||
| language-servers = ["deno-lsp"] | ||
| auto-format = true | ||
| [[language]] | ||
| name = "typescript" | ||
| shebangs = ["deno"] |
There was a problem hiding this comment.
no , its just for helix (editor) like .vscode ... , i'll remove this ...
tests/runtimes/substantial/common.ts
Outdated
| await t.should("compensate account balance", async () => { | ||
| await gql` | ||
| mutation { | ||
| start_compensation(kwargs: { account: $account_balance }) | ||
| } | ||
| ` | ||
| .withVars({ account_balance }) | ||
| .on(e); | ||
| }); |
There was a problem hiding this comment.
Shouldn't this expect an error?
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
tests/runtimes/substantial/workflows/workflow.ts (2)
153-167: 🛠️ Refactor suggestionMove helper functions outside for better reusability and testability.
The helper functions should be moved outside the main function to improve code organization and enable unit testing.
Apply this diff:
+const debitAccount = (account: number, value: number) => { + return account - value; +}; + +const creditAccount = (account: number, value: number) => { + return account + value; +}; + +const risky_transaction = () => { + throw Error("Transaction Failed"); +}; + export async function compensation(ctx: Context) { const { account } = ctx.kwargs; - - const debitAccount = (value: number) => { - return account - value; - }; - - const creditAccount = (value: number) => { - return account + value; - }; - - const risky_transaction = () => { - throw Error("Transaction Failed"); - };
168-170: 🛠️ Refactor suggestionAdd balance validation and consistent assertions.
The first transaction includes an assertion but others don't. Also, balance validation is missing.
Add balance validation and consistent assertions:
+ const validateTransaction = (balance: number, amount: number) => { + assertEquals(typeof amount, 'number', 'Amount must be a number'); + assertEquals(amount > 0, true, 'Amount must be positive'); + assertEquals(balance >= amount, true, 'Insufficient balance'); + }; await ctx.save(() => { + validateTransaction(account, 4); debitAccount(4) }, { compensateWith: () => [creditAccount(4), assertEquals(account, 1000)], });src/typegate/src/runtimes/substantial/deno_context.ts (1)
385-385: 🛠️ Refactor suggestionFix the typo in 'compensationOnfristFail'.
The property name has a typo.
Apply this diff:
- compensationOnfristFail: boolean; + compensationOnFirstFail: boolean;tests/runtimes/substantial/common.ts (1)
168-177: 🛠️ Refactor suggestionEnhance test coverage for compensation workflow.
The current test only verifies the error case. Add tests for successful compensation and intermediate states.
Add these test cases:
await t.should("verify compensation workflow execution", async () => { await gql` query { results(name: "compensation") { completed { runs { result { status value } } } } } ` .expectData({ results: { completed: { runs: [{ result: { status: "COMPLETED_WITH_ERROR", value: "Transaction Failed" } }] } } }) .on(e); }); await t.should("verify account state after compensation", async () => { await gql` query { results(name: "compensation") { completed { runs { events { type compensation { save_id error compensation_result } } } } } } ` .expectBody((body) => { const events = body.data.results.completed.runs[0].events; const compensationEvents = events.filter( (e: any) => e.type === "compensation" ); assertEquals(compensationEvents.length > 0, true); assertEquals( compensationEvents.every((e: any) => e.compensation.compensation_result === 1000), true, "Account balance not restored after compensation" ); }) .on(e); });
🧹 Nitpick comments (4)
tests/runtimes/substantial/workflows/workflow.ts (2)
172-174: Add missing assertions to remaining transactions.For consistency, all transactions should include balance assertions in their compensation handlers.
Apply this diff:
await ctx.save(() => debitAccount(10), { - compensateWith: () => creditAccount(10), + compensateWith: () => [creditAccount(10), assertEquals(account, 1000)], }); await ctx.save(() => debitAccount(100), { - compensateWith: () => creditAccount(100), + compensateWith: () => [creditAccount(100), assertEquals(account, 1000)], });Also applies to: 183-185
176-181: Fix potential race condition in account updates.The current implementation might lead to race conditions as account balance is shared state. Consider using atomic operations or a proper transaction mechanism.
Consider these improvements:
- Use a proper transaction manager to handle atomic operations
- Implement optimistic locking
- Add version control to detect concurrent modifications
Would you like me to provide an example implementation?
src/typegate/src/runtimes/substantial/deno_context.ts (2)
140-140: Use optional chaining for better readability.The condition can be simplified using optional chaining.
Apply this diff:
- if (compensationStack && compensationStack.length) { + if (compensationStack?.length) {🧰 Tools
🪛 Biome (1.9.4)
[error] 140-140: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
502-518: Improve Utils class implementation.The Utils class implementation could be improved for better error handling and type safety.
Consider these improvements:
- Add input validation for the random method
- Add error handling for UUID generation
- Add proper typing for return values
class Utils { constructor(private ctx: Context) {} async now() { return await this.ctx.save(() => new Date()); } async random(a: number, b: number) { + if (typeof a !== 'number' || typeof b !== 'number') { + throw new TypeError('Arguments must be numbers'); + } + if (a > b) { + throw new Error('First argument must be less than or equal to second argument'); + } return await this.ctx.save(() => Math.floor(Math.random() * (b - a + 1)) + a ); } async uuid4() { + try { return await this.ctx.save(() => randomUUID()); + } catch (error) { + throw new Error(`Failed to generate UUID: ${error.message}`); + } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/typegate/src/runtimes/substantial/deno_context.ts(10 hunks)tests/runtimes/substantial/common.ts(1 hunks)tests/runtimes/substantial/workflows/workflow.ts(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/typegate/src/runtimes/substantial/deno_context.ts
[error] 140-140: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: lint-compat (macos-14, aarch64-apple-darwin, false)
- GitHub Check: bulid-docker (linux/amd64, custom-ubuntu-large)
- GitHub Check: lint-compat (macos-13, x86_64-apple-darwin, false)
- GitHub Check: test-full
- GitHub Check: pre-commit
try to make test run, still have prblm to query workflow
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
tests/runtimes/substantial/workflows/workflow.ts (2)
158-168: 🛠️ Refactor suggestionMove helper functions outside the main function.
Helper functions should be pure and moved outside the main function for better reusability and testing.
+const debitAccount = (account: number, value: number) => { + return account - value; +}; + +const creditAccount = (account: number, value: number) => { + return account + value; +}; + +const risky_transaction = () => { + throw Error("Transaction Failed"); +}; + export async function compensationExample(ctx: Context) { const { account } = ctx.kwargs; - - const debitAccount = (value: number) => { - return account - value; - }; - - const creditAccount = (value: number) => { - return account + value; - }; - - const risky_transaction = () => { - throw Error("Transaction Failed"); - };
170-190: 🛠️ Refactor suggestionAdd consistent balance validation and assertions across all transactions.
The transactions lack proper balance validation and have inconsistent assertions. Only the first transaction includes an assertion.
- Add a validation helper:
const validateTransaction = (balance: number, amount: number) => { assertEquals(typeof amount, 'number', 'Amount must be a number'); assertEquals(amount > 0, true, 'Amount must be positive'); assertEquals(balance >= amount, true, 'Insufficient balance'); };
- Apply consistent validation and assertions:
await ctx.save(() => { + validateTransaction(account, 4); - debitAccount(4) + return debitAccount(account, 4); }, { - compensateWith: () => { - creditAccount(4); - assertEquals(account, 1000); - }, + compensateWith: () => [creditAccount(account, 4), assertEquals(account, 1000)], }); await ctx.save(() => { + validateTransaction(account, 10); - debitAccount(10) + return debitAccount(account, 10); }, { - compensateWith: () => creditAccount(10), + compensateWith: () => [creditAccount(account, 10), assertEquals(account, 1000)], }); await ctx.save(() => { + validateTransaction(account, 2); - debitAccount(2); + const newBalance = debitAccount(account, 2); risky_transaction(); + return newBalance; }, { - compensateWith: () => creditAccount(2), + compensateWith: () => [creditAccount(account, 2), assertEquals(account, 1000)], }); await ctx.save(() => { + validateTransaction(account, 100); - debitAccount(100) + return debitAccount(account, 100); }, { - compensateWith: () => creditAccount(100), + compensateWith: () => [creditAccount(account, 100), assertEquals(account, 1000)], });
🧹 Nitpick comments (3)
tests/runtimes/substantial/imports/common_types.ts (2)
84-84: Consider using a more specific return type for compensateWith.The
anyreturn type loses type safety. Consider defining a more specific type that represents the expected compensation result.- compensateWith?: () => any | Promise<any>; + compensateWith?: () => CompensationResult | Promise<CompensationResult>;Where
CompensationResultcould be defined as:type CompensationResult = { success: boolean; message?: string; data?: unknown; };
87-90: Add JSDoc comments to document the Utils interface.The interface methods would benefit from documentation, especially the
randommethod's parameters and the range behavior.export interface Utils { + /** Returns the current date and time */ now: () => Promise<Date>; + /** Returns a random number between a (inclusive) and b (exclusive) */ random: (a: number, b: number) => Promise<number>; + /** Generates a random UUID v4 string */ uuid4: () => Promise<string>; }tests/runtimes/substantial/workflows/workflow.ts (1)
155-156: Add JSDoc documentation and type safety.Add documentation explaining the purpose of this example and type safety for the account parameter.
+/** + * Example demonstrating compensation mechanism in workflows. + * Shows how to handle transaction rollbacks when operations fail. + * @param ctx Context object containing workflow parameters + * @param ctx.kwargs.account {number} The account balance to operate on + */ -export async function compensationExample(ctx: Context) { +export async function compensationExample(ctx: Context & { kwargs: { account: number } }) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.ghjk/deno.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
tests/runtimes/substantial/common.ts(2 hunks)tests/runtimes/substantial/imports/common_types.ts(2 hunks)tests/runtimes/substantial/substantial.py(2 hunks)tests/runtimes/substantial/workflows/workflow.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/runtimes/substantial/substantial.py
- tests/runtimes/substantial/common.ts
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: test-full
- GitHub Check: lint-compat (macos-14, aarch64-apple-darwin, false)
- GitHub Check: lint-compat (macos-13, x86_64-apple-darwin, false)
- GitHub Check: bulid-docker (linux/amd64, custom-ubuntu-large)
- GitHub Check: pre-commit
🔇 Additional comments (2)
tests/runtimes/substantial/imports/common_types.ts (2)
50-51: LGTM! Well-structured interface extensions.The addition of
loggerandutilsproperties to the Context interface provides a clean way to access logging and utility functions.
Line range hint
4-7: Track TODOs in the issue system.The TODO comments lack specificity and should be tracked in the issue system:
- "include this as part of the metagen generated code"
- "merge these"
Please create corresponding issues and update these comments with the issue references.
Would you like me to help create these issues with detailed descriptions?
| @@ -78,7 +79,15 @@ export interface SaveOption { | |||
| minBackoffMs: number; | |||
| maxBackoffMs: number; | |||
| maxRetries: number; | |||
| compensationOnfristFail: boolean; | |||
There was a problem hiding this comment.
Fix typo in property name.
The property name contains a typo: compensationOnfristFail should be compensationOnFirstFail.
- compensationOnfristFail: boolean;
+ compensationOnFirstFail: boolean;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| compensationOnfristFail: boolean; | |
| compensationOnFirstFail: boolean; |
| @@ -75,6 +77,7 @@ export async function retryExample(ctx: Context) { | |||
| minBackoffMs: 1000, | |||
| maxBackoffMs: 5000, | |||
| maxRetries: 4, | |||
| compensationOnfristFail: false, | |||
There was a problem hiding this comment.
Fix typo in retry configuration property name.
The property name compensationOnfristFail contains a typo. It should be compensationOnFirstFail.
- compensationOnfristFail: false,
+ compensationOnFirstFail: false,Also applies to: 100-100
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #945 +/- ##
==========================================
+ Coverage 77.77% 77.83% +0.06%
==========================================
Files 160 160
Lines 19657 19711 +54
Branches 1964 1964
==========================================
+ Hits 15289 15343 +54
Misses 4347 4347
Partials 21 21 ☔ View full report in Codecov by Sentry. |
Add compensation && utils feature to deno context
Migration notes
Summary by CodeRabbit
Release Notes
New Features
Protocol Updates
Compensationmessage type to capture detailed error and compensation information.Eventmessage to support compensation-related data.Type Definitions
OperationEventtype with additional compensation details.Tests
Chores
.helix/to.gitignore.