Using Flask-User
for the first time. For the most part, I'm pretty impressed. Great project, well executed. Apologies in advance for the lengthy issue posting, but I hope all the extra detail is helpful.
The problem
I've just spent quite a while tracking down a number of unexpected behaviors due to a schema assumption that I have not found explicitly stated in the project documentation. I'm using the SQLAlchemyAdapter
in my project.
For quite some time, it was impossible to figure out what was going on because there were no errors thrown by Flask-User
. Instead, when it found itself unable to do something, it just dropped me back to the login page without alerting me to what the actual failure was (in dev/debug mode).
Pretty much every single bit of functionality that works with a User
object expects to do so via an explicit id
column/attribute existing on the User
model itself.
I'm pretty sure I'm not alone in the Python world in not using id
as my primary key column in my db models--because id()
is part of Python's __builtin__
module. Kind of like how I never use type
as an attribute, for the same reason. Instead, I've long made it a habit of using pk
as the primary key attribute in db models.
How does this matter to Flask-User
?
Well, in some places, I was initially able to get around this by adding an id
property to my model (trying to not have to create a model with a primary key attribute different from all the other application models):
@property
def id(self):
return self.pk
This worked for a bit, in certain spots, until I started tackling password resets. I was able to get a reset email sent off successfully, but each time I clicked on the link, I was told the token was invalid. I verified the token myself, and it was indeed valid. I also verified it had not expired (it'd only been a few seconds, but anything could happen, right?). I checked directly with the database to ensure it was indeed saved to the User
object. It was.
So, I started debugging the logic from the built in reset_password()
view function. That's when I landed on the problem being in UserManager.find_user_by_id()
. It was returning no User
object. Here's a quick refresher on the logic I copied over:
# from reset_password() view
mgr = current_app.user_manager
is_valid, has_expired, user_id = mgr.verify_token(token, mgr.reset_password_expiration)
# returns: is_valid=True, has_expired=False, user_id=123
# next up, find the user who owns the token
user = mgr.find_user_by_id(user_id) # here, user is None
Interestingly enough, UserManager.verify_token()
was returning the user_id
for the token provided to the view. So, that's when I figured it was the id
attribute itself--something must be happening in the query that was actually passing an id=user_id
instead of relying on a more abstract query that doesn't assume the name of the primary key column. I also guessed that my id
property def was screwing up raising a helpful error (it was).
So, I checked out the find_user_by_id()
source, and found:
# inside flask_user/__init__.py
def find_user_by_id(self, user_id):
return self.db_adapter.find_object(self.db_adapter.UserClass, id=user_id)
That call to SQLAlchemyAdapter.find_object
in turn calls:
# inside db_adapters.py
query = query.filter(field==field_value) # case sensitive!!
Eureka!
That filter()
method from SQLAlchemy
is an attribute-specific query method. So, it wasn't failing because my id
property was an attribute, but it was failing to have the desired effect (finding the user by its primary key value).
I know, I know, this wouldn't be happening if I'd just used the id
column like the docs show.
The problem is, I didn't actually notice the User
model schema until I was hours into working with Flask-User
. I know, still my fault. But I wanted to create this issue anyway, both to offer some feedback, and provide some help in case others experience the same thing.
Suggestions for schema-agnostic primary key queries
The major motivator for submitting this issue is to suggest this is, while still arguably a developer's fault for not thinking the schema matters so dearly, an opportunity to improve Flask-User
's handling of primary key queries and any needs to refer to and/or work with User
objects by their primary key values.
I'm not sure about the other db adapter classes, but I know SQLAlchemy
offers a handy helper method when it comes to looking up objects by their primary keys -- it's the Query.get() method! All you have to do is provide it the object's primary key as a single argument (no field names, keywords, or anything else required). An example implementation could be as simple as:
# in db_adapters.py
# for SQLAlchemy, not sure about others
def get_object(self, ObjectClass, pk):
""" Find single object of class 'ObjectClass' by specified primary key"""
return ObjectClass.query.get(pk)
Query.get()
is guaranteed to always return one and only one object based on its underlying identity field as defined in its model. So, it's the easiest query that can be made via SQLAlchemy
. It would also help make Flask-User
less coupled to an assumed schema for User
models.
Suggestion for schema-agnostic primary key usage
Because Flask-User
depends on Flask-Login
(and even offers its own UserMixin
developers can use that extend's Flask-Login
's own UserMixin
class), I felt like this was a pretty good situation to highlight how relying on certain schema columns/attributes was a bit too much coupling for an extension.
Flask-Login
already expects a get_id()
method, and its UserMixin
includes this method, as well as instructing developers to implement their own method if they need to specify how to get the id
value for their custom User
models. Moreover, it also instructs developers to specify their own user_loader()
method to ensure that whatever the id
value is, the LoginManager
can query it from the db. I haven't gone through all the Flask-User
code, but it seems to me the best de-coupled approach would be for Flask-User
to rely, like Flask-Login
does, on the result of the get_id()
method whenever it needs a User
object's primary key value. When it needs to query by that value, it should use the query.get()
method from SQLAlchemy
, instead of filtering on a schema-dependent primary key attribute value.
From what I can see, Flask-User
implements the Flask-Login
requirement for a user_loader
method via its _user_loader()
method. That method calls the schema-dependent find_user_by_id()
method that I've already detailed above. Based on my suggestions, and barring any other areas I haven't yet found in the code, if find_user_by_id()
was changed to use a get()
instead of a filter()
query call, this would instantly decouple Flask-User
from its schema dependence for primary keys (for SQLAlchemy
, at least).
It doesn't solve other schema-dependence issues, but those attributes are, for the most part, pretty sensible as far as field names are concerned (and they don't clash with __builtin__
methods/keywords). However, I still think it'd be helpful to point out that the fields are required--and required to be named exactly as they appear--in the docs. Even better would be to offer users a way to specify those fields that Flask-User
requires for its functionality to work properly. That could be as easy as a dict
or tuple
passed in so people could specify field mappings.
Thanks for an overall fantastic extension.
Feature request in dev