braintree / braintree_dotnet

Braintree .NET library
https://developer.paypal.com/braintree/docs/start/overview
MIT License
136 stars 73 forks source link

PaymentInstrumentType filter on TransactionSearchRequest always returns empty result #117

Open peter-sabath opened 3 years ago

peter-sabath commented 3 years ago

General information

Issue description

We have a problem with PaymentInstrumentType filter on an TransactionSearchRequest using tsr.PaymentInstrumentType.Is(PaymentInstrumentType.PAYPAL_ACCOUNT.ToString()); results in an empty result set.

looking at the source I don't understand why this is still a MultipleValueNode<TransactionSearchRequest, string> typed filter and not an EnumMultipleValueNode<TransactionSearchRequest, PaymentInstrumentType> one as it is used for other enum based filters.

Another unexpected behavior is that using

tsr.CreditCardCardType.Is(CreditCardCardType.MASTER_CARD.ToString()); returns different results than tsr.CreditCardCardType.Is(CreditCardCardType.MASTER_CARD);

The string typed one returns the expected results, the "older" string-based usage returns fewer elements.

peter-sabath commented 3 years ago

Update

I checked how those values get serialized to XML and found a difference:

using tsr.PaymentInstrumentType.Is(PaymentInstrumentType.PAYPAL_ACCOUNT.ToString()); creates the following xml:

<?xml version="1.0"?>
<search>
  <payment-instrument-type type="array">
    <item>PAYPAL_ACCOUNT</item>
  </payment-instrument-type>
</search>

while manually using tsr.PaymentInstrumentType.Is("paypal_account"); works and creates that XML

<?xml version="1.0"?>
<search>
  <payment-instrument-type type="array">
    <item>paypal_account</item>
  </payment-instrument-type>
</search>

Similar there is a difference for the CreditCardType (and most likely for the other enums as well) Using tsr.CreditCardCardType.Is(CreditCardCardType.MASTER_CARD.ToString()); create

<?xml version="1.0"?>
<search>
  <credit-card-card-type type="array">
    <item>MASTER_CARD</item>
  </credit-card-card-type>
</search>

while using tsr.CreditCardCardType.Is(CreditCardCardType.MASTER_CARD); create

<?xml version="1.0"?>
<search>
  <credit-card-card-type type="array">
    <item>MasterCard</item>
  </credit-card-card-type>
</search>

So the mixed-case variant is the right one compared to your docs, but I am wondering why the all-caps version returns any result at all.

For the PaymentInstrumentType it is also an casing issue.

Epreuve commented 3 years ago

Hey @peter-sabath

Thanks for the thorough details on the issue. There's a few things at play here, so I figured I'd try to address them one at a time.

  1. So the mixed-case variant is the right one compared to your docs, but I am wondering why the all-caps version returns any result at all.

That's a great question. The API the search portion of the SDK hits is maintained by another team, so I don't have too much knowledge around it's inner workings. I'd assume it's a quirk related to some older SDK version, but I can't say for sure. We'll take a look with that team and see if we can work in unison to make it a bit more failure resistant to slightly-off values to prevent confusion in the future.

  1. We have a problem with PaymentInstrumentType filter on an TransactionSearchRequest using tsr.PaymentInstrumentType.Is(PaymentInstrumentType.PAYPAL_ACCOUNT.ToString()); results in an empty result set.

    looking at the source I don't understand why this is still a MultipleValueNode<TransactionSearchRequest, string> typed filter and not an EnumMultipleValueNode<TransactionSearchRequest, PaymentInstrumentType> one as it is used for other enum based filters.

A great find. I also can't find a reason as to why it's currently typed as a String either. It's been that way for a number of years (since 3.0 at least, which was in 2016) so it's likely an artifact of that major version bump that we overlooked.

Ideally we'd move it over to the same EnumMultipleValue collection same as CreditCardCardType and continue to leverage the DescriptionAttributes and GetDescription (via our EnumHelper) on PaymentInstrumentType, same as we do for CreditCardCardType. That should clean up the API there and also help to avoid the unnecessary confusion here.

As a stop gap, I'm going to test if we can at least control the downcase scenario with PaymentInstrumentType (and other string values) in the SDK, which should allow you to use the enum albeit still calling ToString until we can shift to the proper enum type constraint. As I believe this would constitute a breaking change, we'd have to push it in the next major version bump to stay semantic, but I'll see what we can come up with.

hollabaq86 commented 2 years ago

For our internal notekeeping, ticket number 1614