Ban commits/transactions using AST analysis and lintersI’ve been working on a spec for months. Finally…every stakeholder approves. I’ve cataloged every piece of code that needs to be migrated in a Notion DB. I’m working on one of the first PRs and then I see it…<br>db.commit()
“…ffffuuuuuummmmccckkkk mmmmmmeee…”<br>The fucking “transactions” are being committed while wearing a coat of atomicity.<br>“fuck, fuck, fuck, how the fuck did I miss this.”<br>Looking through my notes…I see…“investigate random-ass commits” and I forgot to account for them…<br>ORMs can blow your leg off, this ain’t that rant.<br>Parametrized SQL statements can be your saviour, this ain’t that rant.<br>Query builders can be a nice middle ground, this isn’t that rant.<br>This rant is about fucking up code organization and abstractions so badly that code wrapped inside transactions ain’t atomic.<br>Sample Exhibits#<br>These are meant to be illustrative, so code has been cleaned up and doesn’t contain all the tiny details of any framework/library. Sessions being created, etc.<br>The Hidden Enemy#<br>class DBAccess:<br>@staticmethod<br>def create_records(records: List[DomainModel]):<br># All records are meant to be a single transaction<br>with transaction():<br>for r in records:<br>DBAccess.create_main_records(r) # calls helpers that eventually call commit()<br># Another transaction is auto-started<br>DBAccess.create_details_records(r)
DBAccess.create_records(recs)
The code is fucking around with manual commits 2+ levels away from the transaction decorator/context manager. So, when create_main_records is called from the manager, no one has ANY god damn idea that the method commits.<br>The Silent Frenemy#<br>class DBAccess:<br>@staticmethod<br>def fetch_records(ids: List[int]) -> List[DBModel]:<br>db_models = session.query(DBModel).filter(DBModel.id.in_(ids)).all()<br>return cast(List[DBModel], db_models)
with transaction():<br>db_models = DBAccess.fetch_records(ids)<br>db_models[0].yo_mama_fat = True # This is a silent DB write<br># context manager exit saves your ass
The code is passing DB models around, treating them like normal domain models, setting properties that seem to do nothing other than setting the value but underneath it all, there are DB writes being triggered.<br>Daddy’s Milk Run#<br>class DBAccess:<br>@staticmethod<br>def fetch_records(ids: List[int]) -> List[DBModel]:<br>db_models = session.query(DBModel).filter(DBModel.id.in_(ids)).all()<br>return cast(List[DBModel], db_models)
# transaction has been commented out<br># with transaction():<br>db_models = DBAccess.fetch_records(ids)<br>db_models[0].yo_mama_fat = True<br># request ends, data poofs into the ether<br># like the programmer's dad leaving to get milk and never returning
The code doesn’t auto-commit and has no transactions, it’s causing data loss.<br>Want to hear more of my unhinged rants? Drop your email.
Subscribeor use the RSS feed if you hate email.
Who is responsible?#<br>DO NOT blame the framework<br>DO NOT blame the business requirement.<br>The programmer is the only one responsible. In a lot of cases, you can be absolved of the blame for the state of the code, but NOT in this case. ANYTHING is better than random ass commits.<br>In the end, your ass, whether you wrote the code or not (my case) is on the line for fixing this shit. Like it has been for months.<br>WTF are we learning?#<br>DO NOT sprinkle in DB sessions or transactions like salt-bae. If you do so, expect to crash and burn like him. The DB abstraction layer owns the transactions and commits.<br>DO NOT pass DB models in and out of the DB layer.<br>Read 1+2 again, then read them again. Absorb them.<br>DO NOT fuck with transactions, commits, queries or whatever outside of the DB layer. Don’t even look at it, don’t even think about it.<br>DO NOT commit manually. Especially if you use context managers or decorators. You are not that guy/gal pal.<br>DO NOT split code across helpers. Atomic multi-writes = one function . Write the 3 line god-damn insert again. The duplication keeps atomicity visible and you won’t create hidden enemies. Otherwise, watch your god-damn back.<br>How do you enforce the lessons?#<br>AST Analysis:#<br>Either through custom tests that use AST analysis or through flake8/linters. Whatever you choose, it should:<br>Ban commits from being called manually, COMPLETELY<br>Ban db session from being accessed outside the DB access layer<br>Ban transactions from being accessed outside the DB access layer<br>Ban DB model imports from outside the DB access layer<br>Bans using AST#<br>class TestDBBoundaries:<br>def test_no_manual_commits(self):<br>violations = []<br>for path, tree in parsed_source_files(): # ast.parse over your source tree<br>for node in ast.walk(tree):<br>if not isinstance(node, ast.Call):<br>continue<br>func = node.func<br>if (<br>(isinstance(func, ast.Attribute) and func.attr == "commit") # session.commit()<br>or (isinstance(func, ast.Name) and func.id == "commit") # commit = session.commit;...