vapor / fluent-postgres-driver

🐘 PostgreSQL driver for Fluent.
MIT License
146 stars 53 forks source link

Add ability to set `inTransaction` flag when configuring a database #198

Closed 0xTim closed 2 years ago

0xTim commented 2 years ago

This allows you to configure a test database when you manually start and stop transactions outside of Fluent in order to rollback changes in tests without any other code changes to get Fluent queries to work

0xTim commented 2 years ago

Some context:

This came from a client who has a test set up and teardown for integration tests to run the whole test in a transaction. Their setup involves a

try await sqlDB.raw("BEGIN TRANSACTION;").run()

and their teardown involves

try await sqlDB.raw("ROLLBACK;").run()

However Fluent internally stores whether the DB is in a transaction to be able to work out which code path to take https://github.com/vapor/fluent-postgres-driver/blob/main/Sources/FluentPostgresDriver/FluentPostgresDatabase.swift#L84-L86. So in order for Fluent to execute correctly they need to set the flag or change all their business logic to see if they are in a test or not and then either execute the closure or start a transaction I did try to create a custom extension for Fluent Postgres to expose it as in the PR but everything is internal so it was a complete copy and paste of the entire stack

rausnitz commented 2 years ago

Would it be better to have something like automateTransactionCommands property in the config (defaulting to true)?

The one additional code change that would require is that this guard https://github.com/vapor/fluent-postgres-driver/blob/main/Sources/FluentPostgresDriver/FluentPostgresDatabase.swift#L84-L86 would need to be guard !self.inTransaction && automateTransactionCommands else.

A couple reasons for this, neither of which affects the underlying functionality:

  1. I think that property name would make the purpose of this config option clearer.
  2. If you never want to use the automated transaction commands in your application, it feels weird to set inTransaction to true during config when you are in fact not in a transaction yet.
gwynne commented 2 years ago

I'm uneasy about adding this - there are a lot of ways to turn this into a footgun. I'd ask why this client can't leverage the existing withTransaction(_:) API instead of manually issuing raw transaction control queries; certainly some refactoring would be required, but it beats opening an explicit hole in the transaction safety logic.

0xTim commented 2 years ago

Closing in favour of a more explicit start and end transaction