biosustain / potion

Flask-Potion is a RESTful API framework for Flask and SQLAlchemy, Peewee or MongoEngine
http://potion.readthedocs.io
Other
487 stars 51 forks source link

Let __init__ of your model get access to all given properties #160

Open ticosax opened 5 years ago

ticosax commented 5 years ago

In some scenario your model might define some default values based on properties given at creation time. This is only possible if __init__ receives all properties at once.

coveralls commented 5 years ago

Coverage Status

Coverage increased (+0.4%) to 89.111% when pulling 7a4996ec02f05ed8c9a5d4eeab4c1438a7701a59 on ticosax:initialize-model-as-python-expect-it into 296d244a18d0a91f46cd8152251ebed67831a7f9 on biosustain:master.

lyschoening commented 5 years ago

Unfortunately, this will not always work as the constructor of a model sometimes has unexpected behavior.

ticosax commented 5 years ago

The current implementation is slower due to iteration of every properties and not very pythonic. If there is an expected way of constructing an instance of a class is by passing all the argument to the constructor. Initializing an instance and then calling setattr for all properties is anti-pattern I would say. Our project overrides the create method on several locations already because of this. The current implementation forces projects to defer from the generic behaviour. I don't know what are your constraints, but potion should not impose this particular case to all projects. The adoption of specific behaviour should be done in specific projects, not for everyone.

lyschoening commented 5 years ago

I agree that it is not ideal, but there are legitimate issues with the other approach that led to this implementation. For one, SQLAlchemy imposes no restrictions on the constructor of a class. An acceptable approach could be calling the __new__() method as SQLAlchemy does internally.

https://docs.sqlalchemy.org/en/latest/orm/constructors.html

Given that this code is only used in a write operation, I don't see speed as the primary concern.

ticosax commented 5 years ago

Even if they claim they impose anything on __init__ they still initiate the attributes of the orm columns through it. When one create a new instance in the desired to insert it in the table. https://github.com/zzzeek/sqlalchemy/blob/master/lib/sqlalchemy/ext/declarative/base.py#L808

It means this is legit to expect __init__ to be called to instantiate a class from the Resource.create method.

SQLAlchemy bypasses __init__ when they load an object from the DB because they don't want to cause side effect added in user's __init__ when an object is simply read form the DB and mapped as a python object. This is 2 different use case in my opinion.