feat: add an api to add items to a cart atomically #13
Conversation
raymondfeng
force-pushed
the
add-cart-item
branch
from
8c6d8c9 to
8d27b46
Compare
| async addItem(userId: string, item: ShoppingCartItem) { | ||
| const connector = this.kvModelClass.dataSource!.connector!; | ||
| // tslint:disable-next-line:no-any | ||
| const execute = promisify((cmd: string, args: any[], cb: Function) => { |
There was a problem hiding this comment.
I'm not sure I follow what execute does? Apart for this and it's usage, the rest of it looks good.
There was a problem hiding this comment.
execute basically runs a Redis command.
There was a problem hiding this comment.
But we're still using this.get, this.set? So what do the commands do?
There was a problem hiding this comment.
this.get/this.set delegate to execute behind the scene.
There was a problem hiding this comment.
The reason to use execute is that we don't have a this.checkAndSet. Maybe we should add it to the KVRepository, such as:
checkAndSet(key, current => {... return newValue; });There was a problem hiding this comment.
Read up some more on Redis commands and transactions at https://redis.io/commands
Makes sense, I can see this being a common pattern so a helper function for transaction would definitely be a good idea. That said I'm not sure why this needs to be a transaction as there is only one item (shopping cart). We're not creating items themselves as entries in Redis.
There was a problem hiding this comment.
I'm considering the multiple device case. A user can access the app from multiple devices that share the same cart.
There was a problem hiding this comment.
But each call does a get and then set. Is a user really going to generate multiple requests at the same time from multiple devices?
Either way, I'm ok with the change -- can you just add a comment stating the reason for using a transaction so it's clear to anyone looking at this example.
There was a problem hiding this comment.
Added more comments.
| @property() | ||
| price?: number; | ||
|
|
||
| constructor(data?: Partial<ShoppingCartItem>) { |
There was a problem hiding this comment.
Is the partial needed in this case since this isn't a model directly exposed over REST and won't face issues of trying to create an instance without a productId?
There was a problem hiding this comment.
Not a big deal as the data itself is optional. So we can create a empty instance and populate the data afterward. The required constraint should be enforced by model validation.
There was a problem hiding this comment.
The required constraint should be enforced by model validation.
@raymondfeng Please note that KeyValue repository does not perform validation right now! See lib/kvao/set.js. I think this is an oversight we should eventually fix.
raymondfeng
force-pushed
the
add-cart-item
branch
from
8d27b46 to
cfe7375
Compare
Signed-off-by: Raymond Feng <[email protected]>
raymondfeng
force-pushed
the
add-cart-item
branch
from
cfe7375 to
7b9c903
Compare
|
|
||
| /** | ||
| * Use Redis WATCH and Transaction to check and set against a key | ||
| * See https://redis.io/topics/transactions#optimistic-locking-using-check-and-set |
There was a problem hiding this comment.
@raymondfeng IIUC, the algorithm outlined in the linked page is expecting to re-run the transaction if a race condition was detected, and repeat those re-run until the transaction succeed.
Using the above code, if there are race conditions and another client modifies the result of val in the time between our call to WATCH and our call to EXEC, the transaction will fail.
We just have to repeat the operation hoping this time we'll not get a new race. This form of locking is called optimistic locking and is a very powerful form of locking. In many use cases, multiple clients will be accessing different keys, so collisions are unlikely – usually there's no need to repeat the operation.
I don't see that logic implemented here. Are we expecting the REST clients to retry the operation? Could you please explain how are we signaling the race condition to them?
| userId: 'user-0001', | ||
| items: [ | ||
| { | ||
| new ShoppingCartItem({ |
There was a problem hiding this comment.
Can you use givenAnItem() please?
The PR adds an
addItemapi to atomically add items to a cart.The implementation utilizes
execute, which is a bit hacky.