Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add idempotency interceptor #4531

Merged
merged 17 commits into from
Oct 27, 2023
Merged

Conversation

mahendraHegde
Copy link
Contributor

@mahendraHegde mahendraHegde commented Oct 15, 2023

What change does this PR introduce?

  • Adds Idempotency interceptor to provide idempotency to POST and PATCH requests
  • Updates Docs
  • Adds tests

Reopens #4472

Why was this change needed?

closes #4469

Other information (Screenshots)

New request
Screenshot 2023-10-25 at 11 29 35 PM

Replayed request
Screenshot 2023-10-25 at 11 29 44 PM

@Cliftonz
Copy link
Contributor

@mahendraHegde We are seeing the following error in the pipeline

 ERROR in /home/runner/work/novu/novu/apps/api/src/app/shared/framework/idempotency.interceptor.ts
16:28-50
[tsl] ERROR in /home/runner/work/novu/novu/apps/api/src/app/shared/framework/idempotency.interceptor.ts(16,29)
      TS2307: Cannot find module 'libs/shared/dist/cjs' or its corresponding type declarations.
      ``` 
      Would you know what this means? if not I can ask a team member.

@mahendraHegde
Copy link
Contributor Author

@mahendraHegde We are seeing the following error in the pipeline

 ERROR in /home/runner/work/novu/novu/apps/api/src/app/shared/framework/idempotency.interceptor.ts
16:28-50
[tsl] ERROR in /home/runner/work/novu/novu/apps/api/src/app/shared/framework/idempotency.interceptor.ts(16,29)
      TS2307: Cannot find module 'libs/shared/dist/cjs' or its corresponding type declarations.
      ``` 
      Would you know what this means? if not I can ask a team member.

@Cliftonz i think its a mistake of auto import, let me fix this

@mahendraHegde mahendraHegde force-pushed the idempotency-impl branch 2 times, most recently from f4bf5fb to 8d253d7 Compare October 16, 2023 14:08
@mahendraHegde
Copy link
Contributor Author

@mahendraHegde We are seeing the following error in the pipeline

 ERROR in /home/runner/work/novu/novu/apps/api/src/app/shared/framework/idempotency.interceptor.ts
