B is not idempotent for identical requests #100

Activity

1. civsiv

   added

   bugSomething isn't workingSomething isn't working

   on Mar 5, 2021
2. lukehesluke commented on May 6, 2021

   

   lukehesluke

   on May 6, 2021

   Contributor

   More actions

   I concur. Here is a test run which tests idempotency (and fails):

   free-opportunities_opportunity-free-without-checkpoints_OpenBookingSimpleFlow_FacilityUseSlot.md

   This is only for Simple Flow, btw. Approval Flow allows a repeated B:

   free-opportunities_opportunity-free-without-checkpoints_OpenBookingApprovalFlow_FacilityUseSlot.md
3. nickevansuk commented on Jun 4, 2021

   

   nickevansuk

   on Jun 4, 2021

   Last edited by nickevansuk

   Contributor

   More actions

   Noting this could be solved by storing the hash of a successful request at B, along with the successful response, and then reading that hash if an Order already exists to determine whether to return the same response, or an OrderAlreadyExistsError

   And that it also would need some kind of AsyncLock to avoid two Orders with the same uuid being processed at the same time
4. nickevansuk commented on Jun 4, 2021

   

   nickevansuk

   on Jun 4, 2021

   Last edited by nickevansuk

   Contributor

   More actions

   Suggested design:

   1. Could be an additional idempotency hook “GetIdempontentOrderResponse” added to the OrderStore, that simply resolves the response string based on a ClientID, UUID, and request hash, and returns null otherwise (a specific hook will make it clear to implementers they need to do something for this?).
   2. Then CreateOrder could return false instead of OrderAlreadyExistsError
   3. Then the "GetIdempontentOrderResponse" gets called by the StoreBookingEngine if CreateOrder returns false, and either responds with the response string or a OrderAlreadyExistsError
   4. Also a new hook “CompleteOrder” that is called after “UpdateOrder” (before the transaction completes), accepting the same params as UpdateOrder with the additional params of “serialisedResponseOrder” and “requestIdempotencyHash”. This ensures the Order is not serialised twice. (Note the Order can be mutated in UpdateOrder, and the OrderItemIds are added after CreateOrder - hence suggestion to add a new hook at the end of the flow)

   Also worth noting that idempotency could be implemented using MemoryCache in the OrderStore, or another similar method, rather than storing the hash/response in the Order table - as the response probably only needs to be cached for e.g. 15 minutes. Perhaps the ref impl should use MemoryCache rather than storing this in the DB in its example, as it’s also cleaner from a GDPR compliance perspective (personal data in the response is not stored for longer than is needed, and not duplicated)?

   Or perhaps the above hooks could be be virtual rather than abstract and include a default MemoryCache implementation? Would make for a cleaner upgrade path for existing implementers too. If we did this potentially the default implementation should also compress responses (e.g.) to optimise memory usage.

   The above hooks would support both…

   Also add an AsyncLock or similar to ensure that two requests for the same UUID are not processed in parallel
5. matdavies commented on Jun 6, 2021

   

   matdavies

   on Jun 6, 2021

   Last edited by matdavies

   Contributor

   More actions

   I started trying to implement per the above design suggestions, but came across an issue:

   StoreBookingEngine.ProcessFlowRequest (which is where I think you're suggesting we make modifications) returns TOrder, not a string (or ResponseContent). So part 3 above is not really possible in that method? Instead if GetOrder returns false, we'd need to deserialise the response into a TOrder again, and return that?

   So instead I looked at modifying ProcessOrderCreationB in CustomBookingEngine, but of course, CustomBookingEngine does not have access to the OrderStore, so it can't call the GetIdempotentOrderResponse method from there. Instead IBookingEngine would need that GetIdempotentOrderResponse method on it (which, in the StoreBookingEngine, would just pass off to the OrderStore).

   Let me know which you would prefer, or if I've got the wrong end of the stick.

   Also, the above aside, I think CreateOrder should return this:

       /// <summary>
       /// Result of creating (or attempting to create) an Order in a store
       /// </summary>
       public enum CreateOrderResult
       {
           OrderSuccessfullyCreated,
           OrderAlreadyExists
       }

   instead of a bool, as the above matches DeleteOrder and makes it easier to see what to return (as opposed to having to guess that you need to return false from CreateOrder if the order already exists - otherwise people could easily just continue to throw exceptions there which is what the reference impl does)
6. nickevansuk commented on Jun 7, 2021

   

   nickevansuk

   on Jun 7, 2021

   Last edited by nickevansuk

   Contributor

   More actions

   Yes sounds good - CreateOrderResult is much clearer!

   Can see the advantages of doing this at the CustomBookingEngine level, as for one implementation pattern we're really adding a caching hook.

   The tradeoff is that this is more prescriptively suggesting that a separate caching layer is used, rather than e.g. using the database to store it within the same transaction. An alternative could be that ProcessFlowRequest returns a string, and responsibility of OpenActiveSerializer.Serialize(response) is moved into the StoreBookingEngine?
7. nickevansuk

   added this to OpenActive Developer Documentation and Tooling Supporton Jun 15, 2022
8. nickevansuk commented on Aug 3, 2023

   

   nickevansuk

   on Aug 3, 2023

   Contributor

   More actions

   After some thinking about the design, I've opted for doing this optionally at the CustomBookingEngine level (see #207), as it's cleaner and more closely follows the pattern of other libraries e.g. https://github.com/ikyriak/IdempotentAPI (which could be used if openactive/open-booking-api#239 is adopted in future).

   This does mean that it's not as easy to store a hash of the request in the database within CreateOrder, however it does result in a more maintainable solution overall.
9. nickevansuk

   closed this as completedin #207on Aug 17, 2023
10. github-project-automation

    moved this to Done in OpenActive Developer Documentation and Tooling Supporton Aug 17, 2023