Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: clone history events to prevent mutation #479

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions apps/tests/aws-runtime/test/test-service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1566,3 +1566,37 @@ export const getPresigned = command("getPresigned", {}, async () => {

return { key, putUrl, getUrl, headUrl, deleteUrl };
});

const mutateEntity = entity("mutate", {
partition: ["pk"],
attributes: {
pk: z.string(),
obj: z.object({
key: z.string(),
}),
},
});

// see: https://github.com/functionless/eventual/pull/479
export const mutateEvents = workflow("mutateEvents", async () => {
await mutateEntity.put({
pk: "pk",
obj: {
key: "v1",
},
});

let e = await mutateEntity.get({
pk: "pk",
});

e!.obj.key = "v2";

await mutateEntity.put(e!);

e = await mutateEntity.get({
pk: "pk",
});
Comment on lines +1597 to +1599
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: delete to clean up


return e!.obj.key;
});
3 changes: 3 additions & 0 deletions apps/tests/aws-runtime/test/tester.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {
eventDrivenWorkflow,
failedWorkflow,
heartbeatWorkflow,
mutateEvents,
parentWorkflow,
queueWorkflow,
SocketMessage,
Expand Down Expand Up @@ -53,6 +54,8 @@ eventualRuntimeTestHarness(

testCompletion("sleep", workflow3, "done!");

testCompletion("mutate events", mutateEvents, "v2");

testCompletion("parallel", workflow4, [
{ status: "fulfilled", value: ["hello sam", "hello chris", "hello sam"] },
{ status: "fulfilled", value: ["HELLO SAM", "HELLO CHRIS", "HELLO SAM"] },
Expand Down
4 changes: 4 additions & 0 deletions packages/@eventual/core-runtime/src/deep-clone.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
export function deepClone<T>(item: T): T {
// TODO: more efficient deep clone
return item === undefined ? item : JSON.parse(JSON.stringify(item));
Comment on lines +2 to +3
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

create issue?

}
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ export class RemoteExecutorProvider<Context = undefined>
executor: WorkflowExecutor<any, any, any>
): Promise<{ storedBytes: number }> {
// provides a shallow copy of the history events.
const historyEvents = executor.history.slice(0);
const historyEvents = executor.historyCloned.slice(0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe just use the cloned one in the executor's iterator and return the original array in .history?

historyEvents.push(...newHistoryEvents);
const { bytes } = await this.props.executionHistoryStateStore.updateHistory(
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ import {
type UnresolvedEventualDefinition,
} from "./eventual-definition.js";
import { formatExecutionId } from "./execution.js";
import { deepClone } from "../deep-clone.js";

/**
* Put the resolve method on the promise, but don't expose it.
Expand Down Expand Up @@ -206,13 +207,17 @@ export class WorkflowExecutor<Input, Output, Context = undefined> {
) => void;
} = {};

// clone the history so it cannot be modified by the user
public historyCloned: HistoryStateEvent[];

constructor(
private workflow: Workflow<any, Input, Output>,
public history: HistoryStateEvent[],
// TODO: properties used in the workflow should be encoded in the history to keep them constant between runs
private propertyRetriever: PropertyRetriever,
private eventualFactory: EventualFactory = createDefaultEventualFactory()
) {
this.historyCloned = deepClone(history);
Comment on lines +210 to +220
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe use .historyCloned in the iterator in here, leave it private, and then leave the original array in the .history?

this.nextSeq = 0;
this.expected = iterator(history, isCallEvent);
this.events = iterator(history, isCompletionEvent);
Expand Down
Loading