Mangopay / mangopay2-ruby-sdk

Ruby Gem for MANGOPAY
https://rubygems.org/gems/mangopay
MIT License
42 stars 38 forks source link

Idempotency support does not work for Direct Pay In #44

Closed javiercr closed 8 years ago

javiercr commented 8 years ago

With both the latest version of the gem from Rubygems, and the latest version available in the master branch, this call to create a Direct Pay In does not add the Idempotency-Key http header to the request.

MangoPay::PayIn::Card::Direct.create({
  AuthorId:            author_id,
  CreditedUserId:      account_id,
  CreditedWalletId:    wallet_id,
  DebitedFunds:        { Currency: curreny, Amount: amount },
  Fees:                { Currency: curreny, Amount: 0 },
  CardType:            'CB_VISA_MASTERCARD',
  CardId:              card_id,
  SecureModeReturnURL: 'https://test.com/',
  Tag:                 'any tag'
}, idempotency_key)

Sample headers logged with HTTPLogger:

User_agent: MangoPay V2 RubyBindings/3.0.22
Authorization: Bearer c7e7ef2b0b644b3880209da57ed487ff
Content-Type: application/json
X_mangopay_client_user_agent: {"bindings_version":"3.0.22","lang":"ruby","lang_version":"2.2.5 p319 (2016-04-26)","platform":"x86_64-darwin15","uname":"Darwin MacBook-Air-2.local 15.5.0 Darwin Kernel Version 15.5.0: Tue Apr 19 18:36:36 PDT 2016; root:xnu-3248.50.21~8/RELEASE_X86_64 x86_64"}
Accept-Encoding: gzip;q=1.0,deflate;q=0.6,identity;q=0.3
Accept: */*
User-Agent: Ruby
Host: api.sandbox.mangopay.com
Content-Length: 282

However it does work for preauthorizations like this:

MangoPay::PreAuthorization.create({
 AuthorId: author_id,
 DebitedFunds: { Currency: currency, Amount: amount },
 CardId: card_id,
 SecureMode: 'DEFAULT',
 SecureModeReturnURL: 'https://test.com',
 Tag: 'any tag'
}, idempotency_key)

Sample headers logged with HTTPLogger:

User_agent: MangoPay V2 RubyBindings/3.0.21
Authorization: Bearer 059dfeae29b84c7098c6a00bd8185e4d
Content-Type: application/json
X_mangopay_client_user_agent: {"bindings_version":"3.0.21","lang":"ruby","lang_version":"2.2.5 p319 (2016-04-26)","platform":"x86_64-darwin15","uname":"Darwin MacBook-Air-2.local 15.5.0 Darwin Kernel Version 15.5.0: Tue Apr 19 18:36:36 PDT 2016; root:xnu-3248.50.21~8/RELEASE_X86_64 x86_64"}
Idempotency-Key: preauthorization-49
Accept-Encoding: gzip;q=1.0,deflate;q=0.6,identity;q=0.3
Accept: */*
User-Agent: Ruby
Host: api.sandbox.mangopay.com
Content-Length: 200

I've traced the problem to this method, when the idempotency key is passed to MangoPay::PayIn::Card::Direct.create, it is received in this method as the id argument instead of the idempotency_key. I am not sure what's the reason for that legacy logic.

Should I be calling MangoPay::PayIn::Card::Direct.create in a different way? Is this a bug?

hobailey commented 8 years ago

Thanks for bring this up @javiercr. It looks like the legacy logic was added very early on, but I can't immediately see why. We're looking into it and the best way to get round this and I'll update you when I can :-)

javiercr commented 8 years ago

Thank you for the prompt reply @hobailey.

The legacy logic was actually added in the same commit that introduced the support for idempotency keys, but I don't understand what it's for.

javiercr commented 8 years ago

You can reproduce this by editing /spec/mangopay/shared_resources.rb like this:

 let(:new_payin_card_direct) { create_new_payin_card_direct(new_wallet) }
  def create_new_payin_card_direct(to_wallet, amnt = 1000)
    cardreg = new_card_registration_completed
    MangoPay::PayIn::Card::Direct.create({
      AuthorId: new_natural_user['Id'],
      CreditedUserId: to_wallet['Owners'][0],
      CreditedWalletId: to_wallet['Id'],
      DebitedFunds: { Currency: 'EUR', Amount: amnt },
      Fees: { Currency: 'EUR', Amount: 0 },
      CardType: 'CB_VISA_MASTERCARD',
      CardId: cardreg['CardId'],
      SecureModeReturnURL: 'http://test.com',
      Tag: 'Test PayIn/Card/Direct'
    }, 'idempotency-key-test-12345')
  end

Enabling logging with http_logger (in /spec/spec_helper.rb) and running: rspec ./spec/mangopay/payin_card_direct_spec.rb:19

Relevant output:

