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.pkThis 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.
React
Typescript
Django
Laravel
Facebook
Microsoft
Google
Alibaba
D3
Tencent