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

schema.load is no longer taking into consideration the Models default values #47

Closed
AdrielVelazquez opened this issue Dec 9, 2015 · 6 comments

Comments

@AdrielVelazquez
Copy link

commented Dec 9, 2015

As of today 12/9 loading a dictionary to a model that has default fields is no longer using the alchemy default fields and throwing errors.

Example below, Python 3

from datetime import datetime

from app import db
from app.models.base import Base as SQLBASE
from app.schemas import Base

__all__ = ["TestModel"]

class TestModel(SQLBASE):
    __tablename__ = 'test_table'
    id = db.Column(db.Integer, primary_key=True)
    name = db.Column(db.Unicode(255), nullable=False, unique=True)
    created_at = db.Column(db.DateTime, nullable=False, default=datetime.utcnow)
    updated_at = db.Column(db.DateTime, nullable=False, default=datetime.utcnow)

class TestModelSchema(Base):

    class Meta(Base.Meta):
        dateformat = "%Y-%m-%d"
        model = TestModel

test_schema = TestModelSchema()

class TestSchemaFunctions:

    def test_no_defaults(self):
        dict_model = {"name": "TestName"}
        results = test_schema.load(dict_model)
        print(results.errors, results.data)
        # (Pdb) results.errors
        # {'updated_at': ['Missing data for required field.'], 'created_at': ['Missing data for required field.'], 'id': ['Missing data for required field.']}
        # (Pdb) results.data
        # {'name': 'TestName'}
@AdrielVelazquez

This comment has been minimized.

Copy link
Author

commented Dec 9, 2015

I should note that passing the partial flag now allows it to work.

Is that new?

from datetime import datetime

from app import db
from app.models.base import Base as SQLBASE
from app.schemas import Base

__all__ = ["TestModel"]

class TestModel(SQLBASE):
    __tablename__ = 'test_table'
    id = db.Column(db.Integer, primary_key=True)
    name = db.Column(db.Unicode(255), nullable=False, unique=True)
    created_at = db.Column(db.DateTime, nullable=False, default=datetime.utcnow)
    updated_at = db.Column(db.DateTime, nullable=False, default=datetime.utcnow)

class TestModelSchema(Base):

    '''
    Serializer for Advertiser

    .. seealso: Advertiser Model for all acceptable params
    '''
    class Meta(Base.Meta):
        dateformat = "%Y-%m-%d"
        model = TestModel

test_schema = TestModelSchema()

class TestSchemaFunctions:

    def test_no_defaults(self):
        dict_model = {"name": "TestName"}
        results = test_schema.load(dict_model, partial=True)
        print(results.errors, results.data)
@jmcarp

This comment has been minimized.

Copy link
Contributor

commented Dec 11, 2015

As of release 0.7, columns that aren't nullable are translated to fields that are required. One solution to this issue would be to relax that constraint for columns with default values. Alternatively, we could also try to use the default value on deserialization, but default values in SQLAlchemy are allowed to access the context of the current row and to call SQL functions, both of which would be hard to introspect.

What do you all think @AdrielVelazquez @sloria?

@AdrielVelazquez

This comment has been minimized.

Copy link
Author

commented Dec 12, 2015

@jmcarp I understand the reasoning; and we were able to switch the models that have defaults to not have nullable=False.

However, there's another bug. If you have a model with the a column that's primary_key and auto_increment=True. Marshmallow still throws an error when not receiving an id.

This is definitely a bug that anything considered a primary key with auto_increment=True should not be defaulted to required cause an id will never be passed.

Thoughts?

@jmcarp

This comment has been minimized.

Copy link
Contributor

commented Dec 12, 2015

So it seems like there are several cases in which a non-nullable column shouldn't be required: if it has a default value, or server_default, or autoincrement. How about changing our logic so that non-nullable columns are required, unless any of the above attributes is set?

@AdrielVelazquez

This comment has been minimized.

Copy link
Author

commented Dec 12, 2015

Sounds good.

jmcarp added a commit to jmcarp/marshmallow-sqlalchemy that referenced this issue Dec 12, 2015

Require non-nullable columns without default values.
Currently, we require values for all non-nullable columns to be set.
However, as pointed out by @AdrielVelazquez, this leads to incorrect
behavior for non-nullable columns with default values. This patch sets
columns as required only when non-nullable and no default value is set,
either using the `default` or `server_default` settings, or if the
column uses `auto_increment`.

[Resolves marshmallow-code#47]
@jmcarp

This comment has been minimized.

Copy link
Contributor

commented Dec 12, 2015

I took a stab at this in #48. Feedback welcome @AdrielVelazquez @sloria

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.