[RFC] Refactor API functions to return light-weight canonical objects, instead of raw JSON

[hammem]

Looking for feedback and thoughts on this as the API grows and we have more folks touching the code. Want to make it easy to keep expanding, help folks avoid errors, and prevent from having a utterly unmaintainable MonarchMoney class.

Why bother?

As the API grows, we've started to move into mutations. With that, comes the increased chance a user accidentally edits something they didn't intend, creates a bunch of things by mistake, or (worse) deletes things they weren't intending to.

To help with this, as well as the maintainability of the codebase, I'm proposing to make the following changes:

• Create a new MonarchTransaction class, with a MonarchObj base for other things like budgets and accounts
• Enable a type-checking job on all PRs and merges via Pyre to aid in spotting accidental typing / data handling errors

The key would be to keep them light-weight. Not looking to go fully schematized, since staying in sync with Monarch won't be trivial. But, rather, strike a balance of enabling static analyzers to help you from making mistakes without making it feel onerous to use.

MonarchObj

This would be the base object for all return types.

• base object
• would extend Dict to provide a Pythonic experience
• has simple API to
  • access the original data,
  • the ID,
  • update the object,
  • delete the object,
  • a static method to create an object,
  • maybe a clone as well?

MonarchTransaction

• inherits from MonarchObj
• optionally, adds some convenience methods (e.g. update_tags)

How MonarchMoney.py would change

As a result of this, function return types would start undergoing the following changes:

async def get_transactions(...) -> Dict[str, Any]

becomes...

async def get_transactions(...) -> List[MonarchTransaction]

Other thoughts

We could go 'all in' and also create ID types, to help with statically determining if code is handling the 'right' type of ID for a transaction. But, at this point, that's probably too much overhead.

[hammem]

#31 is a good example of how not taking an object route will become increasingly challenging to maintain: large function specs and tricky required/option/default value parameterization, which is inherently brittle.

[hammem]

@CalvinChanCan , @grablair , curious for your thoughts on this proposed refactor.

[grablair]

Thanks for drafting this, @hammem! Sorry for the delay, I was on vacation.

Initial thoughts:

• has simple API to
  • access the original data,
  • the ID,
  • update the object,
  • delete the object,
  • a static method to create an object,
  • maybe a clone as well?

In this new world, would a user not call the MonarchMoney client object at all for updates/deletes? If so, that would require the MonarchObj to cache the client which retrieved it somehow, or require the caller to pass in a MonarchMoney client object. In the case where a user may have reconstructed a Monarch* object from some cached value (say, from disk), the object wouldn't have any reference to the client in the first place.

Mostly I think the design looks good to me. I like the idea of modeling these types, and allowing users to set some of the most commonly changed values via methods that are discoverable in things like irb. I think still requiring the customer to explicitly call the MonarchMoney APIs to make mutations is the right move, though. I think the MonarchMoney class should maintain that responsibility.

---

I do think we could pass off some of the code from MonarchMoney to the various Monarch* data type objects, e.g. generating the graphql query strings, which the MonarchMoney class can call, so as to reduce the monolithicness of the MonarchMoney class.

I'll think about this a bit more. Thanks again!

[hammem]

Great feedback! You're right, it needs some clean pattern to handle connection management. This is especially since moving to objects means someone will queue up 100s+ new objects and try to 'commit' them all at once. Requiring passage to the MonarchMoney class for the 'commit' will make connection management, parallelism, caching, etc, much easier to build out.

I think what we could refactor MonarchMoney to is:

• login
• mfa_login
• session saving / loading
• saving new objects
• updating existing objects
• deleting existing objects
• querying for objects (open to this though)

Let me know what you think of that as part of this refactor. It'll be a beast :)

[grablair]

That all sounds good to me!