fix(medusa): Idempotency workStage used within transaction

[adrien2p]

What
The actual idempotency service workStage method was in a try catch block, when a transaction to wrap the work as been added, the try catch have created some side effects due to the fact that the transaction does not fail and end up in a specific state where the some data are rollback and some other data are committed.

How
Remove the try catch from the workStage and instead the catch must been placed at the higher level of transaction since we want to know if the transaction failed.

Tests
Add new integration test on the create-claims order as this is the one which have triggered this. The tests purposefully create a claim item with item without return reason which lead to an error. We then validate that there is no more claim order in the database once the stage has failed.

Notes
The other way of fixing it would be that the work stage create a child transaction serializable from either manager_ | transactionManager_.

FIXES CORE-680

[changeset-bot]

🦋 Changeset detected

Latest commit: c7ea7b1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
@medusajs/medusa	Minor
medusa-react	Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[pKorsholm]

If this is the solution we choose I think the code looks good. We just need to remove the error parameter from the return type.

What is the reasoning to choose this approach over, as you write, creating a transaction in the workStage method in order to get a proper rollback?

To me it seems that it would be the safer choice to use a transaction inside the workStage and it seems to logically make sense as well, not saying this doesn't, but it's not outrageous to create a transaction in the workStage in order to make sure that it's properly rolled back if something failed. We are wrapping all the workStage invocations in transactions individually anyway, this would clean up the invocation a bit. Furthermore throwing and not catching breaks the api of the service. In turn if anybody has implemented custom services using this they are required to update it.
Not saying that we shouldn't break the API as certain points, just that we need to be wary.

[pKorsholm]

remove error return param

[adrien2p]

If this is the solution we choose I think the code looks good. We just need to remove the error parameter from the return type.

What is the reasoning to choose this approach over, as you write, creating a transaction in the workStage method in order to get a proper rollback?

To me it seems that it would be the safer choice to use a transaction inside the workStage and it seems to logically make sense as well, not saying this doesn't, but it's not outrageous to create a transaction in the workStage in order to make sure that it's properly rolled back if something failed. We are wrapping all the workStage invocations in transactions individually anyway, this would clean up the invocation a bit. Furthermore throwing and not catching breaks the api of the service. In turn if anybody has implemented custom services using this they are required to update it. Not saying that we shouldn't break the API as certain points, just that we need to be wary.

I totally agree with the fact that it breaks a bit the way it works actually. But i think it is a trade off until we settle on the transaction refactoring to introduce a new pattern. At the moment, if a user is using the idempotency service without calling the withTransaction it will be broken on concurrent requests. If a user call the idempotency service in a transaction wrapper it will be broken as well as the last stage will not be rollback. This is why i choose to go with that approach as for now it ensure that the workflow works. But as you mentioned, we might need to indicate that some how to the users.

At the moment If we create a sub transaction and the user call it with the withTransaction the transaction wrapper will not rollback as well. And if the user call it without the withTransaction then then it will still not rollback the main transaction as it will be catch inside and the manager will be shared as it was previously

@olivermrbl any comment regarding those points?

[olivermrbl]

But i think it is a trade off until we settle on the transaction refactoring to introduce a new pattern

I am not entirely sure, I see the trade-off. From what I understand, adding a serializable child transaction in the work stage would fix the issue and not break the API, so I would immediately think this would be the better choice of the two options here.

Can I get you to elaborate on the issues with that approach? Introducing a new transaction management pattern is still quite far out in the future, so I want to ensure we are making the correct trade-off with that in mind.

[adrien2p]

After discussion with oli, it has been decided to go with that refactoring as it does not create any hidden behaviour and is pretty straight forward and explicit. The changement on the service api will be documented in order for a user to be able to use the idempotency key service properly

[olivermrbl]

Great work. Can I get you to add a changeset with a minor dump to @medusajs/medusa?

[olivermrbl]

LGTM, I've mentioned you in an issue in Linear regarding release content. We'll have to include a short description of this one specifically, as we'll be breaking the API of the idempotency key service.