Add transaction to CreateProgram

[timothycpoon]

Whenever we create a program, we want to ensure that it is associated to a user profile. Currently, the CreateProgram function in handler/db.go does both operations separately. To do this, we should create a new function in db/db.go which both creates a program and associates it with a user. For inspiration, you can look at ForkProgram in db/program.go.

[mizlan]

@lumisphere902 Is there a reason why this should be a new function? Would it be easier to just edit CreateProgram in handler/db.go?

Also, would these be the only necessary lines to wrap in the transaction?

https://github.com/uclaacm/teach-la-go-backend/blob/836670f083e3383baffba92d21106b1ac2b3ccfe/handler/program.go#L90-L112

[mizlan]

Ah I see, that doesn't work right out the box, since handler relies on TLADB which doesn't necessarily have RunTransaction(). Maybe it would make sense to add a RunTransaction method (which takes a callback akin to firebase's DB.RunTransaction() to the TLADB interface? This way we can reuse this method for other functionality that would ideally run in transactions.

Implementation details: RunTransaction would presumably do nothing special for a mockDB, and would just delegate to DB.RunTransaction() for Firebase

[mizlan]

Hmm This would mean having to implement all of firestore.Transaction's methods as an interface

[mizlan]

Is it okay to assume the read operations won't error? why is the err value ignored below?

https://github.com/uclaacm/teach-la-go-backend/blob/836670f083e3383baffba92d21106b1ac2b3ccfe/handler/program.go#L91

https://github.com/uclaacm/teach-la-go-backend/blob/836670f083e3383baffba92d21106b1ac2b3ccfe/handler/program.go#L94

[mizlan]

@krashanoff Wanted to ping you for your guidance. I think our goal here is to implement a transaction abstraction in our database interface. Of course not all the handlers require transactions in their computations, only the more complex ones, so one option is to just reimplement these complex handlers by writing out internal functions with transactions (as I have begun here: https://github.com/uclaacm/teach-la-go-backend/pull/171). Or another alternative is to abstract over the fundamental Get/Set/Create/etc. functions over databases and transactions so that we rewrite all the functions to rely on this abstraction. I'm going to proceed with the former.

Also note: it's hard to test this, since the transaction behavior is sort of unique to the actual Firestore database and not the mock.

Also idea: use the Cloud Firestore Emulator for testing?

[mizlan]

Another thing I forgot to consider: in a transaction you must perform all reads before any writes. This makes it harder to use the existing abstractions such as LoadUser and CreateProgram in a human-readable order, especially when considering classes support, which would require additional code that must be split into reads at the top and writes at the bottom.

[mizlan]

I think the most reliable way for now is to introduce some code duplication and work with only the primitives within transaction callbacks, ignoring the potential to abstract things away using our existing helper functions. This is how ForkProgram() currently works. This does make it a bit complicated for our mock database abstraction though... Might fold and use Cloud Firestore Emulator

[krashanoff]

Hey Michael. Hope you've been well. I can contribute a few thoughts. First, the premise of the issue:

Whenever we create a program, we want to ensure that it is associated to a user profile. Currently, the CreateProgram function in handler/db.go does both operations separately.

I'll be the bad guy here and state (with many qualifiers) that in most web applications, in this sort of interaction, this specific kind of incorrectness is tolerable. If the application can operate with a floating object in its database, one can add a periodic task to sweep out and report strays. If the number of strays exceeds a threshold that worries you, then one can justify making the whole thing a transaction. This said, Firebase offers a pretty generous free tier, so unless you have a user spamming the service with no rate limit, you're probably fine even without a garbage collector style process.

If you'd like to be able to abstract the transaction interface at large for testing, though, there is some value there. Off the top of my head, I think you can probably get away with something like this:

classDiagram
class Database {
 <<interface>>
+Get()
+Set()
+Find()
}

Database <|.. Client
Database <|.. Tx
class Tx {
-backend: *firestore.Transaction
}
class Client {
+AsTransaction(): Tx
-backend: *firestore.Client
}
Client ..> Tx
Client <|-- Mock

There are trade-offs to this approach. This will introduce code duplication in another way since one would implement any addition to the database interface on three data types, counting the mock. On the other hand, it avoids the weird abc vs. abcTransact nomenclature, and lets any function that needs to work with the interface be easily serializable.

Another thing I forgot to consider: in a transaction you must perform all reads before any writes. This makes it harder to use the existing abstractions such as LoadUser and CreateProgram in a human-readable order, especially when considering classes support, which would require additional code that must be split into reads at the top and writes at the bottom.

In my opinion, there's no need to sweat this detail; it will only cause you more pain. The only way this could be a concern is if you are testing a query planner for correctness. My advice would be to focus on high level functionality for whether the app performs the operation atomically. When implementing a mock, you can just manually lock and unlock the underlying data structure so each competing client goes one at a time.

[mizlan]

Got it!

I'll be the bad guy here and state (with many qualifiers) that in most web applications, in this sort of interaction, this specific kind of incorrectness is tolerable.

Okay, maybe I'll work on something else for now :)

However:

Another thing I forgot to consider: in a transaction you must perform all reads before any writes. This makes it harder to use the existing abstractions such as LoadUser and CreateProgram in a human-readable order, especially when considering classes support, which would require additional code that must be split into reads at the top and writes at the bottom.

In my opinion, there's no need to sweat this detail; it will only cause you more pain

This is actually a technical limitation of Firestore transactions, not (just) a theoretical "should-be-done-this-way" deal. So I can't avoid this issue.

Also: all this has caused me to want to use Cloud Firestore Emulator (see this), which I think would remove the need for building a mock and make life easier at the cost of having to install the gcloud CLI.

[timothycpoon]

Sorry for the delay; I didn't have access to my computer. Leo seems to have covered most concerns, though.

This is actually a technical limitation of Firestore transactions, not (just) a theoretical "should-be-done-this-way" deal. So I can't avoid this issue.

Also: all this has caused me to want to use Cloud Firestore Emulator (see this), which I think would remove the need for building a mock and make life easier at the cost of having to install the gcloud CLI.

This is probably the "most correct" way to do testing, although it has a relatively hefty overhead/onboarding cost. Up to you if it's worth it.

The other stated options (i.e. postponing the ticket, working within the constraints) also work.

I think you answered a lot of your own questions, but here are a couple:

Ah I see, that doesn't work right out the box, since handler relies on TLADB which doesn't necessarily have RunTransaction(). Maybe it would make sense to add a RunTransaction method (which takes a callback akin to firebase's DB.RunTransaction() to the TLADB interface? This way we can reuse this method for other functionality that would ideally run in transactions.

Implementation details: RunTransaction would presumably do nothing special for a mockDB, and would just delegate to DB.RunTransaction() for Firebase

I'm not 100% confident on this, but I believe that if you only used RunTransaction from within DB.go, (e.g. in DB.CreateProgram), then there is no need for TLADB to know what a transaction is at all.

Is it okay to assume the read operations won't error? why is the err value ignored below?

https://github.com/uclaacm/teach-la-go-backend/blob/836670f083e3383baffba92d21106b1ac2b3ccfe/handler/program.go#L91

https://github.com/uclaacm/teach-la-go-backend/blob/836670f083e3383baffba92d21106b1ac2b3ccfe/handler/program.go#L94

This is likely just a mistake