Skip to content
This repository has been archived by the owner on Feb 22, 2024. It is now read-only.

Committing changes in SQLAlchemyDatastore #327

Closed
wants to merge 2 commits into from
Closed

Committing changes in SQLAlchemyDatastore #327

wants to merge 2 commits into from

Conversation

Xumeiquer
Copy link

When performing any action which modifies the data base like create_role or add_role_to_user, for example. The modifications wasn't committed into the database, so adding self.commit() in SQLAlchemyDatastore under the put and delete functions solves the issue.

When performing any action which modifies the data base like create_role or add_role_to_user the modifications wasn't committed into the database, so adding self.commit() in SQLAlchemyDatastore under the put and delete functions solves the issue.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) when pulling 373f8ce on Xumeiquer:patch-1 into 76ad77a on mattupstate:develop.

@Xumeiquer Xumeiquer changed the title Update datastore.py Committing changes in SQLAlchemyDatastore Oct 9, 2014
@Xumeiquer
Copy link
Author

All tests passed except the PyPy one. According to the error message the potential solution will be updating the test fixture for sqlalchemy_datastore in terms of removing the quotes in the relationship inside the User class. The new code will look like this

class User(db.Model, UserMixin):
        id = db.Column(db.Integer, primary_key=True)
        email = db.Column(db.String(255), unique=True)
        username = db.Column(db.String(255))
        password = db.Column(db.String(255))
        last_login_at = db.Column(db.DateTime())
        current_login_at = db.Column(db.DateTime())
        last_login_ip = db.Column(db.String(100))
        current_login_ip = db.Column(db.String(100))
        login_count = db.Column(db.Integer)
        active = db.Column(db.Boolean())
        confirmed_at = db.Column(db.DateTime())
        roles = db.relationship(Role, secondary=roles_users,
                                backref=db.backref('users', lazy='dynamic'))

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) when pulling 535aa08 on Xumeiquer:patch-1 into 76ad77a on mattupstate:develop.

@jonafato
Copy link
Collaborator

jonafato commented May 7, 2015

I can see where this may be useful, but I'm not a fan of implicit commits like this. SQLAlchemy encourages explicit session commits, and many people expect and depend on their code to work that way. All of Flask-Security's views that modify data already commit using after_this_request, and elsewhere, a developer probably doesn't want Flask-Security committing their session under the hood before all of the modifications to that session have been made.

Is there a compelling reason this should happen within the put and delete methods or a use case where this makes code significantly cleaner?

@wjt wjt mentioned this pull request Jul 15, 2015
@jonafato
Copy link
Collaborator

jonafato commented Nov 5, 2015

I'm closing this due to lack of response. If there is still interest in elaborating on the rationale behind this PR, feel free to reopen it.

@jonafato jonafato closed this Nov 5, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants