Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize DB accesses by using an SQL JOIN when retrieving a user. (opr #679) #56

Merged
merged 2 commits into from
May 6, 2019

Conversation

jwag956
Copy link
Member

@jwag956 jwag956 commented May 5, 2019

Datastore.find_user() is used on every page load to verify the user in
the current session; until now, find_user() would trigger 2+ DB
accesses, since the roles relationship uses lazy loading by default.
This is useful to reduce the rows accessed when retrieving multiple
users from the DB.

However, in find_user() we always retrieve only one, so
it makes sense to retrieve all roles in one SQL operation instead of
always triggering 2 or more.

This would be unnecessary if the roles relationship was defined with
lazy="joined". However, that would greatly increase the number of rows
accessed on all queries (even when querying all users), so I think it's
better to be selective and optimize only these accesses which happen on
every page load.

@jwag956 jwag956 merged commit 32968e5 into master May 6, 2019
@jwag956 jwag956 changed the title Optimize DB accesses by using an SQL JOIN when retrieving a user. Optimize DB accesses by using an SQL JOIN when retrieving a user. (opr #679) May 6, 2019
@jwag956 jwag956 deleted the opr679 branch May 6, 2019 02:36
@madstacks
Copy link

madstacks commented Jun 6, 2019

After this change I am seeing this new error. Do you have any recommendations?

Got exception during processing: <class 'sqlalchemy.exc.InvalidRequestError'> - 
'User.roles' does not support object population - eager loading cannot be applied.

My roles relationship on the User model has lazy='dynamic' set.

@madstacks
Copy link

Removing lazy='dynamic' fixed the issue, but perhaps this should be in the release notes as something to be aware of when upgrading from flask-security to flask-security-too.

@jwag956
Copy link
Member Author

jwag956 commented Jun 6, 2019

Thanks for working this out. I really don't want to break backwards compat if I can help it - this was a PR I got from the original flask-security.

Can you add to this thread the snippet of model code you had to change?

@madstacks
Copy link

madstacks commented Jun 6, 2019

This is what I had to change on the User model. However, it also requires updating any code that relies on roles returning a Query object. e.g. this will no longer work user.roles.count() .

Thanks for your work on keeping this project alive!

     roles = db.relationship('Role',
                             secondary=roles_users,
-                            backref=db.backref('users', lazy='dynamic'),
-                            lazy='dynamic')
+                            backref=db.backref('users', lazy='dynamic'))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants