Comparasing between objects causes crash if _eq_ of db model class is "occupied"
stenhardvara opened this issue · 24 comments
There seems to be an incorrect check for object equality. It works if dunder eq is not used in application since it will probably default to object reference equality.
Look into changing ""==" to "is" in
I started to think about this. I think it might be quite hard to solve actually. I couldn't find if the eq has any definition (except for python default) or "==" is used anywhere in sqlalchemy for a model class.
The only reason it crashed for me seems to be that I used the eq for application purposes and it was initialised to a non-callable (for my application to get an exception if "==" was used before equality was defined).
I am not really sure about the purpose for the test for equality in the line above, is to check for model object equality in the sense having the same data and private key? Or is it to check if it actually is the same object by checking memory address?
Looking at the previous implementation is seems that a corresponding check is done towards a list with "in". Which will return true if any of the two cases with either "is" or "==" is true with any element in the list. Maybe the solution here is to write a specific check that never uses "==" and takes in consideration the "get_pk" method when specified. With no get_pk compare keys and that the object is of same type with isinstance,
Can you provide a reproducible example causing the error?
@ElLorans Sure, when editing a scoreboard object here it will crash trying to call the dunder eq for the class result:
File "/home/.venv/lib/python3.11/site-packages/flask_admin/contrib/sqla/fields.py", line 170, in iter_choices
yield (pk, self.get_label(obj), obj in self.data)
^^^^^^^^^^^^^^^^^
File "/home/admin_test.py", line 28, in __eq__
return Result.score_eq(self, value)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: 'NoneType' object is not callable
from flask import Flask
from flask_admin import Admin
from flask_admin.contrib.sqla import ModelView
from flask_sqlalchemy import SQLAlchemy
db = SQLAlchemy()
app = Flask(__name__, instance_relative_config=True)
class Scoreboard(db.Model):
id = db.Column(db.Integer, primary_key = True)
name = db.Column(db.Text)
results = db.relationship('Result')
def __str__(self):
return f'{self.name}'
class ScoreboardView(ModelView):
create_modal = True
edit_modal = True
form_columns = ['name','results']
class Result(db.Model):
score_gt = None
score_eq = None
def __eq__(self, value: object) -> bool:
return Result.score_eq(self, value)
def __gt__(self, value: object) -> bool:
return Result.score_gt(self, value)
@classmethod
def set_score_comparison(cls, gt, eq):
cls.score_gt = gt
cls.score_eq = eq
id = db.Column(db.Integer, primary_key = True)
name = db.Column(db.Text)
score = db.Column(db.Integer)
scoreboard_id = db.Column(db.Integer, db.ForeignKey('scoreboard.id'))
scoreboard = db.relationship('Scoreboard', back_populates='results', lazy='joined')
class ResultView(ModelView):
create_modal = True
edit_modal = True
form_columns = ['name','score','scoreboard']
def create_app():
app.config['SECRET_KEY'] = 'secret_stuff'
app.config["SQLALCHEMY_DATABASE_URI"] = 'sqlite://'
app.config['FLASK_ADMIN_SWATCH'] = 'darkly'
admin = Admin(template_mode="bootstrap3")
db.init_app(app)
admin.init_app(app)
with app.app_context():
db.create_all()
admin.add_view(ScoreboardView(Scoreboard, db.session))
admin.add_view(ResultView(Result, db.session))
@app.route("/")
def show_scores():
return "<p>*Not EQ*</p>"
if __name__ == "__main__":
create_app()
app.run(debug=True)
I believe the error is in your code:
class Result(db.Model):
score_gt = None
score_eq = None # fix this
@ElLorans No that is not an error! That is a feature on my application code. What I am saying that application code should be able to have any thing or any callable in that line without braking flask-admin. Right now flask-admin as a library is dependent on that the '''eq()''' method is defined and also means some specific form of equality for application specific classes.
I don't think that is suitable since equality of objects (in this case application specific objects) means different things in different applications. Imagine having a class "person" that map to your application database and you add 'eq()' to check is height is equal. Then flask-admin would not provide expected behaviour, but start to list some of the persons as selectable in some forms.
it's the 'in' operator failing on your code because your eq calls a None value. This is not a flask-admin issue but a design flaw in your code. You can easily solve it.
@ElLorans No you don't understand. That fact that is crashes just exposes what I think is a very bad behavoiur of flask-admin. I think it is a fundamental design error of flask-admin to use the == operator on database mapped class instances. The "==" is not defined as the same idea of equality in different applications. Flask-admin cannot check "equality" with "==" of the database table mapped classes instances since it not good assumption that it is equality in the same abstract sense in different applications. Equality can mean different thing in different cases.
I understand perfectly. Once again, the issue is not in flask-admin which is calling the 'in' operator. If you believe the 'in' operator should not call eq and you are unwilling to change your code so that your object is able to be compared, you can post an issue on cPython.
First of all. No, flask admin does not call the in operator where this happens. Flask-admin calls the "==" operator.
Calling the "==" in flask-admin on application defined database table row mapped class makes absolutly no sense. Because equality is application dependent in sqlalchemy table mapped classes .
Database table row equality is in the private key/id.
Object instance equality is in the memory address/reference of the objects.
These are not necessarly the same!
There can be many instances from the same table row. Then the private key/id is the same but the object reference is different.
And on top of that "==" can mean whatever the application want.
Could you explain how it is supposed to make sense to check for equality on user defined database table mapped classes instances with "=="?
First of all. No, flask admin does not call the in operator where this happens. Flask-admin calls the "==" operator.
That's not what the error says. Take this as an example:
class Result:
score_gt = None
score_eq = None
def __eq__(self, value: object) -> bool:
return Result.score_eq(self, value)
def __gt__(self, value: object) -> bool:
return Result.score_gt(self, value)
@classmethod
def set_score_comparison(cls, gt, eq):
cls.score_gt = gt
cls.score_eq = eq
def __init__(self, id, name, score, scoreboard):
self.id = id
self.name = name
self.score = score
self.scoreboard = scoreboard
a = Result(1, "a", 0, 0)
b_named_a = Result(1, "a", 0, 0)
c = Result(1, "c", 1, 0)
d = Result(1, "d", 0, 1)
"a" in [a, b_named_a, c, d] # this will raise an error
I would expect the in operator to work on a collection of objects, but your design makes it impossible: you would first need to set the score_eq function on each object inside the collection. Is that really what your logic requires you to do? I would say you are not following OOP.
Sorry I messed up in my exemple, it is not crashing where I intended. Didn't look close enough at the error.
The intention was to fail at the line in my original post where "==" is.
My application code does not look like this and been significantly refactored since July.
I agree that the in operator should work on a collection. So "==" should throw NotImplemented if not implemented and then in will check "is".
However I still thing that "==" in my original post should be either changed to "is" for reference equality or to "a.get_pk() == b.get()" where a and be is being compared. This depends of the purpose for flask-admin to check this equality.
That you can do it yourself by overriding correctly the eq method
I think that is an incorrect statement. The eq method is supposed to be used for value equality. Which will cause in some cases sqlachemy to do multiple queries and unexpected results.
Either the private key (table row the same) or the object reference (same object) is what should make sense for flask-admin to consider as equality.
fask-admin will happily do that if you tell your object to do it.
You only need to change to this:
'''
def eq(self, value: object) -> bool:
return self.id == value.id
'''
I know that I can set it to whatever I want in my application context. That is also irrelevant in my specific application at this point since everything relating to this is already refactored away for other reasons.
I still think flask-admin as a library should not be dependent on "==" on sqlalchemy database mapped object instances.
your example crashes on the 'in' operator, that calls the == operator. You can run my example or look at the error you provided to verify that.
Yes I have clearly stated that is a misstake by me when attempted to recreate the problem. Please ignore the example.
Please look at the specific line provided in the first comment when I reported this.
okay now I understand what you are talking about. But that boolean is needed to understand what field must be marked as 'selected'. using 'is' would not work.
If "is" wont work and test is to check if a object match the selected one in seems to me that the primary key equality is perfect.
Then if a user have multiple entries in the database have same values (for other then primary key) not cause any problems. Like two people with the same name or something else.
And no extra queries would automatically be made by sqlalchemy to get unloaded fields/relationships that is not necessary.
So I would suggest doing something like obj.get_pk() == self.data.get_pk() if that doesn't create problems.
but self.data is the wtf form, it does not have a pk. Comparing a orm table's attribute vs a form value using == makes total sense.
But then "==" doesn't make sense either because that is a comparison between different objects? A sql mapped class object always have a primary key unless it is comittes to database. If is not yet committed uniqueness is not guaranteed as far as I know.
You could have many objects with the same data (except for primary key) in a database.
I think this is a really hard problem to get right.
it really isn't. == compares values, so if the table row column A has value 1, and the form attribute A has value 1, the == will return True.
Nope, it will be way messier.
If table column A has value 4 and table column B has value "cucumber". Then it will be dependent on how the query looks if both A or B is loaded from database at the same time or not. Lazyloading is also default for relationship loading in sqlalchemy. So there is concurrency issues there as well.
But this doesn't really makes sense here either, since one of the values that should be compared if checking value equality is the primary key. If thats the case then this check should be always be false(the wtform don't have a PK yet). Something is strange here.