search

GSMA-CPAS / BWRP-common-adapter

The "Layer 2.5" with all Common functionality APIs
Apache License 2.0
2 stars 0 forks source link

Webhook callback handling / use of eventStorageKey #20

Open sschulz-t opened 3 years ago

sschulz-t commented 3 years ago

Sebastian found a log entry in the blockchain adapter regarding a wrong http response code.

The common adapter returns 200 (OK) and the blockchain adapter expects 202 (Accepted). Returning 202 means that the request was accepted and will be processed, the processing might not be done yet. This is how it was originally designed and implemented in the webhook server demo code.

I was wondering how the common adapter handles the callback. It seems like the common adapter is blocking until all processing is done and correctly returns a 200 afterwards. I will add this to the blockchain adapter as a valid response code (see https://github.com/GSMA-CPAS/BWRP-blockchain-adapter/issues/17)

When researching the common adapter behavior i stumbled upon this code: https://github.com/GSMA-CPAS/BWRP-common-adapter/blob/9cd7b40da09e530efd227d904e5e0d5a937b5254/server/services/EventService.js#L102

As far as I understand, a webhook callback triggers the following code, passing the storage key of the event to a helper function: https://github.com/GSMA-CPAS/BWRP-common-adapter/blob/9cd7b40da09e530efd227d904e5e0d5a937b5254/server/services/EventService.js#L105

But this function https://github.com/GSMA-CPAS/BWRP-common-adapter/blob/9cd7b40da09e530efd227d904e5e0d5a937b5254/server/services/EventService.js#L48 does not use the parameter eventStorageKey at all.

So the event callback will copy and delete all pending documents and not the one specific to this event.

Fetching and deleting all documents, regardless of the eventStorageKey, could make sense e.g. in case we missed an event during a server reboot etc. However this behavior should be clearly documented and it needs further testing what happens if two webhook callbacks arrive at the same time (or with a tiny delay). If this behavior is intended, I would suggest to remove eventStorageKey as a parameter to getBlockchainReferenceIds.

csarthou commented 3 years ago

For the returned 200 response, like you said, "the common adapter is blocking until all processing is done". But I think that a 202 response, with a not blocking processing is a better implementation response for this API endpoint.

For the "getBlockchainReferenceIds" function, the idea is that it could exist a solution to find the documents referenceIds form storageKeys. Today, this solution is not defined, and we download all the available documents referenceIds, but this function implementation could be updated in next releases. The comment of this function explains this goal, I think: "Define the list of blockchain reference Ids to download from the storageKey of a received event"

For the case where 2 webhook callbacks arrive as the same time, this contracts to store in the mongoDB contains an unique referenceId index. So only the first insert will be allowed, and the second will be rejected.