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.

Sign up for GitHub

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

Jump to bottom

Add idempotency to Stripe Create Calls #1909

Open
wants to merge 33 commits into
base: master
Choose a base branch
from
Open

Add idempotency to Stripe Create Calls #1909

wants to merge 33 commits into from

Conversation

arnav13081994
Copy link
Collaborator

This PR adds support for the user to:

  1. Pass in their own idempotency keys to Stripe Create calls.
  2. If the user does not pass their own idempotency keys, djstripe generates and uses its own.

jleclanche
jleclanche requested changes Mar 27, 2023
View reviewed changes
Copy link
Member

@jleclanche jleclanche left a comment

Choose a reason for hiding this comment

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

Why are you storing the IK in the metadata everywhere?

@arnav13081994
Copy link
Collaborator Author

In case the user wants to retry the request for whatever reason. And this way the information can be accessed from the dashboard too in case the local db gets nuked.

And please note I am storing the last generated key only.

@arnav13081994 arnav13081994 mentioned this pull request Mar 27, 2023
Open
@jleclanche
Copy link
Member

We have a metadata store already for this: We have a djstripe idempotency key model. You should be atomically checking that model when you make the call. See how it's done for the user/customer creation.

@arnav13081994
Copy link
Collaborator Author

The current implementation of Idempotency model only works for customer.create(user) model. That's it. It cannot apply to generic api calls. And Idempotency data is exclusive to dj-stripe and is not stored on Stripe which can lead to inconsistencies in cases where the local db gets removed and the user would like to re create it from upstream Stripe. This way all cases are taken care of.

@jleclanche
Copy link
Member

The current implementation of Idempotency model only works for customer.create(user) model. That's it. It cannot apply to generic api calls. And Idempotency data is exclusive to dj-stripe and is not stored on Stripe which can lead to inconsistencies in cases where the local db gets removed and the user would like to re create it from upstream Stripe. This way all cases are taken care of.

The model of IdempotencyKey was created to store any idempotency key. The tag attached to it allows a structure which creates a unique key for a unique call.

If the model is inappropriate, please suggest changes to the model. The PR as-is is unmergeable.

@arnav13081994
Copy link
Collaborator Author

The current implementation of Idempotency model only works for customer.create(user) model. That's it. It cannot apply to generic api calls. And Idempotency data is exclusive to dj-stripe and is not stored on Stripe which can lead to inconsistencies in cases where the local db gets removed and the user would like to re create it from upstream Stripe. This way all cases are taken care of.

The model of IdempotencyKey was created to store any idempotency key. The tag attached to it allows a structure which creates a unique key for a unique call.

If the model is inappropriate, please suggest changes to the model. The PR as-is is unmergeable.

I think I may have misunderstood something because I am still not sure what all makes this PR unmergeable. I'd appreciate if you could list all the opportunities you see to improve upon this PR.

@jleclanche
Copy link
Member

@arnav13081994 To merge this PR, please use the IdempotencyKey model similarly to how it's used in the customer creation operation. Don't track idempotency keys outside of the db.

The only other place we could track them is the cache but it's not a good idea to change this at this point, and I absolutely don't want to introduce a separate way of tracking them when we already have a model.

