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 deserialisation with defined session and nested field throws an exception #67

Closed
nickw444 opened this issue May 23, 2016 · 14 comments
Closed

Comments

@nickw444
Copy link

@nickw444 nickw444 commented May 23, 2016

Not sure if this is intended functionality, however the code below throws ValueError: Deserialization requires a session when trying to load data into a nested object.

I would expect passing a session would propagate to nested fields, however this doesn't seem to be the case. Is this by design?

If i remove followup (the nested field) from the only= property, the data loads fine from the non-nested field.

class FollowUp(ModelSchema):
    reasonCode =fields.String(attribute='reason_code')
    comment = fields.String()

class Occurence(ModelSchema):
    #... other fields before ...
    followup = fields.Nested(FollowUp())
    status = fields.String()

# Inside my request handler:
obj = DBOccurence.query.get_or_404(occ_id)
res = Occurence(only=['status', 'followup']).load(
            request.json,
            instance=obj,
            session=current_app.db.session
        )
@sloria
Copy link
Member

@sloria sloria commented May 30, 2016

Yes, the intended behavior is that the session propagates to nested fields. What version of marshmallow-sqlalchemy and marshmallow are you using? @nickw444

@nickw444
Copy link
Author

@nickw444 nickw444 commented May 30, 2016

Using:

marshmallow==2.7.3
marshmallow-sqlalchemy==0.8.1

I'll try reproduce this in a standalone flask app

@nickw444
Copy link
Author

@nickw444 nickw444 commented May 30, 2016

Here's a minimal example:

from flask import Flask, request
from marshmallow import fields, validate
from marshmallow_sqlalchemy import ModelSchema
from flask_sqlalchemy import SQLAlchemy

app = Flask(__name__)
db = SQLAlchemy(app)

class NestedModel(db.Model):
    __tablename__ = "nested_model"
    id = db.Column(db.Integer(), primary_key=True)
    field1 = db.Column(db.String(255))
    field2 = db.Column(db.String(255))

class TopLevelModel(db.Model):
    __tablename__ = "top_level_model"
    id = db.Column(db.Integer(), primary_key=True)
    nested_id = db.Column(db.Integer(), db.ForeignKey('nested_model.id'))
    nested = db.relationship('NestedModel')
    field1 = db.Column(db.String(255))

class MyNested(ModelSchema):
    field1 = fields.String()
    field2 = fields.String()

class TopLevel(ModelSchema):
    nested = fields.Nested(MyNested())
    field1 = fields.String()

@app.route('/', methods=['POST'])
def index():
    obj = TopLevelModel()
    res = TopLevel().load(
        request.json,
        instance=obj,
        session=db.session
    )

    print(res)

    print(obj)
    print(obj.nested_id)
    print(obj.nested)

    return ""

if __name__ == '__main__':
    app.run(debug=True)

Posting this payload:

{
  "field1": "Test Field",
  "nested": {
  }
}

Results in this traceback

Traceback (most recent call last):
  File "~~redacted~~/venv/lib/python3.5/site-packages/flask/app.py", line 2000, in __call__
    return self.wsgi_app(environ, start_response)
  File "~~redacted~~/venv/lib/python3.5/site-packages/flask/app.py", line 1991, in wsgi_app
    response = self.make_response(self.handle_exception(e))
  File "~~redacted~~/venv/lib/python3.5/site-packages/flask/app.py", line 1567, in handle_exception
    reraise(exc_type, exc_value, tb)
  File "~~redacted~~/venv/lib/python3.5/site-packages/flask/_compat.py", line 33, in reraise
    raise value
  File "~~redacted~~/venv/lib/python3.5/site-packages/flask/app.py", line 1988, in wsgi_app
    response = self.full_dispatch_request()
  File "~~redacted~~/venv/lib/python3.5/site-packages/flask/app.py", line 1641, in full_dispatch_request
    rv = self.handle_user_exception(e)
  File "~~redacted~~/venv/lib/python3.5/site-packages/flask/app.py", line 1544, in handle_user_exception
    reraise(exc_type, exc_value, tb)
  File "~~redacted~~/venv/lib/python3.5/site-packages/flask/_compat.py", line 33, in reraise
    raise value
  File "~~redacted~~/venv/lib/python3.5/site-packages/flask/app.py", line 1639, in full_dispatch_request
    rv = self.dispatch_request()
  File "~~redacted~~/venv/lib/python3.5/site-packages/flask/app.py", line 1625, in dispatch_request
    return self.view_functions[rule.endpoint](**req.view_args)
  File "~~redacted~~/marshmallow_67.py", line 36, in index
    session=db.session
  File "~~redacted~~/venv/lib/python3.5/site-packages/marshmallow_sqlalchemy/schema.py", line 186, in load
    return super(ModelSchema, self).load(data, *args, **kwargs)
  File "~~redacted~~/venv/lib/python3.5/site-packages/marshmallow/schema.py", line 542, in load
    result, errors = self._do_load(data, many, partial=partial, postprocess=True)
  File "~~redacted~~/venv/lib/python3.5/site-packages/marshmallow/schema.py", line 610, in _do_load
    index_errors=self.opts.index_errors,
  File "~~redacted~~/venv/lib/python3.5/site-packages/marshmallow/marshalling.py", line 294, in deserialize
    index=(index if index_errors else None)
  File "~~redacted~~/venv/lib/python3.5/site-packages/marshmallow/marshalling.py", line 67, in call_and_store
    value = getter_func(data)
  File "~~redacted~~/venv/lib/python3.5/site-packages/marshmallow/marshalling.py", line 287, in <lambda>
    data
  File "~~redacted~~/venv/lib/python3.5/site-packages/marshmallow/fields.py", line 263, in deserialize
    output = self._deserialize(value, attr, data)
  File "~~redacted~~/venv/lib/python3.5/site-packages/marshmallow/fields.py", line 433, in _deserialize
    data, errors = self.schema.load(value)
  File "~~redacted~~/venv/lib/python3.5/site-packages/marshmallow_sqlalchemy/schema.py", line 185, in load
    raise ValueError('Deserialization requires a session')
