Changes from gocardless/client-library-templates

gocardless_pro/services/bank_account_holder_verifications_service.py

@@ -42,8 +42,10 @@ def create(self,params=None, headers=None):
         except errors.IdempotentCreationConflictError as err:
           if self.raise_on_idempotency_conflict:
             raise err
+          # `params` is the create payload and is deliberately not forwarded: this is a
+          # GET for one resource, and requests would serialise the payload's keys into the
+          # query string.
           return self.get(identity=err.conflicting_resource_id,
-                          params=params,
                           headers=headers)
         return self._resource_for(response)
 

gocardless_pro/services/bank_authorisations_service.py

@@ -39,8 +39,10 @@ def create(self,params=None, headers=None):
         except errors.IdempotentCreationConflictError as err:
           if self.raise_on_idempotency_conflict:
             raise err
+          # `params` is the create payload and is deliberately not forwarded: this is a
+          # GET for one resource, and requests would serialise the payload's keys into the
+          # query string.
           return self.get(identity=err.conflicting_resource_id,
-                          params=params,
                           headers=headers)
         return self._resource_for(response)
 

gocardless_pro/services/base_service.py

@@ -54,7 +54,16 @@ def _attempt_request(self, method, path, params, headers):
 
 
     def _inject_idempotency_key(self, headers):
-        headers = headers or {}
+        """Return the caller's headers with an idempotency key added, without touching theirs.
+
+        Writing the key into the caller's own dict would leave it there after the call, so a
+        caller reusing one dict across several creates would send the first call's key every
+        time, and silently get back the resource the first call created.
+
+        The copy is taken once per call, before the retry loop, so a retried request still
+        carries the same key.
+        """
+        headers = dict(headers or {})
         if 'Idempotency-Key' not in headers:
             headers['Idempotency-Key'] = str(uuid4())
 

gocardless_pro/services/billing_request_templates_service.py

@@ -90,8 +90,10 @@ def create(self,params=None, headers=None):
         except errors.IdempotentCreationConflictError as err:
           if self.raise_on_idempotency_conflict:
             raise err
+          # `params` is the create payload and is deliberately not forwarded: this is a
+          # GET for one resource, and requests would serialise the payload's keys into the
+          # query string.
           return self.get(identity=err.conflicting_resource_id,
-                          params=params,
                           headers=headers)
         return self._resource_for(response)
 

gocardless_pro/services/billing_requests_service.py

@@ -40,8 +40,10 @@ def create(self,params=None, headers=None):
         except errors.IdempotentCreationConflictError as err:
           if self.raise_on_idempotency_conflict:
             raise err
+          # `params` is the create payload and is deliberately not forwarded: this is a
+          # GET for one resource, and requests would serialise the payload's keys into the
+          # query string.
           return self.get(identity=err.conflicting_resource_id,
-                          params=params,
                           headers=headers)
         return self._resource_for(response)
 

gocardless_pro/services/blocks_service.py

@@ -39,8 +39,10 @@ def create(self,params=None, headers=None):
         except errors.IdempotentCreationConflictError as err:
           if self.raise_on_idempotency_conflict:
             raise err
+          # `params` is the create payload and is deliberately not forwarded: this is a
+          # GET for one resource, and requests would serialise the payload's keys into the
+          # query string.
           return self.get(identity=err.conflicting_resource_id,
-                          params=params,
                           headers=headers)
         return self._resource_for(response)
 

gocardless_pro/services/creditor_bank_accounts_service.py

@@ -39,8 +39,10 @@ def create(self,params=None, headers=None):
         except errors.IdempotentCreationConflictError as err:
           if self.raise_on_idempotency_conflict:
             raise err
+          # `params` is the create payload and is deliberately not forwarded: this is a
+          # GET for one resource, and requests would serialise the payload's keys into the
+          # query string.
           return self.get(identity=err.conflicting_resource_id,
-                          params=params,
                           headers=headers)
         return self._resource_for(response)
 

gocardless_pro/services/creditors_service.py

