sqlalchemy-mixins
sqlalchemy-mixins copied to clipboard
absent1706
Commit changes to DB. Fix #53
Notes:
- Tested manually using MySQL v8.0.21
- Ran all
pytesttest cases. (76/76 passed. 16 warnings)
Fixed. I've added an autocommit check. Do you feel the try:except block should also be moved to just be around the commit command?
I can't decide because, even when flush autocommits, there's a chance things might fail, and we want a clean session object.
There's one concern. All users who have used the previous hack will start seeing the same error you were seeing in the tests. Not an issue, but this might require a major version number change instead of a minor one, to prevent existing workflows from breaking.
@Shashwat986 if this is a breaking change then we will have to wait and plan it out. I also feel like autocommit flag does the job well. I've been testing and data is being persisted without any issue.
After some more tinkering myself, I also feel that using the autocommit=True flag is better suited for the job. It provides users with more flexibility — some may want to autocommit, others may not want to. Not hardcoding commit() into the save() method also allows users to temporarily skip autocomitting when necessary. For example, to wrap several insert/update statements into a single transaction.
session = scoped_session(sessionmaker(bind=engine, autocommit=True))
BaseModel.set_session(session)
# Create two users within a single transaction,
# does not commit until the end of the block is reached
# (or until we manually call User.session.commit())
with User.session.begin():
User.create(email="[email protected]")
User.create(email="[email protected]")
However, I think it would be good to emphasize more strongly that the autocommit flag should be set. I can see it in the examples for activerecord. But perhaps it would be good to add it to the examples for all_features, too? And/or directly in the README?
Thanks, @FinnWoelm. I totally agree. Let me make the updates to the README. @Shashwat986 in light of the current discussion, I won't be merging this PR. I do apologize for any miscommunication.
So, the closest that ActiveRecord has to this functionality, is a .new method that does exactly this. Creates an instance without storing it to the DB.
Alternatively, Rails has specific ActiveRecord::Base.transaction blocks that can be used for this. I feel, even if we want to have this functionality, there should be a more ActiveRecord-like way of doing it.
Rails does not have the concept of flush vs commit. In Rails, every flush is automatically a commit. So to get Rails-like behavior, you want to pass autocommit=True to the sessionmaker. Then, every save() and every create() (etc...) is automatically committed to the database, just like in Rails.
This also allows you to replicate the transactions-behavior from Rails that you mentioned, by writing:
# everything below happens in one transaction
with User.session.begin():
Account.create(...)
User.create(...)
As someone coming from Rails, the difference between flush and commit was confusing to me. I found this SO thread very helpful: https://stackoverflow.com/questions/4201455/sqlalchemy-whats-the-difference-between-flush-and-commit
PS: Since you mentioned .new: To replicate the behavior of .new with this package, you would either just call User(email="XYZ", name="ABC", ...) or you could call User().fill(email="XYZ", name="ABC").)