HTTP POST (1945.88ms)   https://api.sandbox.mangopay.com:443/v2.01/sdk-unit-tests/payins/card/direct
Request body   {"AuthorId":"14306735","CreditedUserId":"14306735","CreditedWalletId":"14306737","DebitedFunds":{"Currency":"EUR","Amount":1000},"Fees":{"Currency":"EUR","Amount":0},"CardType":"CB_VISA_MASTERCARD","CardId":"14306742","SecureModeReturnURL":"http://test.com","Tag":"Test PayIn/Card/Direct"}
HTTP request header   User_agent: MangoPay V2 RubyBindings/3.0.22
HTTP request header   Authorization: Bearer bc7fb47987bd43c3b4386acc8290ff3e
HTTP request header   Content-Type: application/json
HTTP request header   X_mangopay_client_user_agent: {"bindings_version":"3.0.22","lang":"ruby","lang_version":"2.3.0 p0 (2015-12-25)","platform":"x86_64-darwin15","uname":"Darwin MacBook-Air-2.local 15.5.0 Darwin Kernel Version 15.5.0: Tue Apr 19 18:36:36 PDT 2016; root:xnu-3248.50.21~8/RELEASE_X86_64 x86_64"}
HTTP request header   Accept-Encoding: gzip;q=1.0,deflate;q=0.6,identity;q=0.3
HTTP request header   Accept: */*
HTTP request header   User-Agent: Ruby
HTTP request header   Host: api.sandbox.mangopay.com
HTTP request header   Content-Length: 289
Response status   Net::HTTPOK (200)
HTTP response header   Cache-Control: no-cache
HTTP response header   Pragma: no-cache
HTTP response header   Content-Type: application/json; charset=utf-8
HTTP response header   Expires: -1
HTTP response header   Server: Leetchi
HTTP response header   Date: Mon, 27 Jun 2016 13:13:45 GMT
HTTP response header   Content-Length: 638
Response body   {"Id":"14306743","Tag":"Test PayIn/Card/Direct","CreationDate":1467033223,"AuthorId":"14306735","CreditedUserId":"14306735","DebitedFunds":{"Currency":"EUR","Amount":1000},"CreditedFunds":{"Currency":"EUR","Amount":1000},"Fees":{"Currency":"EUR","Amount":0},"Status":"SUCCEEDED","ResultCode":"000000","ResultMessage":"Success","ExecutionDate":1467033225,"Type":"PAYIN","Nature":"REGULAR","CreditedWalletId":"14306737","DebitedWalletId":null,"PaymentType":"CARD","ExecutionType":"DIRECT","SecureMode":"DEFAULT","CardId":"14306742","SecureModeReturnURL":null,"SecureModeRedirectURL":null,"SecureModeNeeded":false,"StatementDescriptor":null}

Note the absence of the Idempotency-Key header.

hobailey commented 8 years ago

OK, thanks for these infos. Effectively, I'm not sure either why the legacy support was added, but I've asked @sergiusz-woznicki to update us as to why :-)

javiercr commented 8 years ago

After some further research I've found this is not actually a bug but an inconsistency in the (ruby) API:

MangoPay::Preauthorization.create expects two arguments params and idempotency_key as defined here.

This led me to think that idempotency_key was always the second argument passed to any create method.

However the other objects which clases do not overwrite the create method, expect 3 arguments: params, id and idempotency_key as can be seen in this example:

MangoPay::NaturalUser.create(u, nil, idempotency_key)

@sergiusz-woznicki is there a reason to have different signatures in the created method of PreAuthorization and the rest of API objects? Thanks!

hobailey commented 8 years ago

Hi @javiercr, I haven't forgotten about this, but Sergiusz has been away this week - he'll get back to us in the next couple of days :-)

ghost commented 8 years ago

@javiercr is right: it's not a bug, it's an inconsistency between the general create method and its newer versions (in PreAuthorization and Mandate classes). The id param, present in the general signature, is not required in most cases (I don't even recall if it's used anywhere in the SDK itself), however I've left it here for legacy reasons: it may be used somewhere in the wild out there. The new versions do not use this param so I've decided to simplify these signatures.

hobailey commented 8 years ago

@javiercr does this make sense, and is it satisfactory for you?

javiercr commented 6 years ago

I just got bitten by this problem again... 2 years later.

To clarify the issue, the main problem is that the current SDK expect these method calls:

MangoPay::PreAuthorization.create(params, idempotency_key)
MangoPay::PayIn::Card::Direct.create(params, nil, idempotency_key)
MangoPay::NaturalUser.create(params, nil, idempotency_key)

Note that nil in the last two calls is the value for the id argument defined here.

Having to pass nil for creating objects such as PayIn::Direct or NaturalUser in order to use idempotency keys, doesn't make a lot of sense IMHO.

Also the problem is that if you happen to use

MangoPay::PayIn::Card::Direct.create(params, idempotency_key)
MangoPay::NaturalUser.create(params, idempotency_key)

(note the absence of nil). The SDK won't complain and you'll think you are using idempotency keys when in reality you are not (they won't be included in the HTTP request).

If changing the method signature for these methods is too much of a breaking change, at least I think this behavior should be documented somewhere.

Thanks!

mickaelpois commented 6 years ago

Hello @javiercr,

What SDK version are you using ? We have release a v4.0 version with some technical improvements, maybe it could help?

I'll have a look closer too, and will keep you posted.

Mickaël