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(medusa): Transaction lock issues on create/update cart items #2612

Merged
merged 20 commits into from
Nov 18, 2022

Conversation

adrien2p
Copy link
Member

@adrien2p adrien2p commented Nov 16, 2022

what

The fixes will be done in multiple steps

  • (This PR) Add idempotency support on the create-line-item handler and few performance improvements along the way
  • (later) Ensure the create line item only handler line item creation and not update
  • (later) Refactor cart service add line item method to multiple smaller method with each of them having their responsibility

FIXES CORE-801

@changeset-bot
Copy link

changeset-bot bot commented Nov 16, 2022

🦋 Changeset detected

Latest commit: 75b9822

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

This PR includes changesets to release 1 package
Name Type
@medusajs/medusa Patch

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

@adrien2p adrien2p marked this pull request as ready for review November 16, 2022 11:10
@adrien2p adrien2p requested a review from a team as a code owner November 16, 2022 11:10
@adrien2p adrien2p force-pushed the fix/create-item-transaction-lock branch from d115c30 to 2066980 Compare November 16, 2022 11:11
Copy link
Collaborator

@srindom srindom left a comment

Choose a reason for hiding this comment

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

This fix will require the caller of addLineItem to always bring a cart's line items to a valid state. I don't think we should force the caller to hold this responsibility.

Could we instead update the isolation level in the controller to be serializable and throw a 409 if there are read/write dependencies?

@srindom
Copy link
Collaborator

srindom commented Nov 16, 2022

If we make the endpoint idempotent, we can add retry functionality in the js client that automatically retries the request if we reach a 409.

@adrien2p
Copy link
Member Author

adrien2p commented Nov 16, 2022

Yes sure, i should add idempotency support in that case but yeah if we explicitly explain to the consumer that in some cases it needs to be retried i am ok with that 👍 i thought about it but wasn’t sure as it transform the endpoint management 🥹

@adrien2p adrien2p force-pushed the fix/create-item-transaction-lock branch 3 times, most recently from ca034d5 to f1f302a Compare November 17, 2022 11:10
Copy link
Collaborator

@srindom srindom left a comment

Choose a reason for hiding this comment

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

Wdyt of avoiding joining all items in addLineItem like I have done here: develop...fix/serialized-add-to-cart#diff-b1b00df3e31608f5099498d5dc7dfc5f92032809e1e16cbd7b1e9a85304c8edbR578

Curious if this would be more performant in cases where the cart holds many items, despite the extra db queries?

packages/medusa/src/services/cart.ts Show resolved Hide resolved
packages/medusa/src/services/cart.ts Outdated Show resolved Hide resolved
@adrien2p adrien2p requested a review from srindom November 18, 2022 08:47
@adrien2p adrien2p force-pushed the fix/create-item-transaction-lock branch from 153f5e4 to 902aefe Compare November 18, 2022 09:11
Copy link
Contributor

@olivermrbl olivermrbl left a comment

Choose a reason for hiding this comment

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

LGTM, will let @srindom take a final stab at it as he had a couple of suggestions.

Copy link
Collaborator

@srindom srindom left a comment

Choose a reason for hiding this comment

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

LGTM!

@adrien2p adrien2p merged commit a777806 into develop Nov 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants