ShopApiPlugin icon indicating copy to clipboard operation
ShopApiPlugin copied to clipboard

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

Open diimpp opened this issue 5 years ago • 3 comments
trafficstars

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 avatar Oct 17 '20 01:10 diimpp

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')

diimpp avatar Oct 17 '20 02:10 diimpp

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.

lchrusciel avatar Oct 23 '20 10:10 lchrusciel

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

diimpp avatar Oct 23 '20 11:10 diimpp