Skip to content

Fix OpenBanking mapping crash when a scalar field is returned as an array - #100

Open
RadoslavSheytanovGP wants to merge 1 commit into
globalpayments:masterfrom
RadoslavSheytanovGP:fix/openbanking-array-cast
Open

Fix OpenBanking mapping crash when a scalar field is returned as an array#100
RadoslavSheytanovGP wants to merge 1 commit into
globalpayments:masterfrom
RadoslavSheytanovGP:fix/openbanking-array-cast

Conversation

@RadoslavSheytanovGP

Copy link
Copy Markdown

Fixes #99. Also resolves #97.

Problem

ReportingService.BankPaymentDetail(...).Execute() can return a 500 that never reaches the caller's Results. The crash is in JsonDoc.GetValue<T> (Utils/JsonUtils.cs), not in the mapping.

When the gateway returns a field as a JSON array where a scalar is expected, ParseObject stores it as a List<string>. GetValue<T> then hits Convert.ChangeType(List<string>, DateTime), which throws InvalidCastException. The existing catch runs return (T)_dict[name], casting the list straight to the target type, so it throws a second time and that one is uncaught.

Reported against created_on, but it is not specific to that field. Every field in MapTransactionSummary reads through GetValue<T> and fails the same way if it arrives as an array.

Stack trace from the field report:

  • JsonDoc.GetValue[T]
  • OpenBankingMapping.MapTransactionSummary
  • OpenBankingMapping.MapReportResponse[T]
  • OpenBankingProvider.ProcessReport[T]
  • ReportBuilder.Execute

Change

One method, JsonDoc.GetValue<T>. The catch now:

  • returns the value when it is already type T (the original intent, e.g. a nested JsonDoc),
  • collapses an array to its first convertible element,
  • returns default(T) otherwise, instead of re-throwing.

No public API change. The path only runs inside a catch that previously always threw, so the worst case turns a crash into a value or a default(T).

Tests

GpEcomOpenBankingMappingTest, two cases:

  • created_on as an array. Throws on current master, passes here.
  • created_on as a plain string. Guards the normal path, which stays exact.

Verified red then green: reverting the JsonUtils.cs change makes the array test fail with the exact reported exception, InvalidCastException: Unable to cast object of type 'System.Collections.Generic.List1[System.String]' to type 'System.DateTime'.

Note for triage

Re-running the report later returns the transaction's current state, which is scalar, so it looks unreproducible. The payload that failed was the one from the original callback, not a fresh query. To confirm the live trigger, pull the raw stored callback JSON for the transaction rather than re-querying it.

…rray

JsonDoc.GetValue<T> could throw an uncaught InvalidCastException while
mapping an Open Banking BankPaymentDetail response. When a field such as
created_on came back as a JSON array it was stored as a List<string>;
Convert.ChangeType then failed and the catch block cast the list straight
to the target type, throwing a second time. That surfaced to callers as a
500 during ReportingService.BankPaymentDetail(...).Execute().

The catch now returns the value when it is already the requested type,
otherwise collapses an array to its first convertible element, and falls
back to default(T) instead of re-throwing. This covers every field mapped
through GetValue<T>, not just created_on.

Adds GpEcomOpenBankingMappingTest covering the arrayed and scalar cases.
@RadoslavSheytanovGP

Copy link
Copy Markdown
Author

One follow-up worth flagging while reviewing.

This PR stops the crash and recovers the value on a best-effort basis. There is a smaller, separate quirk behind it: Newtonsoft parses a scalar ISO date straight into a DateTime, so the normal path stays exact, but an array element goes through child.Value<string>() in ParseTypeArray, which reformats it. So for the pathological case of an arrayed date, the recovered value ends up culture and serializer dependent. Arrayed string fields (a more realistic case, like an arrayed order_id) recover exactly. The test asserts what is guaranteed rather than a fragile timestamp.

If a reviewer wants exact dates even on a malformed array, there are two options I deliberately kept out of this change to keep the blast radius small:

  1. Parse with DateParseHandling.None in JsonDoc.Parse so ISO dates stay as strings and both the scalar and array paths convert identically. Cleanest, but it touches the shared parser and every mapper that relies on the current behaviour, so it wants its own PR and a wider test pass.
  2. Read the date defensively at the mapping layer, e.g. GetValue<DateTime?>("created_on") with a converter in MapTransactionSummary. Narrow and OpenBanking only, but it does not help the other fields.

Happy to fold either in if you prefer, otherwise I would suggest tracking option 1 separately. This PR is scoped to the "do not 500" fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant