marshmallow-code / marshmallow-sqlalchemy

SQLAlchemy integration with marshmallow
https://marshmallow-sqlalchemy.readthedocs.io
MIT License
549 stars 94 forks source link

Clarify behaviour of include_relationships when calling dump() on schema? #504

Closed falloutphil closed 1 year ago

falloutphil commented 1 year ago

Hi,

Requirements for reference:

alembic==1.10.2
aniso8601==9.0.1
click==8.1.3
Flask==2.2.3
flask-marshmallow==0.14.0
Flask-Migrate==4.0.4
Flask-RESTful==0.3.9
Flask-Seeder==1.2.0
Flask-SQLAlchemy==3.0.3
greenlet==2.0.2
itsdangerous==2.1.2
Jinja2==3.1.2
Mako==1.2.4
MarkupSafe==2.1.2
marshmallow==3.19.0
marshmallow-sqlalchemy==0.29.0
packaging==23.0
python-dotenv==1.0.0
pytz==2023.2
six==1.16.0
SQLAlchemy==2.0.7
typing_extensions==4.5.0
Werkzeug==2.2.3

If I have a model with a foreign key, my expectation was that include_relationships would mean that any serialization taking place on an instance of my model would nest the model referenced by the foreign key.

However after running some basic examples that doesn't seem to be happening - so my question is what exactly does include_relationships do? In my experiments it seems to behave almost identically to include_fk?

Some details below on what I tried. Perhaps there is some weird interaction with my methodology, or perhaps I've just misunderstood the parameter? Can anyone shed any light?

Thanks!

Helper class so I can set include_relationships everywhere at once (problem occurs without using this but provided for context):

class BaseAutoOpts(SQLAlchemyAutoSchemaOpts):
    def __init__(self, meta, ordered=False):
        if not hasattr(meta, "sqla_session"):
            meta.sqla_session = db.Session
        #if not hasattr(meta, "load_instance"):
        #    meta.load_instance = True
        if not hasattr(meta, "include_relationships"):
            meta.include_relationships = True
        if not hasattr(meta, "include_fk"):
            meta.include_fk = True
        super(BaseAutoOpts, self).__init__(meta, ordered=ordered)

Simple as possible schema:

class QuoteSchema(BaseAutoSchema):
    class Meta:
        model = Quote

Model referenced in the Schema - contains one foreign key:

class Quote(db.Model):
    id = db.Column(db.Integer, primary_key=True)
    user_id = db.Column(db.Integer, db.ForeignKey("user.id"))
    user = db.relationship("User")

Model referenced by the foreign key which isn't serialised in the output below.

class User(db.Model):
    id = db.Column(db.Integer, primary_key=True)
    username = db.Column(db.String(12), nullable=False, unique=True)
    fullname = db.Column(db.String(60), nullable=False)

For some context this is how I serialise the object using dump():

class QuoteResource(Resource):
     def get(self, quote_id=None):
        quote_schema = QuoteSchema()
        if quote_id:
            quote = Quote.query.get_or_404(quote_id)
            print(quote)
            return quote_schema.dump(quote)

In the helper class (or setting it directly under QuoteSchema's Meta - it makes no difference), setting include_relationships=True results in the following dump - notice user is not showing nested objects, instead it is just setting the the foreign key id representing the nested object.

I kinda expected that include_relationships would toggle if the referenced model was nested in the serialized object?

{
    "user": 2,
    "id": 1,
}

Set include_relationships=False results in the following dump - this is as expected; only Quote's fields are in the output:

{
    "id": 1
}

However if also set include_fk, I just get the user_id key which duplicates to the value of the user key:

{
    "id": 1,
    "user": 2,
    "user_id": 2,
}

Finally note I am able to get more or less the behaviour I thought I'd get for free using this in my QuoteSchema, but it seems like I might be re-inventing the wheel?

user = fields.Nested(UserSchema) if Meta.include_relationships else auto_field("user")
falloutphil commented 1 year ago

Quick additon - #292 seems to ask the same question, and the response seems to line up with my final suggestion of using fields.Nested as a workaround (but without the conditional test which is wrong). Also it looks like I've misunderstood the original justification which is to set include_relationships to False, specifically to avoid it duplicating the values already providied by include_fk? It was never designed to include auto generated nested schemas in the dumped JSON, nor is such a thing possible currently by any other means. This is covered fairly conclusively in #98, I think.

Unless anyone comments otherwise (i.e. my summary above is wrong!) I'll close this shortly - I note in the issues above at least a couple of other people seem to have made similar incorrect assumptions to me. It might be worth making the context and purpose of include_relationships setting a bit more explicit in the otherwise good documentation on the library, in case a few other people have fallen into the same trap as me! I can raise another ticket for that if people think it is worthwhile?