ValueError: Deserialization requires a session
@gabrielelanaro
Copy link

@gabrielelanaro gabrielelanaro commented Jun 23, 2016

I'm having the same exact problem (same versions), is there any fix for that?

@YuriHeupa
Copy link
Contributor

@YuriHeupa YuriHeupa commented Jul 12, 2016

Also having the same problem, got the latest version, is it still not fixed?

@ewittle
Copy link
Contributor

@ewittle ewittle commented Aug 22, 2016

I'm also having the same problem, would appreciate knowing if this is being fixed.

@nickw444
Copy link
Author

@nickw444 nickw444 commented Aug 23, 2016

I ended up writing this field type to solve the problem: https://github.com/TwoPiCode/marshmallow-sqlalchemy-referential. It makes the interface a bit more restful too - for example, you aren't creating a nested object, but rather creating an association.

The README is a bit bare, but the docstring describes the usage:

For example, on a one to many relationship, on the many side, you may
        want to set the list of related entities.
        To do so, you would specify:
        {
            "myRelatedField": [
                {"id": 1},
                {"id": 2}
            ]
        }
        And be returned a response which resolves to the actual objects:
        {
            "myRelatedField": [
                {
                    "id": 1
                    "name": "foo"
                },
                {
                    "id": 2
                    "name": "bar"
                }
            ]
        }

Obviously the objects you are associating would already need to have been created (maybe through another API endpoint)?

@YKdvd
Copy link

@YKdvd YKdvd commented Nov 14, 2016

It looks like this bug is unresolved? @sloria indicated back on May 30 that a passed session SHOULD propagate to nested fields, but it looks like it DOESN'T, even now, and this has not been addressed?
I've got an odd case where the scoped sessionmaker I need to use varies depending on the request, and passing a session to load() is the way I was going to handle things, which this bug seems to break.
I'll check and see if I can find a way use "sqla_session", but is there any status update on this bug?

@nickw444
Copy link
Author

@nickw444 nickw444 commented Nov 14, 2016

I've been using a workaround by manually de-serialising nested objects, but yes, I agree, this is not an optimal solution.

@YKdvd
Copy link

@YKdvd YKdvd commented Nov 14, 2016

It looks like even if you specify "sqla_session" for the TopLevel schema definition, it doesn't get passed along to the Nested schemas, so Nested() is broken completely except where the nested schema itself specifies a "sqla_session" in its definition?
I took a quick look at the code (schema.py), and it would seem that there is no way that the session could have been intended to propagate to nested schemas? The constructor would have to attach the "session" parm it was passed to any nested mmsqla.ModelSchema fields after the super() call constructed the parent schema. And load() would have to something similar as a one-time thing. I didn't see anything that looked like it could do this.
Also, ModelSchema keeps a "session" attribute which gets set by init() and load(). But load() looks like it doesn't preserve the existing value? So if you had something like this, the second call would wipe out the schema specified in the constructor, and future load() calls will use the last session explicitly passed to load():

#permSession and tempSession are a couple of sqlAlchemy sessions
class MySchema(ModelSchema):
   ...
schema = MySchema(session=permSession)   # always use permSession unless overridden by load()
result = schema.load(data)   # will use permSession
result = schema.load(data, session=tempSession)  # will use tempSession
result = schema.load(data)  # should use permSession, but previous load has set schema.session to tempSession but never restored it to permSession.

I haven't tested this, but scanning the code seems to suggest that's what would happen.

@igortg
Copy link

@igortg igortg commented Jan 16, 2018

What you guys think about the solution presented by @torypages on marshmallow-code/marshmallow#658? Couldn't we just convert the session attribute to a @property and propagate it to the nested fields on the setter? Would a PR using that solution be accepted?

