fix: transaction in a workflow transform error #450
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
thantos
commented
Sep 18, 2023
•
thantos
commented
- BUG: Transaction in a workflow would error instead of returning a successful response
- Bad change during the executor refactor that assumed the base transaction executor return an object with success/fail and not just an object.
- BUG: In a transaction the condition operation would fail if the version was 0 (aka, should not exist yet).
- This happens when a entity.get return undefined within a transaction and we want to assert that the item doesn't exist by the end of the transaction.
- Root Cause: a EntityAttributeValueMap key was being created for expected version when the value was defined, but only used when defined or not 0.
- Solution: Fixed the logic to match.
- BUG: In Local, transaction could conflict when started at overlapping intervals.
- This was because the local transaction assumed that the calls would be synchronous when there were not. The Logic is 1. validate all versions, 2. apply changes, but the validation may be invalid by feat: add copy to README #2 AND the lower level operations will re-assert the expected version logic before applying, failing.
- Solution: TransactWrite only uses sync methods in local now, maintaining control through step 1 and 2.
sam-goodwin
reviewed
Sep 18, 2023
Comment on lines
-26
to
+33
| if (result.succeeded) { | ||
| return createEvent<TransactionRequestSucceeded>( | ||
| { | ||
| type: WorkflowEventType.TransactionRequestSucceeded, | ||
| result: result.output, | ||
| seq, | ||
| }, | ||
| executionTime | ||
| ); | ||
| } else { | ||
| return createEvent<TransactionRequestFailed>( | ||
| { | ||
| type: WorkflowEventType.TransactionRequestFailed, | ||
| error: "Transaction Failed", | ||
| message: "", | ||
| seq, | ||
| }, | ||
| executionTime | ||
| ); | ||
| } | ||
| return createEvent<TransactionRequestSucceeded>( | ||
| { | ||
| type: WorkflowEventType.TransactionRequestSucceeded, | ||
| result, | ||
| seq, | ||
| }, | ||
| executionTime | ||
| ); |
There was a problem hiding this comment.
The executor returns the value (any), not the object with the success boolean. turning success: false is done in the nested executor because that logic is shared between the workflow and the normal transaction call now (before it was duplicated).
sam-goodwin
reviewed
Sep 18, 2023
| return assertNever(item); | ||
| }) | ||
| ); | ||
| items.forEach((item) => { |
There was a problem hiding this comment.
No, the problem was that they were promises and happening in "parallel" with other things (ex: other transactions).
So now the whole function is async (except for the response).
sam-goodwin
approved these changes
Sep 19, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.