@@ -39,8 +39,10 @@ def create(self,params=None, headers=None):
         except errors.IdempotentCreationConflictError as err:
           if self.raise_on_idempotency_conflict:
             raise err
+          # `params` is the create payload and is deliberately not forwarded: this is a
+          # GET for one resource, and requests would serialise the payload's keys into the
+          # query string.
           return self.get(identity=err.conflicting_resource_id,
-                          params=params,
                           headers=headers)
         return self._resource_for(response)
 

gocardless_pro/services/customer_bank_accounts_service.py

@@ -54,8 +54,10 @@ def create(self,params=None, headers=None):
         except errors.IdempotentCreationConflictError as err:
           if self.raise_on_idempotency_conflict:
             raise err
+          # `params` is the create payload and is deliberately not forwarded: this is a
+          # GET for one resource, and requests would serialise the payload's keys into the
+          # query string.
           return self.get(identity=err.conflicting_resource_id,
-                          params=params,
                           headers=headers)
         return self._resource_for(response)
 

gocardless_pro/services/customers_service.py

@@ -39,8 +39,10 @@ def create(self,params=None, headers=None):
         except errors.IdempotentCreationConflictError as err:
           if self.raise_on_idempotency_conflict:
             raise err
+          # `params` is the create payload and is deliberately not forwarded: this is a
+          # GET for one resource, and requests would serialise the payload's keys into the
+          # query string.
           return self.get(identity=err.conflicting_resource_id,
-                          params=params,
                           headers=headers)
         return self._resource_for(response)
 

gocardless_pro/services/instalment_schedules_service.py

@@ -58,8 +58,10 @@ def create_with_dates(self,params=None, headers=None):
         except errors.IdempotentCreationConflictError as err:
           if self.raise_on_idempotency_conflict:
             raise err
+          # `params` is the create payload and is deliberately not forwarded: this is a
+          # GET for one resource, and requests would serialise the payload's keys into the
+          # query string.
           return self.get(identity=err.conflicting_resource_id,
-                          params=params,
                           headers=headers)
         return self._resource_for(response)
 
@@ -102,8 +104,10 @@ def create_with_schedule(self,params=None, headers=None):
         except errors.IdempotentCreationConflictError as err:
           if self.raise_on_idempotency_conflict:
             raise err
+          # `params` is the create payload and is deliberately not forwarded: this is a
+          # GET for one resource, and requests would serialise the payload's keys into the
+          # query string.
           return self.get(identity=err.conflicting_resource_id,
-                          params=params,
                           headers=headers)
         return self._resource_for(response)
 

gocardless_pro/services/mandate_imports_service.py

@@ -46,8 +46,10 @@ def create(self,params=None, headers=None):
         except errors.IdempotentCreationConflictError as err:
           if self.raise_on_idempotency_conflict:
             raise err
+          # `params` is the create payload and is deliberately not forwarded: this is a
+          # GET for one resource, and requests would serialise the payload's keys into the
+          # query string.
           return self.get(identity=err.conflicting_resource_id,
-                          params=params,
                           headers=headers)
         return self._resource_for(response)
 

gocardless_pro/services/mandates_service.py

@@ -39,8 +39,10 @@ def create(self,params=None, headers=None):
         except errors.IdempotentCreationConflictError as err:
           if self.raise_on_idempotency_conflict:
             raise err
+          # `params` is the create payload and is deliberately not forwarded: this is a
+          # GET for one resource, and requests would serialise the payload's keys into the
+          # query string.
           return self.get(identity=err.conflicting_resource_id,
-                          params=params,
                           headers=headers)
         return self._resource_for(response)
 

gocardless_pro/services/outbound_payment_imports_service.py

@@ -39,8 +39,10 @@ def create(self,params=None, headers=None):
         except errors.IdempotentCreationConflictError as err:
           if self.raise_on_idempotency_conflict:
             raise err
