marshmallow icon indicating copy to clipboard operation
marshmallow copied to clipboard

[RFC] Merge child Schema Meta with inherited parent Meta

Open lafrech opened this issue 2 years ago • 1 comments

@sloria commented:

Should we also "auto-inherit" Meta even when doing single inheritance?

class MyBaseSchema(Schema):
    class Meta:
        render_module = ujson

class ArtistSchema(MyBaseSchema):
    class Meta:  # implicitly inherits MyBaseSchema.Meta
        ordered = True

My apps generally define a base schema and it is a PITA to have to explicitly inherit BaseSchema.Meta. Plus I generally forget to do it as I wrongly assume Meta is inherited already. I just got hit by this again.

Which behaviour is the most intuitive is arguable. Current behaviour of not merging might be the most Pythonic. But since those are not really classes but merely a place to store attributes, we're free to do what we think is the most sensible and useful.

lafrech avatar Mar 29 '22 13:03 lafrech

Looking at it a little deeper with a marshmallow-sqlalchemy example, there might be side effects to consider.

class AutoSchema(SQLAlchemyAutoSchema):
    class Meta:
        include_fk = True  # This is the part I'd like to inherit everywhere


class BuildingSchema(AutoSchema):
    class Meta(AutoSchema.Meta):  # I must inherit explicitly, and sometimes I forget
        table = Building.__table__
        exclude = ("country_id",)  # Excluding an auto-created field

    id = msa.auto_field(dump_only=True)


class BuildingPutSchema(BuildingSchema):
    class Meta:
        exclude = ("site_id",)  # Excluding another field

This works. Note that I override Meta in the last schema without any merge/inheritance. The last schema contains neither country_id nor site_id so it looks like country_id is not there because excluded fields are not inherited in normal marshmallow inheritance which is what I expected, although thinking of it it is not that obvious.

Now if we had auto-merge inheritance, it would be equivalent to:

class BuildingPutSchema(BuildingSchema):
    class Meta(BuildingSchema.Meta):  # Inheritance again
        # The following are inherited:
        # include_fk = True
        # table = Building.__table__
        # exclude = ("country_id",)
        exclude = ("site_id",)  # Overrides above line

This is wrong because this time country_id field is not excluded and it is created again (because we inherit table, I guess).

I should have written

class BuildingPutSchema(BuildingSchema):
    class Meta(BuildingSchema.Meta):
        exclude = BuildingSchema.Meta.exclude + ("site_id",)
        # One may prefer the following
        # exclude = ("country_id", "site_id",)

The more I look at it, the more I think this is out of topic as the root cause is that when inheriting Meta one should always be careful about overriding exclude vs. extending it.

We could be tempted to apply the same logic to exclude (i.e. auto-merge inherit) but I'd rather not engage into this.

Overall, I think auto-merging inherited Meta would be an improvement. Of course it would be a breaking change (marshmallow 4 feature).

lafrech avatar Mar 29 '22 13:03 lafrech