Remove customization from Plaid client code #96

nprzy · 2024-06-24T05:24:50Z

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.

The PersonalFinanceCategoryEnum existed as custom code that had been embedded into the generated Plaid client. This extracts the custom enum and associated logic so that the Plaid client may be more easily regenerated from the OpenAPI spec.

This getTimestamp method was custom-added on top of the generated client code. Dropping this usage will make it easier to regenerate the Plaid client from the OpenAPI spec.

The PlaidTransactionId type alias is custom, added manually to the generated client code. This reverts PlaidTransactionId usage back to string to make it easier to regenerate the Plaid client.

Commit a31977f dropped the Transaction-scoped PaymentChannel enums in favor of the top-level model.PaymentChannel enum. This commit reverts that change. Although I think that those customizations make more sense than what the code generator did, reverting this change will make it easier to auto-generate the Plaid client from new versions of the OpenAPI spec.

Add a few new Firefly transactions to the input to try and "trick" the matching logic.

This commit aims to progress us towards eliminating the SortableTransaction interface, which is only used here in this transaction matching logic. The new logic uses wrapper classes as an alternative to adding custom behavior to the generated Plaid client's Transaction model.

nprzy · 2024-06-24T22:03:07Z

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

dvankley · 2024-06-25T16:16:24Z

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 · 2024-06-25T16:17:24Z

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 · 2024-06-25T17:49:14Z

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 · 2024-06-29T00:06:34Z

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

Good change overall, thanks for the contribution.

Hypocrisy disclaimer (because I definitely didn't do this the first time around): you may want to run the TransactionConverter and TransferMatcher tests with coverage and look at shoring up coverage gaps in the matching logic.

src/main/kotlin/net/djvk/fireflyPlaidConnector2/api/plaid/models/Transaction.kt

src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverter.kt

src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransferMatcher.kt

src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverter.kt

dvankley · 2024-06-29T01:03:20Z

src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/PlaidFireflyTransaction.kt

+/**
+ * Represents a single non-transfer transaction, including both the Plaid and the Firefly representation of it.
+ */
+sealed interface PlaidFireflyTransaction {


A description of the distinction between the subtypes and their intended lifecycle would be useful here.

Better documentation added in nprzy@a1fc26c

src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/PlaidFireflyTransaction.kt

src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/TransactionConverter.kt

dvankley · 2024-06-29T01:43:41Z

@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 · 2024-07-01T01:10:29Z

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:

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.
Look at improving test coverage as mentioned above.

bmschwa · 2024-07-01T22:45:33Z

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?

dvankley

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

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.

Look at improving test coverage as mentioned above.

Agreed, but don't sweat it too much.
Same as 1. I definitely think you've improved the overall quality of the code, so don't feel obligated.

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

dvankley · 2024-07-03T23:54:56Z

src/main/kotlin/net/djvk/fireflyPlaidConnector2/transactions/PlaidFireflyTransaction.kt

+ * This subtype is used when we've received a transaction from Plaid that already has a corresponding transaction
+ * in Firefly. Note that this is NOT related to


Looks like you didn't finish your thought here.

Fixed in nprzy@32687b8

nprzy · 2024-07-05T19:35:02Z

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 (nprzy@6301282) and Transfer type (nprzy@32687b8). 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 · 2024-07-07T17:48:47Z

Good stuff; let's do it.

nprzy added 9 commits June 23, 2024 23:15

Inject Plaid URL in wrapper rather than PlaidApi

4b7fba7

Drop usage of custom Plaid Transaction.getTimestamp

ddea2f1

This getTimestamp method was custom-added on top of the generated client code. Dropping this usage will make it easier to regenerate the Plaid client from the OpenAPI spec.

Revert usage of PlaidTransactionId to String

f84db3b

The PlaidTransactionId type alias is custom, added manually to the generated client code. This reverts PlaidTransactionId usage back to string to make it easier to regenerate the Plaid client.

Drop custom getDirection from generated Plaid Transaction

ab4805e

Strengthen Firefly/Plaid transfer matching test

b045db6

Add a few new Firefly transactions to the input to try and "trick" the matching logic.

Remove SortableTransaction and related orphaned code

3e75c5a

dvankley self-requested a review June 27, 2024 01:22

dvankley reviewed Jun 29, 2024

View reviewed changes

nprzy added 5 commits June 30, 2024 16:17

Re-introduce PlaidTransactionId typealias

56ad857

Clarify usage of PlaidFireflyTransaction and subtypes

a1fc26c

Remove unnecessary variable declarations

bcb576c

Replace unnecessary use of map with forEach

9819fa9

Name lambda variables to avoid shadowing

9713978

This was referenced Jul 2, 2024

FAIL_ON_UNKNOWN_PROPERTIES ... is an enum a property? #103

Closed

FAIL_ON_UNKNOWN_PROPERTIES ... is an enum a property? #104

Closed

dvankley mentioned this pull request Jul 3, 2024

Converting Plaid updates to transfers #106

Open

dvankley reviewed Jul 4, 2024

View reviewed changes

Add polled TransactionConverter test cases

6301282

Represent Transfers as subtype of PlaidFireflyTransaction

32687b8

dvankley merged commit 6848eb1 into dvankley:main Jul 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove customization from Plaid client code #96

Remove customization from Plaid client code #96

nprzy commented Jun 24, 2024

nprzy commented Jun 24, 2024

dvankley commented Jun 25, 2024

dvankley commented Jun 25, 2024

nprzy commented Jun 25, 2024

bmschwa commented Jun 29, 2024

dvankley left a comment •

edited

Loading

dvankley Jun 29, 2024

nprzy Jul 1, 2024

dvankley commented Jun 29, 2024

nprzy commented Jul 1, 2024 •

edited

Loading

bmschwa commented Jul 1, 2024

dvankley left a comment

dvankley Jul 3, 2024

nprzy Jul 5, 2024

nprzy commented Jul 5, 2024

dvankley commented Jul 7, 2024

		* This subtype is used when we've received a transaction from Plaid that already has a corresponding transaction
		* in Firefly. Note that this is NOT related to

Remove customization from Plaid client code #96

Remove customization from Plaid client code #96

Conversation

nprzy commented Jun 24, 2024

nprzy commented Jun 24, 2024

dvankley commented Jun 25, 2024

dvankley commented Jun 25, 2024

nprzy commented Jun 25, 2024

bmschwa commented Jun 29, 2024

dvankley left a comment • edited Loading

Choose a reason for hiding this comment

dvankley Jun 29, 2024

Choose a reason for hiding this comment

nprzy Jul 1, 2024

Choose a reason for hiding this comment

dvankley commented Jun 29, 2024

nprzy commented Jul 1, 2024 • edited Loading

bmschwa commented Jul 1, 2024

dvankley left a comment

Choose a reason for hiding this comment

dvankley Jul 3, 2024

Choose a reason for hiding this comment

nprzy Jul 5, 2024

Choose a reason for hiding this comment

nprzy commented Jul 5, 2024

dvankley commented Jul 7, 2024

dvankley left a comment •

edited

Loading

nprzy commented Jul 1, 2024 •

edited

Loading