@arnav13081994 arnav13081994 self-assigned this Apr 19, 2023
arnav13081994 added a commit that referenced this pull request Apr 20, 2023
@arnav13081994
Changes from PR #1909
fb8750b
@arnav13081994 arnav13081994 mentioned this pull request Apr 20, 2023
Open
@arnav13081994 @jleclanche
Added SourceTransaction model.
afe87b0
@arnav13081994 @jleclanche
Added SourceTransaction.__str__() method
18f2bfe
@arnav13081994 @jleclanche
Added SourceTransaction.get_stripe_dashboard_url() method
2215029 
This was done because Stripe does not expose a separate url to access these objects and they can only be accessed with the Related Source Object
@arnav13081994 @jleclanche
Exposed SourceTransaction Model on the Admin
0a05fea
@arnav13081994 @jleclanche
Added SourceTransaction.api_list() method
eae5870
@arnav13081994 @jleclanche
Added SourceTransaction.api_retrieve() method
8e8e6da
@arnav13081994 @jleclanche
SourceTransaction instances will now get synced by the djstripe_sync_…
2f98077 
…models command
@arnav13081994 @jleclanche
Refactored Source object event handler into its own independent handler
8dd521a 
This was done because the "Source" event handler was receiving all events of the type source.X.Y which was causing a mismatch between the event type and the target object type. This is why the target object type was explicitly checked as done for the "Charge" event handler.
@arnav13081994 @jleclanche
Added a webhook handler to handle all source.transaction.Y events
869d3c4
@arnav13081994 @jleclanche
Updated _handle_crud_like_event for SourceTransaction Objects
ffc3f5c 
This was done because newly created Sourcetransaction objects cannot be retrieved as Stripe doesn't allow `retrieve` operation yet. And in order to retrieve by the related "Source" object we need the source to be attached to a "Customer" instance which will not always be the case and can't be known ahead of time as the order of webhooks is unreliable. This is why retrieving the data of the SourceTransaction object was skipped.
@arnav13081994 @jleclanche
Fix Linting Errors
9372d6f
@arnav13081994 @jleclanche
Updated Changelog
c89f6fc
@arnav13081994 @jleclanche
Update migration file name
bab69e7
@arnav13081994
Fixed incorrect migration dependency name
493dd0a
arnav13081994 added a commit that referenced this pull request Apr 20, 2023
@arnav13081994
Changes from PR #1909
ecabad8
@arnav13081994
Update 0021_sourcetransaction migration name to 0022_sourcetransaction
8ba678e
@arnav13081994
Remove incorrect migration
71e35ad
@arnav13081994
Set fallback value of metadata to {} instead of None
5c56dca
@arnav13081994
Updated StripeModel._api_create() to accept idempotency_key param
10012c7
@arnav13081994
Updated Charge.refund() to accept idempotency_key param
9800773
@arnav13081994
Updated Customer.add_invoice_item() to accept idempotency_key param
539ed88
@arnav13081994
Updated Customer.send_invoice() to accept idempotency_key param
b1e176b
@arnav13081994
Updated the get_idempotency_key function
e4a8ba8 
The function whenever called always creates a new idempotency key.
This was done because the older implementation was only
suitable for Customer.create() method as was not generic to
handle all `create` or `update` calls.
@arnav13081994
Updated Customer.purge() method to not delete the associated idempote…
94ee2dd 
…ncy_key

This was done because in the updated flow we ALWAYS generate a new
idempotency_key for a request if one is not provided. The old
flow was only applicable for Customer creation with a given user
as a subscriber. Besides re-using these keys are pointless after
a period of 24 hours anyway.
@arnav13081994
Renamed get_idempotency_key to create_idempotency_key
658df74 
This was done to better reflect the functionality of the
function create_idempotency_key.
@arnav13081994
Added Idempotencykey.update_action_field staticmethod
ba8020c 
This is a helper method to update the action model field
post object creation with the stripe object's id to ease
reference of the idempotency_key used to create or update
the stripe object.
@arnav13081994
Copy link
Collaborator Author

@jleclanche Updated the PR. Please review again.

arnav13081994 added a commit that referenced this pull request Apr 20, 2023
@arnav13081994
Changes from PR #1909
bcdaf30
@arnav13081994
Added StripeModel.get_or_create_idempotency_key
67dee8d 
This was done in order to populate the
idempotency_key and use it to make the call on
behalf of the user in case they do
not provide us with one.
@arnav13081994
Updated StripeModel._api_create to use Idempotency Key object
56e4565 
The _api_create method now does the following:

1) Generated a new Idempotency Key object if not given one.
2) Uses the Idempotency key (from 1)) to make the Stripe API call.
3) Updates the action model field of the Idempotency Key object with
   the id of the so created Stripe object.
@arnav13081994
Updated TaxId._api_create() to use Idempotency Keys
01d95c1
@arnav13081994
Updated TransferReversal._api_create() to use Idempotency Keys
832fd36
@arnav13081994
Updated UsageRecord._api_create() to use Idempotency Keys
d2a8672
@arnav13081994
Updated LegacySourceMixin._api_create() to use Idempotency Keys
e8e278c
@arnav13081994
Updated docs with support for idempotency_keys
0308c33
@jleclanche jleclanche force-pushed the master branch 5 times, most recently from 15e04c3 to 8727894 Compare April 25, 2024 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jleclanche jleclanche Awaiting requested review from jleclanche

Assignees

@arnav13081994 arnav13081994

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@arnav13081994 @jleclanche