temporalio / sdk-typescript

Temporal TypeScript SDK
Other
535 stars 106 forks source link

[Feature Request] Worker.runReplayHistory() should accept a serialized History object fetched using fetchHistory() #1362

Open jhecking opened 8 months ago

jhecking commented 8 months ago

Is your feature request related to a problem? Please describe.

I'm testing a patched workflow, by replaying an "unpatched" workflow history, to verify that it does not fail with a non-determinism error. To get the workflow history, I'm using the fetchHistory() function of the workflow client. I persist the history object to disk using JSON.serialize:

const history = await client.workflow.getHandle('wfId').fetchHistory()
await fs.writeFile('./unpatched-workflow-history.json', JSON.stringify(history))

Then I add another unit tests that does nothing but load the serialized workflow history from disk (using JSON.parse) and replay it, e.g.

it('should replay an unpatched workflow history', async function () {
  const history = JSON.parse((await fs.readFile('./unpatched-workflow-history.json')).toString())
  Worker.runReplayHistory(DefaultWorkerOptions, history)
})

This fails with the following error:

Error: fromProto3JSONToInternalRepresentation: google.protobuf.Timestamp must be a string but got object

It took me a while to figure out the problem with Antonio's help: https://temporalio.slack.com/archives/C01DKSMU94L/p1708310456162399?thread_ts=1708279751.707509&cid=C01DKSMU94L

I thought I was following the docs here: https://docs.temporal.io/dev-guide/typescript/testing#replay. In particular, this example:

const filePath = './history_file.json';
const history = await JSON.parse(fs.promises.readFile(filePath, 'utf8'));
await Worker.runReplayHistory(
  {
    workflowsPath: require.resolve('./your/workflows'),
  },
  history,
);

What I didn't realize however, and what I don't think is very obvious, is that the JSON format returned by the Web UI and the Temporal CLI differs from the format returned by the fetchHistory() method.

Describe the solution you'd like

What I eventually found out is that I can use the History.fromObject function to turn the JSON object loaded from disk back into a History object that's accepted by Worker.runReplayHistory, e.g. this works:

it('should replay an unpatched workflow history', async function () {
  const history = History.fromObject(JSON.parse((await fs.readFile('./unpatched-workflow-history.json')).toString()))
  Worker.runReplayHistory(DefaultWorkerOptions, history)
})

I also noticed that the runReplayHistory method already has some built-in logic to determine whether the history input value is a JSON object (from the Web UI or CLI) or a History object. What would be nice is if it could also detect where it's being passed a History object serialized into a JSON object. It could just use History.fromObject in that case, just like I am doing now, as IMHO this distinction between the different JSON formats is very easy to miss and the conversion could be handled automatically for the user.

Additional context

Filing this feature request on the recommendation of Antonio as per our Slack conversation: https://temporalio.slack.com/archives/C01DKSMU94L/p1708310456162399?thread_ts=1708279751.707509&cid=C01DKSMU94L

bergundy commented 8 months ago

You can’t use plain JSON serialization on histories, they’re supposed to be encoded in a proto JSON format.

if you want to save a history you downloaded with a client you’ll need to use https://www.npmjs.com/package/proto3-json-serializer. runReplayHistory supports JSON histories in that format.

jhecking commented 8 months ago

Ok. Like I said, the way I've solved this problem is that I use History.fromObject() to convert the serialised history back into a History object before replaying it.

mjameswh commented 1 month ago

I recently added a historyToJSON() function in common's proto-utils.ts, to properly encode an existing History object to JSON, for internal purpose.

There's definitely user-side use cases for this, and implementing this correctly is far from trivial, due to bugs in proto3-json-serializer. We should therefore expose that function explicitly as part of some "Temporal Utils" API.