Passing arguments in state accessor and mutator

[stevenferrer]

Hi @qmuntal,

We're implementing a state machine that uses Postgres as a backend for external storage. As a solution for the concurrent writes issue, we needed to use row-level locks in Postgres. For this to work, we have to make use of the same transaction object on both state accessor and mutator.

We currently have no way to explicitly pass this via state accessor and mutator so we're injecting the transaction object in the context.

// inject tx to context
tx, _ := db.BeginTx()
ctx := context.WithValue(ctx, "ctxKey", tx)

// access tx from context
func stateAccessor(ctx context.Context) (interface{}, error) {
    tx, ok := ctx.Value("ctxKey").(Tx)
    // read the current state using the tx object
}

// access the same  tx from context
func stateMutator(ctx context.Context, nextState interface{}) error {
    tx, ok := ctx.Value("ctxKey").(Tx)
    // write the next state and commit using the tx object
}

The above solution can work but is not ideal since we're injecting a complex object into the context.

What I propose is to extend the signature of state accessor and mutator to also accept the variadic arguments from the StateMachine.FireCtx method.

func stateMutator(ctx context.context, args ...interface{}) (interface{}, error) { }

func stateAccessor(ctx context.Context, nextState interface{}, args ...interface{}) error { }

With this change, the expected behavior is, when I call StateMachine.FireCtx with argument tx, I should be able to access that argument from both state accessor and mutator.

// Fire an event
stateMachine.Fire(ctx, someTrigger, tx)

// access tx from from args
func stateAccessor(ctx context.Context, args ...interface{}) (interface{}, error) {
    tx, ok := args[0].(Tx)
    // read the current state
}

// access the same  tx from args
func stateMutator(ctx context.Context, nextState interface{}, args ...interface{}) error {
    tx, ok := args[0].(Tx)
    // write the next state and commit using the tx object
}

I'm not sure how this would affect the existing API, but I'm willing to help with the change if needed. Please let me know if you need more clarification regarding this.

Thanks a lot in advance!

[qmuntal]

Thanks for the detailed proposal @stevenferrer. Passing down the arguments from FireCtx to the state mutator/accessor function is feasible, even without breaking existing API. We could have another state machine constructor that accepts your proposed signatures.

On the other hand, I'm reluctant to add this functionality. The state callbacks are just for retrieving and updating the state, no additional logic should happen there, else the state machine pattern might be defeated. Although your use-case is completely valid, it adds complexity to the API and other users might have problems understanding why is it there when the state machine can already be configured using the StateConfiguration.

On top of that, I don't think that wrapping db.BeginTx() in the context is worst that passing it as a variadic argument, both needs to type cast an interface. Could you elaborate more on why you don't want to pass it inside the context? I'm probably missing something.

[stevenferrer]

Hi @qmuntal, thank you very much for your response and apologies for my late reply.

The reason that we wanted to explicitly pass the tx as an argument instead of injecting in the context is because of some blog post that I read a few months ago (I forgot the link), which suggests only passing simply objects instead of complex objects like structs.

I agree with you and I understand that this feature might add complexity to the library. Actually, we're doing okay at the moment by only passing the tx object in the context. Though, as the library evolves, I'm hoping that these kinds of use-cases would be considered in the future versions of stateless.

[stevenferrer]

...On top of that, I don't think that wrapping db.BeginTx() in the context is worst that passing it as a variadic argument, both needs to type cast an interface.

Closing this issue as I have changed my opinions about this topic. This was something specific to our use-case and not a limitation to the library.