+          # `params` is the create payload and is deliberately not forwarded: this is a
+          # GET for one resource, and requests would serialise the payload's keys into the
+          # query string.
           return self.get(identity=err.conflicting_resource_id,
-                          params=params,
                           headers=headers)
         return self._resource_for(response)
 

gocardless_pro/services/outbound_payments_service.py

@@ -39,8 +39,10 @@ def create(self,params=None, headers=None):
         except errors.IdempotentCreationConflictError as err:
           if self.raise_on_idempotency_conflict:
             raise err
+          # `params` is the create payload and is deliberately not forwarded: this is a
+          # GET for one resource, and requests would serialise the payload's keys into the
+          # query string.
           return self.get(identity=err.conflicting_resource_id,
-                          params=params,
                           headers=headers)
         return self._resource_for(response)
 

gocardless_pro/services/payer_authorisations_service.py

@@ -68,8 +68,10 @@ def create(self,params=None, headers=None):
         except errors.IdempotentCreationConflictError as err:
           if self.raise_on_idempotency_conflict:
             raise err
+          # `params` is the create payload and is deliberately not forwarded: this is a
+          # GET for one resource, and requests would serialise the payload's keys into the
+          # query string.
           return self.get(identity=err.conflicting_resource_id,
-                          params=params,
                           headers=headers)
         return self._resource_for(response)
 

gocardless_pro/services/payments_service.py

@@ -45,8 +45,10 @@ def create(self,params=None, headers=None):
         except errors.IdempotentCreationConflictError as err:
           if self.raise_on_idempotency_conflict:
             raise err
+          # `params` is the create payload and is deliberately not forwarded: this is a
+          # GET for one resource, and requests would serialise the payload's keys into the
+          # query string.
           return self.get(identity=err.conflicting_resource_id,
-                          params=params,
                           headers=headers)
         return self._resource_for(response)
 

gocardless_pro/services/redirect_flows_service.py

@@ -40,8 +40,10 @@ def create(self,params=None, headers=None):
         except errors.IdempotentCreationConflictError as err:
           if self.raise_on_idempotency_conflict:
             raise err
+          # `params` is the create payload and is deliberately not forwarded: this is a
+          # GET for one resource, and requests would serialise the payload's keys into the
+          # query string.
           return self.get(identity=err.conflicting_resource_id,
-                          params=params,
                           headers=headers)
         return self._resource_for(response)
 

gocardless_pro/services/refunds_service.py

@@ -51,8 +51,10 @@ def create(self,params=None, headers=None):
         except errors.IdempotentCreationConflictError as err:
           if self.raise_on_idempotency_conflict:
             raise err
+          # `params` is the create payload and is deliberately not forwarded: this is a
+          # GET for one resource, and requests would serialise the payload's keys into the
+          # query string.
           return self.get(identity=err.conflicting_resource_id,
-                          params=params,
                           headers=headers)
         return self._resource_for(response)
 

gocardless_pro/services/scheme_identifiers_service.py

@@ -77,8 +77,10 @@ def create(self,params=None, headers=None):
         except errors.IdempotentCreationConflictError as err:
           if self.raise_on_idempotency_conflict:
             raise err
+          # `params` is the create payload and is deliberately not forwarded: this is a
+          # GET for one resource, and requests would serialise the payload's keys into the
+          # query string.
           return self.get(identity=err.conflicting_resource_id,
-                          params=params,
                           headers=headers)
         return self._resource_for(response)
 

gocardless_pro/services/subscriptions_service.py

@@ -39,8 +39,10 @@ def create(self,params=None, headers=None):
         except errors.IdempotentCreationConflictError as err:
           if self.raise_on_idempotency_conflict:
             raise err
+          # `params` is the create payload and is deliberately not forwarded: this is a
+          # GET for one resource, and requests would serialise the payload's keys into the
+          # query string.
           return self.get(identity=err.conflicting_resource_id,
-                          params=params,
                           headers=headers)
         return self._resource_for(response)
 

tests/idempotency_test.py