16:28-50
[tsl] ERROR in /home/runner/work/novu/novu/apps/api/src/app/shared/framework/idempotency.interceptor.ts(16,29)
      TS2307: Cannot find module 'libs/shared/dist/cjs' or its corresponding type declarations.
      ``` 
      Would you know what this means? if not I can ask a team member.

@Cliftonz i think its a mistake of auto import, let me fix this

@Cliftonz can you try now?

.cspell.json Outdated Show resolved Hide resolved
apps/api/src/.env.production Outdated Show resolved Hide resolved
apps/api/e2e/idempotency.e2e.ts Outdated Show resolved Hide resolved
@rifont
Copy link
Contributor

rifont commented Oct 17, 2023

Hi @mahendraHegde, thanks for the great work on this PR already.

@Cliftonz and I have discussed a few additions to the idempotency implementation which we think will deliver significant short and long-term benefits to users of this feature, using Stripe API as a guide for these additional implementation details. These are changes that will be potentially breaking to add later, should a client begin to depend on the existing implementation detail.

These additions are summarised below, with rationale in brackets.

  • Return 503 Service unavailable when a request includes an Idempotency-Key header and the request is not processable (this avoids a situation where a client expecting idempotency has their request processed without it, which is potentially dangerous and reputationally damaging for the client in the event of duplicate triggers, for example)
  • Remove the error attribute from the response body entirely (the Novu team will seek to formalize a response error format specification before we implement additional attributes. This may involve either response header or body attributes. It would be great to get your input here)
  • Add an Idempotency-Replayed: true header to the request in the event of a cache hit of the idempotency feature (this is potentially valuable information for the client to know when they produce an idempotency cache hit, we should make this information available in the interest of transparency)
  • Return 409 Conflict when a response is ongoing with the same Idempotency-Key and request body (we are aligning here with Stripe's API and a number of other leading API implementations, choosing to follow the HTTP specification and acknowledging this will result in an additional error code to handle in client implementations).
  • Modify the idempotency cache TTL from 6 hours to 24 hours (this is the de facto standard across leading API implementations, and importantly allows server hosts more time to resolve production incidents affecting idempotent requests)

If you could please include these updates in the PR alongside the other suggestions, we can move this through to include in the next release 😃

@mahendraHegde
Copy link
Contributor Author

@rifont i have fixed comments and added few comments, let me know your thoughts

@rifont
Copy link
Contributor

rifont commented Oct 24, 2023

Hey @mahendraHegde, I've added a few last comments on the PR.

Additionally, could you please test with your own API key via curl/Postman to verify the following scenarios and post screenshots in the PR description, as we had issues in practice with API key auth in the previous merge of the PR:

  1. Request with API Key auth and a noted Idempotency-Key to POST /events/trigger results in 201 Created
  2. Request with API Key auth and the same noted Idempotency-Key to POST /events/trigger results in 201 Created with Idempotency-Replated true.

Lastly, we need to update the Idempotency docs PR (novuhq/docs#202) to reflect new behavior.

Thanks again for your great work on this critical reliability feature. We're nearly there 🙌

Copy link
Contributor

@LetItRock LetItRock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @mahendraHegde 👋 thanks a lot for your contribution to Novu 🙌
I hope that I'm not too late for the party here 😄 left you a few suggestions 😉

apps/api/e2e/idempotency.e2e.ts Outdated Show resolved Hide resolved
apps/api/e2e/idempotency.e2e.ts Outdated Show resolved Hide resolved
apps/api/e2e/idempotency.e2e.ts Outdated Show resolved Hide resolved
@mahendraHegde
Copy link
Contributor Author

@Cliftonz @rifont @LetItRock I believe i have addressed all review comments let me know if i missed any.

@Cliftonz
Copy link
Contributor

@rifont The pipeline is failing due to bad container build all of the unit test have passed for this.
If all the requirements are met for this pr and you have manually tested the functionality, I am good with merging this.

@rifont
Copy link
Contributor

rifont commented Oct 26, 2023

I have verified the feature locally:

Testing Snippet

Initial Request with key 12341

>
curl -v -X POST http://localhost:3000/v1/events/trigger \
  -H "Authorization: ApiKey XXXXXXXXXXXXXXXXXX" \
  -H "Idempotency-Key: 12341" \
  -H "Content-Type: application/json" \
  -d '{
    "name": "sample",
    "to": {
      "subscriberId": "XXXXXXXXXXXXXXXXXX",
      "email": "john@doemail.com",
      "firstName": "John",
      "lastName": "Doe"
    },
    "payload": {
      "name": "Hello World",
      "organization": {
        "logo": "https://happycorp.com/logo.png"
      }
    }
  }'
...
> Authorization: ApiKey XXXXXXXXXXXXXXXXXX
...
< HTTP/1.1 201 Created
...
< Idempotency-Key: 12341
...
{"data":{"acknowledged":true,"status":"processed","transactionId":"297b420f-8685-4545-b055-bea3e81e8841"}}%

Replayed Request with key 12341, Idempotency-Replay header present

>
curl -v -X POST http://localhost:3000/v1/events/trigger \
  -H "Authorization: ApiKey XXXXXXXXXXXXXXXXXX" \
  -H "Idempotency-Key: 12341" \
  -H "Content-Type: application/json" \
  -d '{
    "name": "sample",
    "to": {
      "subscriberId": "XXXXXXXXXXXXXXXXXX",
      "email": "john@doemail.com",
      "firstName": "John",
      "lastName": "Doe"
    },
    "payload": {
      "name": "Hello World",
      "organization": {
        "logo": "https://happycorp.com/logo.png"
      }
    }
  }'
...
> Authorization: ApiKey XXXXXXXXXXXXXXXXXX
> Idempotency-Key: 12341
...
< HTTP/1.1 201 Created
...
< Idempotency-Key: 12341
< Idempotency-Replay: true
...
{"data":{"acknowledged":true,"status":"processed","transactionId":"297b420f-8685-4545-b055-bea3e81e8841"}}%

Reused key 12341, different body - 422 Unprocessable Entity

>
curl -v -X POST http://localhost:3000/v1/events/trigger \
  -H "Authorization: ApiKey XXXXXXXXXXXXXXXXXX" \
  -H "Idempotency-Key: 12341" \
  -H "Content-Type: application/json" \
  -d '{
    "name": "sample",
    "to": {
      "subscriberId": "XXXXXXXXXXXXXXXXXX",
      "email": "john@doesmail.com",
      "firstName": "John",
      "lastName": "Doe"
    },
    "payload": {
      "name": "Hello World",
      "organization": {
        "logo": "https://happycorp.com/logo.png"
      }
    }
  }'
...
> Authorization: ApiKey XXXXXXXXXXXXXXXXXX
> Idempotency-Key: 12341
...
< HTTP/1.1 422 Unprocessable Entity
...
< Idempotency-Key: 12341
< Link: docs.novu.co/idempotency
{"message":"Request with key \"12341\" is being reused for a different body","error":"Unprocessable Entity","statusCode":422}%

Initial Request with key 12345 resulting in 400 Bad Request

>
curl -v -X POST http://localhost:3000/v1/events/trigger \
  -H "Authorization: ApiKey XXXXXXXXXXXXXXXXXX" \
  -H "Idempotency-Key: 12345" \
  -H "Content-Type: application/json" \
  -d '{
    "to": {
      "subscriberId": "XXXXXXXXXXXXXXXXXX",
      "email": "john@doesmail.com",
      "firstName": "John",
      "lastName": "Doe"
    },
    "payload": {
      "name": "Hello World",
      "organization": {
        "logo": "https://happycorp.com/logo.png"
      }
    }
  }'
...
> Authorization: ApiKey XXXXXXXXXXXXXXXXXX
> Idempotency-Key: 12345
...
< HTTP/1.1 400 Bad Request
...
< Idempotency-Key: 12345
...
{"message":["name should not be null or undefined","name must be a string"],"error":"Bad Request","statusCode":400}%

Replayed 400 Bad Request with key 12345, Idempotency-Replay header present

>
curl -v -X POST http://localhost:3000/v1/events/trigger \
  -H "Authorization: ApiKey XXXXXXXXXXXXXXXXXX" \
  -H "Idempotency-Key: 12345" \
  -H "Content-Type: application/json" \
  -d '{
    "to": {
      "subscriberId": "XXXXXXXXXXXXXXXXXX",
      "email": "john@doesmail.com",
      "firstName": "John",
      "lastName": "Doe"
    },
    "payload": {
      "name": "Hello World",
      "organization": {
        "logo": "https://happycorp.com/logo.png"
      }
    }
  }'
...
> Authorization: ApiKey XXXXXXXXXXXXXXXXXX
> Idempotency-Key: 12345
.
< HTTP/1.1 400 Bad Request
...
< Idempotency-Key: 12345
< Idempotency-Replay: true
...
{"message":["name should not be null or undefined","name must be a string"],"error":"Bad Request","statusCode":400}%

Superb work on this feature @mahendraHegde ! Thank you for this important contribution.

@LetItRock could you please take a look at the requested changes?

@LetItRock
Copy link
Contributor

@mahendraHegde Looks good to me. Excellent job! Thank you! 🙌

@LetItRock LetItRock merged commit 460708d into novuhq:next Oct 27, 2023
24 of 27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[NV-3015] Idempotent API Requests Implementation
7 participants