Remove customization from Plaid client code

[nprzy]

This PR removes most of the customization from the Plaid client code.

Background: I have a few tweaks that I'd like to make that require a few of the newer Transaction fields added by Plaid. The API model that was used to generate the client for this tool does not have those fields. Rather than add the fields in manually I felt that it would be preferable to regenerate the client from scratch using the latest API model. And I'd actually propose going a step further by using the OpenAPI Gradle plugin to generate the Plaid client from scratch during the build process so that none of the generated Plaid client code needs to be checked into git at all.

Most of the changes are well explained by their commit messages. The most significant change here is the dropping of the SortableTransaction interface. This interface was really only used by the transfer-matching logic. I took the opportunity to extract this transfer-matching logic out into its own class.

Assuming that you're OK with these changes, I'll follow with a PR to fully replace the Plaid client with generation of a fresh one on build via the OpenAPI Gradle plugin.

[nprzy]

And here is the follow-up work to generate the Plaid client code as a Gradle build step.

[dvankley]

Seems like a lot of work to avoid occasionally updating some API classes... but if you're doing the work then sure, why not.

[dvankley]

Did you get any errors when running the openAPI generator or when compiling the output? I don't remember perfectly, but I know I had to manually fix a fair amount of the original generated code (although maybe that was just on the Firefly side).

[nprzy]

Seems like a lot of work to avoid occasionally updating some API classes

Hah yeah, it was a bit more than I had anticipated when I started, but it's done now so ¯_(ツ)_/¯. I've never written Kotlin before so it was a fun learning exercise.

Did you get any errors when running the openAPI generator or when compiling the output? I don't remember perfectly, but I know I had to manually fix a fair amount of the original generated code.

The OpenAPI generator does spit out a few warnings, but it still generates and compiles successfully. I did see a few of your Plaid client customizations (like this and this) and I understand why they were necessary. Those issues were either things that are now fixed in the latest OpenAPI Kotlin templates, fixable via tweaks to the OpenAPI generator configuration, or were configurable externally without needing to modify the generated code. That follow-up PR will have unit tests to validate the correctness of the new generated Plaid client's HTTP interactions.

[bmschwa]

I just ran into an issue that should be solved by this pr. The error (from a citibank account) was: Cannot deserialize value of type 'net.djvk.fireflyPlaidConnector2.api.plaid.models.Products' from String "statements"... Sure enough when I cut the most recent openapi definitions & do a diff of Products.kt, The updated version has quite a few more....

Old Version: * Values: assets,auth,balance,identity,investments,liabilities,paymentInitiation,identityVerification,transactions,creditDetails,income,incomeVerification,depositSwitch,standingOrders,transfer,employment,recurringTransactions

New Version (commit c793f6d) * Values: assets,auth,balance,identity,identity_match,investments,investments_auth,liabilities,payment_initiation,identity_verification,transactions,credit_details,income,income_verification,deposit_switch,standing_orders,transfer,employment,recurring_transactions,signal,statements,processor_payments,processor_identity,profile,cra_base_report,cra_income_insights,cra_partner_insights,layer

[dvankley]

@bmschwa that seems weird to me, as my personal instance is still working fine with a Citi institution, and the connector shouldn't fail on unknown JSON properties anymore as of https://github.com/dvankley/firefly-plaid-connector-2/commit/3165f7f9475ca7435934406831dc20d52f64aab7. What version of the connector are you running?

[nprzy]

you may want to run the TransactionConverter and TransferMatcher tests with coverage and look at shoring up coverage gaps in the matching logic.

TransferMatcher is 100% covered. TransferConverter has 69% line coverage and 44% branch coverage. I can look at adding another test or two for TransferConverter to give better confidence around the lines that I changed there...

Worked my way through all the feedback (thanks for that). The things that I think are still outstanding for this PR are:

1. I'd like to take another pass at simplifying how transfers are represented. I'm going to look at adding a Transfer subtype of PlaidFireflyTransaction, and making a few other minor improvements to that interface. This would simplify the output of TransferMatcher and I think make the code a little easier to understand and work with.
2. Look at improving test coverage as mentioned above.

[bmschwa]

Appreciate the response, @dvankley .

I'm running from the docker... version I see that its version 1.1.1.... but I see a 1.1.2 came out recently so I'll pull and try that. 2024-06-28T23:10:42.858Z INFO 1 --- [ main] .d.f.FireflyPlaidConnector2ApplicationKt : Starting FireflyPlaidConnector2ApplicationKt v1.1.1 using Java 17.0.10 with PID 1 (/workspace/BOOT-INF/classes started by cnb in /workspace)

@bmschwa that seems weird to me, as my personal instance is still working fine with a Citi institution, and the connector shouldn't fail on unknown JSON properties anymore as of 3165f7f. What version of the connector are you running?

[nprzy]

Let me know when you're ready either way and I'm good to merge this.

After these last two commits, I'm good to merge if you are :)

These commits add the promised tests (https://github.com/nprzy/firefly-plaid-connector-2/commit/630128257630c19ab738660135435d1561a87fc7) and Transfer type (https://github.com/nprzy/firefly-plaid-connector-2/commit/32687b8ddcdcebb59488fae5d79f082d2ea1975e). Test coverage of TransactionConverter is up to 92% (199/215) lines and 68% (79/115) branches. And TransactionMatcher remains at 100% line and branch coverage. I did backport the tests locally and run them against your current v1.1.2 to validate no unexpected changes in behavior in this PR. The only new test that failed against v1.1.2 but passed after the changes in this PR was here where v1.1.2 would try to create that highlighted Plaid transaction, but after this PR it's recognized as already created and is omitted from the output.

[dvankley]

Good stuff; let's do it.