@@ -0,0 +1,103 @@
+# WARNING: Do not edit by hand, this file was generated by Crank:
+#
+#   https://github.com/gocardless/crank
+#
+
+import json
+
+import pytest
+import requests
+import responses
+
+from gocardless_pro import Client
+from gocardless_pro import errors
+from gocardless_pro.services.base_service import BaseService
+
+access_token = 'access-token-xyz'
+
+
+@pytest.fixture
+def client():
+    return Client(access_token=access_token, base_url='http://example.com')
+
+
+def idempotency_keys():
+    return [call.request.headers['Idempotency-Key'] for call in responses.calls
+            if call.request.method == 'POST']
+
+
+def test_does_not_mutate_the_callers_headers():
+    # The dict belongs to the caller. Leaving the generated key in it would make every later
+    # create reuse that key.
+    headers = {'Accept-Language': 'fr'}
+
+    returned = BaseService(api_client=None)._inject_idempotency_key(headers)
+
+    assert returned is not headers
+    assert headers == {'Accept-Language': 'fr'}
+    assert 'Idempotency-Key' in returned
+
+
+def test_honours_an_explicit_idempotency_key():
+    headers = {'Idempotency-Key': 'mine'}
+
+    returned = BaseService(api_client=None)._inject_idempotency_key(headers)
+
+    assert returned['Idempotency-Key'] == 'mine'
+    assert headers == {'Idempotency-Key': 'mine'}
+
+
+@responses.activate
+def test_reused_headers_dict_gets_a_fresh_key_for_each_create(client):
+    # Two independent creates sharing one non-empty headers dict, which is what a
+    # module-level custom-header template looks like.
+    responses.add(responses.POST, 'http://example.com/customers',
+                  body=json.dumps({'customers': {'id': 'CU123'}}), status=201)
+    responses.add(responses.POST, 'http://example.com/customers',
+                  body=json.dumps({'customers': {'id': 'CU456'}}), status=201)
+    shared_headers = {'Accept-Language': 'fr'}
+
+    client.customers.create(params={'company_name': 'A'}, headers=shared_headers)
+    client.customers.create(params={'company_name': 'B'}, headers=shared_headers)
+
+    first, second = idempotency_keys()
+    assert first != second
+    assert shared_headers == {'Accept-Language': 'fr'}
+
+
+@responses.activate
+def test_key_is_stable_across_network_retries(client):
+    # The key is generated once per call, so the API can recognise a retried request as the
+    # same operation rather than a new one.
+    responses.add(responses.POST, 'http://example.com/customers',
+                  body=requests.exceptions.ConnectionError())
+    responses.add(responses.POST, 'http://example.com/customers',
+                  body=json.dumps({'customers': {'id': 'CU123'}}), status=201)
+
+    client.customers.create(params={'company_name': 'A'},
+                            headers={'Accept-Language': 'fr'})
+
+    keys = idempotency_keys()
+    assert len(keys) == 2
+    assert keys[0] == keys[1]
+
+
+@responses.activate
+def test_conflict_refetch_does_not_forward_the_create_payload(client):
+    # On a 409 the client refetches the conflicting resource. That is a GET for one resource,
+    # so the create payload must not be carried into its query string.
+    conflict = {'error': {'type': 'invalid_state', 'code': 409, 'message': 'Conflict',
+                          'errors': [{'reason': 'idempotent_creation_conflict',
+                                      'message': 'A resource has already been created',
+                                      'links': {'conflicting_resource_id': 'CU123'}}]}}
+    responses.add(responses.POST, 'http://example.com/customers',
+                  body=json.dumps(conflict), status=409)
+    responses.add(responses.GET, 'http://example.com/customers/CU123',
+                  body=json.dumps({'customers': {'id': 'CU123'}}), status=200)
+
+    customer = client.customers.create(params={'company_name': 'A', 'metadata': {'x': 'y'}},
+                                       headers={'Accept-Language': 'fr'})
+
+    assert customer.id == 'CU123'
+    refetch = [call.request for call in responses.calls if call.request.method == 'GET'][0]
+    assert refetch.url == 'http://example.com/customers/CU123'