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

[RFC] Merge child Schema Meta with inherited parent Meta #1969

Open
lafrech opened this issue Mar 29, 2022 · 1 comment
Open

[RFC] Merge child Schema Meta with inherited parent Meta #1969

lafrech opened this issue Mar 29, 2022 · 1 comment

Comments

@lafrech
Copy link
Member

lafrech commented Mar 29, 2022

@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 lafrech added this to the 4.0 milestone Mar 29, 2022
@lafrech
Copy link
Member Author

lafrech commented Mar 29, 2022

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 lafrech modified the milestones: 4.0, 5.0 Dec 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant