mattermost / mattermost-plugin-msteams

MS Teams plugin for Mattermost
Other
13 stars 11 forks source link

Adds the transactional store generator #639

Closed mgdelacroix closed 2 months ago

mgdelacroix commented 2 months ago

Summary

This PR adds a generator to the Store that allows to make any method transactional in a quick and easy way, and makes store methods easily reusabe inside the store itself, allowing them to be used alone or as part of a transaction without having to adapt anything in the code. This is achieved through the following changes:

mgdelacroix commented 2 months ago

A few, non-blocking comments, including re: @withTransaction, but I'm curious why we feel the need to generate this code vs. just having explicit, inline DoBlah() and doBlah() implementations (using sq.BaseRunner: that's the key!)?

I'm (slightly) worried that we're trading clear, unambiguous code with metaprogramming and without a dramatically better outcome.

The end result is still explicit, although all the generated public methods end in a specific public_methods.go file and not right above their private implementations. The key reason to use a generator is that all methods need to have their public/private pair for the store to be callable internally and fully transactional, and that's going to be a lot of repeated code if we do it inline, even if it's just 90% proxy methods that don't require a transaction.

I don't think generating those adds that much confusion really, and there is no way to bypass this without understanding it (a developer would need to add a method to the generator's skip list). Can you think of a specific concern or use case that you see as specially confusing?

lieut-data commented 2 months ago

Can you think of a specific concern or use case that you see as specially confusing?

Nothing concrete yet! I think your explanation above makes sense, and I'm less concerned if we can (eventually) move the annotation to the implementation. Thank you!

mgdelacroix commented 2 months ago

@jespino @lieut-data requesting re-review as I've moved the annotations to golang db:withTransaction directives in the private method implementation and added the db:withReplica to use database replicas instead of the master node when possible