Json serializer #183
Json serializer #183
Conversation
…r' into custom-serializer # Conflicts: # dbos/queue.go # dbos/recovery.go # dbos/serialization.go # dbos/system_database.go # dbos/workflow.go
| // Wait for workflow completion | ||
| proceedSignal.Set() // Allow the workflow to proceed to step two | ||
| result, err := resumeHandle.GetResult() | ||
| resultAny, err := resumeHandle.GetResult() |
There was a problem hiding this comment.
We could make the resume handle typed so the returned polling handle can be of type T as well. (Right now on this path, the user gets a polling handle typed any, and the result is already json decoded into an any value, losing its type information)
| resultAny, err := h.GetResult() | ||
| require.NoError(t, err, "failed to get result from recovered root workflow handle") | ||
| castedResult, ok := result.([]int) | ||
| require.True(t, ok, "expected result to be of type []int for root workflow, got %T", result) | ||
| // re-encode and decode the result from []interface{} to []int | ||
| encodedResult, ok := resultAny.([]any) | ||
| require.True(t, ok, "expected result to be a []any") | ||
| jsonBytes, err := json.Marshal(encodedResult) | ||
| require.NoError(t, err, "failed to marshal result to JSON") | ||
| var castedResult []int | ||
| err = json.Unmarshal(jsonBytes, &castedResult) | ||
| require.NoError(t, err, "failed to decode result to []int") |
There was a problem hiding this comment.
on this path the result was already decoded into any, which lost type info. This is because recovery handles are untyped. Just needed for testing as recovery handles are not public.
| // Check if the value is nil (for pointer types, slice, map, etc.) | ||
| if isNilValue(data) { | ||
| // For nil values, return nil pointer |
There was a problem hiding this comment.
In Go, nil values are not necessarily pointers. We need the helper function for the case where T is any.
var p *int // p is nil
var v any = p // v is NOT nil (it's an interface containing a nil pointer)
fmt.Println(p == nil) // true
fmt.Println(v == nil) // false! The interface itself is not nil| type stepInfo struct { | ||
| StepID int // The sequential ID of the step within the workflow | ||
| StepName string // The name of the step function | ||
| Output *string // The output returned by the step (if any) | ||
| Error error // The error returned by the step (if any) | ||
| ChildWorkflowID string // The ID of a child workflow spawned by this step (if applicable) | ||
| } |
There was a problem hiding this comment.
Internal struct so we can distinguish easily between empty outputs and nil outputs. In v1 we can modify StepInfo's Output field to be of type *string instead of any.
| if _, ok := h.dbosContext.(*dbosContext); !ok { | ||
| return *new(R), newWorkflowExecutionError(workflowState.workflowID, fmt.Errorf("invalid DBOSContext: expected *dbosContext")) | ||
| } | ||
| serializer := newJSONSerializer[R]() | ||
| encodedOutput, encErr := serializer.Encode(decodedResult) |
There was a problem hiding this comment.
as an optimization we could pass the encoded value around the workflowOutcome...
| // Handle nil message | ||
| if msg == nil { | ||
| return *new(R), nil | ||
| } |
There was a problem hiding this comment.
Technically should never happen because we have a not null constraint on the message column.
|
|
||
| Launch(dbosCtx) | ||
|
|
||
| t.Run("SendRecvSuccess", func(t *testing.T) { |
There was a problem hiding this comment.
Fixed a race in this suite.
kraftp
left a comment
kraftp
left a comment
There was a problem hiding this comment.
This is getting closer! Questions:
- I don't like how there's a user-facing distinction between typed and untyped workflow handles. All handles should be typed.
- What exactly are list workflows and list steps returning now? The return types still seem to be
any, but we don't have type info. - How are we handling
nilvalues? Why do we need anisNilValuefunction and why is there special-casing if it's true?
Today only the client methods return untyped handles (RetrieveWorkflow, ResumeWorkflow, etc). That's because we don't expose generic package level methods (except for enqueue), but only interface methods on the client. (Except for enqueue) That being said, this PR allow users to declare workflows with
They return JSON strings or an zero The former could be done rather simply in this PR but will be a breaking change. I expect quite a bit more of refactor for the later (which would also be a breaking change).
We are storing nil values as |
On the get wf steps / list wf path, the contract is that if the value is nil, you know it was storing a nil value. Else, the value is a JSON string of your workflow's or step's
T.