Don't return cached errors, allow to retry#36
Don't return cached errors, allow to retry#36unicoder88 wants to merge 3 commits intomahendraHegde:mainfrom
Conversation
packages/core/src/idempotency.ts
Outdated
| IdempotencyErrorCodes.IDEMPOTENCY_FINGERPRINT_MISSMATCH, | ||
| ); | ||
| } | ||
| if (data.response?.error) { |
There was a problem hiding this comment.
Nice work—thanks for the PR!
This change isn’t backward-compatible as-is. Could we add a new configuration option to control whether errors get cached? If unset, it would default to the existing behavior.
When the new config is enabled, it makes sense not to cache the error itself. In that case, we should clear the cache key and allow subsequent requests to retry—this aligns with the intended behavior.
There was a problem hiding this comment.
I've added config and ability to delete cache. Let's see what else can be improved :)
unicoder88
force-pushed
the
feat/retry-errors
branch
from
5bf4dcb to
d69b799
Compare
mahendraHegde
left a comment
mahendraHegde
left a comment
There was a problem hiding this comment.
Thanks for the changes, added few comments
| return val ?? undefined; | ||
| } | ||
|
|
||
| async delete(key: string): Promise<void> { |
There was a problem hiding this comment.
this needs to go inside postgres adapter too, if you are not familiar with postgres, no worries i can take care of it too.
There was a problem hiding this comment.
Good thing is that Postgres adapter already has this :)
https://github.com/mahendraHegde/node-idempotency/blob/main/packages/storage-adapter-postgres/src/adapter-postgres.ts#L108
| return response; | ||
| }), | ||
| catchError((err) => { | ||
| if (idempotencyReq.options?.skipErrorsCache) { |
There was a problem hiding this comment.
why handle it only for nestJS? why not add this logic inside the core(packages/core/src/idempotency.ts)->onResponse so that every adapter will get the feature out of the box?
There was a problem hiding this comment.
Great point, and code get so much simpler. Updated, please review.
|
|
||
| if (res.error && req.options?.skipErrorsCache) { | ||
| // do not cache the error itself, clear the cache key and allow subsequent requests to retry | ||
| await this.storage.delete(cacheKey); |
There was a problem hiding this comment.
did you miss a return statement here? because the set below will set the response anyway.
https://github.com/mahendraHegde/node-idempotency/pull/36/files#diff-3c306f162d9ea758a7d4323df8bfe9476e1a263a7138ea8e22e531e30152b8fdR177-R184
also would you mind adding some tests like we have for other cases so that we could catch bugs like this early(I'd appreciate it if you could add tests for adapters too, if not i can add them too)?
2 participants
Hi!
Thanks for awesome module 👍 When doing tests I bumped into an issue, the module would cache this error.
Examples:
Even after error reason is gone (database is up, source code updated), this module will keep returning old cached error without ability to fix this.
Please review :)