search

Sylius / ShopApiPlugin

Shop API for Sylius.
https://sylius.com
130 stars 89 forks source link

[Bug] CartBlamerListener is never called on carts as it listens on incorrect event #679

Open diimpp opened 3 years ago

diimpp commented 3 years ago

CartBlamer's purpose is to be called on each request by authenticated customer and in case current request is one of /carts/{token} requests, then attach current customer to the cart.

This never happens and therefore AssignCustomerToCart is not called until after checkout is completed.

https://github.com/Sylius/ShopApiPlugin/blob/0cd451b923cec477884628bcc6104b4cbe1998d0/src/Resources/config/services.xml#L67

Reason is that it listens to on_jwt_created (Which happens only on login itself, no cart token there obliviously) instead of on_jwt_authenticated. https://github.com/lexik/LexikJWTAuthenticationBundle/blob/master/Events.php (on_authentication_success doesn't seems to work regardless the name or more suited event class)

I think more proper solution would be to listen to kernel.request event and check if current user is authenticated, then it will decouple plugin from any specific authentication library.

Another problem is the the way carts route is detected - by trying to find cart under any route with {token} argument. Maybe we can do some kind of shop-api firewall check or match /shop-api/carts route prefix.

diimpp commented 3 years ago

https://github.com/Sylius/ShopApiPlugin/blob/0cd451b923cec477884628bcc6104b4cbe1998d0/src/EventListener/CartBlamerListener.php#L48

Also it should be $request->attributes->get('token'), not $request->request->get('token')

lchrusciel commented 3 years ago

Some resolution of /shop-api/cart routes makes sense to me.

This was done this way because I wanted to reduce the amount of calls to CartBlamer. Previously, due to symfony.on_iteraction_login event was trigger with every request (due to being stateless). This resulted in the recalculation logic of the cart for each request. From this moment it was pretty easy to run into race conditions with higher traffic.

diimpp commented 3 years ago

Extra race condition/deadlocks are avoidable with check if customer already assigned, so no extra order processing would be necessary.

By the way, race conditions are pretty heavy right now without this call at all. They happen at concurrent order processing calls per same cart per customer. We had this issue on my projects for months, until I've found a solution with LockingOrderProcesser decoration with symfony/lock as described at https://github.com/Sylius/ShopApiPlugin/issues/667