igortg added a commit to ESSS/flask-restalchemy that referenced this issue Feb 21, 2018
First version of flask-rest-orm was using marshmallow-sqlalchemy
library to serialize SQLAlchemy models. Using marshmallow-sqlalchemy
lead to two main problems:

  1. We were doing a lot of workarrounds due the fact that
     marshmallow-sqlalchemy does not support some basic features of
     SQLAlchemy like 'column_properties' and nested objects.

  2. marshmallow-sqlalchemy serializers require a DB Session object
     to work, since they take the responsibility to commit and load
     objects from the DB. But marshmallow-sqlalchemy has some issues
     to access the Session when dealing with nested objects (see
     marshmallow-code/marshmallow-sqlalchemy#67). There is also the
     fact that for a REST library, it is more consistent to do commits
     and loads on the Resource object (on GET and POST methods), and
     use the serializer just to instancialize and jsonify the models.

So this commit replaces the marshmallow-sqlalchemy serializer by our
own implementation in class ModelSerializer. ModelSerializer
automatically inspect SQLAlchemy model columns and serialize to the
correct type. Additional properties could be serialized by extending
ModelSerializer and decliaring Field objects for it. Nested models can
be serialized with NestedModelFields. All features of flask-rest-orm
were preserved.
igortg added a commit to ESSS/flask-restalchemy that referenced this issue Feb 21, 2018
First version of flask-rest-orm was using marshmallow-sqlalchemy
library to serialize SQLAlchemy models. Using marshmallow-sqlalchemy
lead to two main problems:

  1. We were doing a lot of workarrounds due the fact that
     marshmallow-sqlalchemy does not support some basic features of
     SQLAlchemy like 'column_properties' and nested objects.

  2. marshmallow-sqlalchemy serializers require a DB Session object
     to work, since they take the responsibility to commit and load
     objects from the DB. But marshmallow-sqlalchemy has some issues
     to access the Session when dealing with nested objects (see
     marshmallow-code/marshmallow-sqlalchemy#67). There is also the
     fact that for a REST library, it is more consistent to do commits
     and loads on the Resource object (on GET and POST methods), and
     use the serializer just to instancialize and jsonify the models.

So this commit replaces the marshmallow-sqlalchemy serializer by our
own implementation in class ModelSerializer. ModelSerializer
automatically inspect SQLAlchemy model columns and serialize to the
correct type. Additional properties could be serialized by extending
ModelSerializer and decliaring Field objects for it. Nested models can
be serialized with NestedModelFields. All features of flask-rest-orm
were preserved.
@MattF-NSIDC
Copy link

@MattF-NSIDC MattF-NSIDC commented Apr 29, 2019

I managed to implement a fix that builds upon the one linked in the comment above, making it general-purpose (you don't need to know the names of your nested fields to use this):

class BaseModelSchema(ModelSchema):
    OPTIONS_CLASS = BaseOpts

    @mm.pre_load
    def set_nested_session(self, data):
        """Allow nested schemas to use the parent schema's session. This is a
        longstanding bug with marshmallow-sqlalchemy.

        https://github.com/marshmallow-code/marshmallow-sqlalchemy/issues/67
        https://github.com/marshmallow-code/marshmallow/issues/658#issuecomment-328369199
        """
        nested_fields = {k: v for k, v in self.fields.items() if type(v) == mm.fields.Nested}
        for field in nested_fields.values():
            field.schema.session = self.session
@TMiguelT
Copy link

@TMiguelT TMiguelT commented Aug 8, 2019

Okay so to summarize discussions so far, this is still an outstanding issue (in 2019). There are a few workarounds (notably @MattF-NSIDC's one above), but ideally this shouldn't be needed. This issue has been discussed in other projects upstream and downstream of marshmallow-sqlalchemy, but the conclusion is that it needs to be fixed here.

Would it not be reasonable for us to create our own Nested field that inherits from the original Marshmallow Nested, allowing us to pass the session object down? Would this diminish usability in any way?

@sloria
Copy link
Member

@sloria sloria commented Aug 15, 2019

Thanks all for the discussion and the workarounds. I know this is still an issue, and I'm sorry I haven't been able to address this.

I won't have much time to work on this in the near future (busy with work, and also focusing on getting marshmallow 3.0 final out). Would gladly review/merge a PR addressing this issue. Haven't given much thought as to the proper solution, but I think @TMiguelT 's suggestion of a custom Nested field makes sense.

@sloria sloria closed this in #239 Aug 31, 2019
sloria added a commit that referenced this issue Aug 31, 2019
* Allow session to be passed to nested fields.

Fixes #67

* Update tests/test_marshmallow_sqlalchemy.py

Respond to feedback.

Co-Authored-By: Steven Loria <sloria1@gmail.com>

* Allow many (and all args) to be passed through

* Fix formatting; update changelog

* Update recipes to use ma-sqla's Nested
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

9 participants