Cleanup Transaction inheritance. #16364
Conversation
|
Sorry, I missed the previous discussions. Overall of this doesn't seem like an improvement to me – it seems pretty clear that a real transaction needs to behave differently than an emulated one (i.e. a savepoint), so it seems fair that we have specialized subclass that exposes a similar interface with different implementation. Collapsing down that into a single "Keeping track of state using inheritance is bad" is a fair criticism of the previous implementation is a fair criticism, so removing |
|
Critique was that the txn classes inherited a mix of both interface and implementation and took on transactional recordset duties.
|
|
@chancancode @jeremy , I changed this a bit:
Thats the hierarchy now. |
| records.uniq.each do |record| | ||
| begin | ||
| record.rolledback!(self.is_a?(RealTransaction)) | ||
| record.rolledback!(!savepoint_name) |
There was a problem hiding this comment.
Using an implementation side effect—presence of savepoint_name—to answer a different question, how far the record should roll back.
There was a problem hiding this comment.
so we extract that to a method then?
rollback_record() thats what you mean?
There was a problem hiding this comment.
Something like
record.rolledback! full_rollback?With method defined as true on txn and false on savepoint
Transaction class doesnt need to encapsulate the transaction state using inheritance. This removes all Transaction subclasses, and let the Transaction object controls different actions based on its own state. Basically the only actions would behave differently are `being`,`commit`,`rollback` as they could act in a savepoint or in a real transaction.
Clarify Transaction responsibilities by breaking unneeded inheritance hierarchy.
Transaction class doesnt need to encapsulate the transaction state using
inheritance.
This removes all Transaction subclasses, and let the Transaction object
controls different actions based on its own state. Basically the only
actions would behave differently are
being,commit,rollbackas theycould act in a savepoint or in a real transaction.
[related to https://github.com//pull/16341#issuecomment-50791622]
review @chancancode @tenderlove @rafaelfranca @jeremy