From e3cde8b6e906761cf972a49cdcc77d1ec30849a0 Mon Sep 17 00:00:00 2001 From: Michael Milton Date: Mon, 21 Oct 2019 14:48:13 +1100 Subject: [PATCH 1/5] Implement combine_opts for schemas --- src/marshmallow/schema.py | 24 ++++++++++++---- tests/test_schema.py | 58 +++++++++++++++++++++++++++++++++------ 2 files changed, 68 insertions(+), 14 deletions(-) diff --git a/src/marshmallow/schema.py b/src/marshmallow/schema.py index 7265514fe..89f6d5915 100644 --- a/src/marshmallow/schema.py +++ b/src/marshmallow/schema.py @@ -89,8 +89,12 @@ class SchemaMeta(type): names to field objects. Also sets the ``opts`` class attribute, which is the Schema class's ``class Meta`` options. """ + def __new__(mcs, name, bases, attrs, combine_opts=False): + """ + :param combine_opts: If true, create a new SchemaOpts and Meta subclass from both parents + """ - def __new__(mcs, name, bases, attrs): + # Collect fields and construct the class meta = attrs.get("Meta") ordered = getattr(meta, "ordered", False) if not ordered: @@ -108,10 +112,20 @@ def __new__(mcs, name, bases, attrs): klass = super().__new__(mcs, name, bases, attrs) inherited_fields = _get_fields_by_mro(klass, base.FieldABC, ordered=ordered) + # Instantiate the options class using class Meta meta = klass.Meta - # Set klass.opts in __new__ rather than __init__ so that it is accessible in - # get_declared_fields - klass.opts = klass.OPTIONS_CLASS(meta, ordered=ordered) + if combine_opts and len(bases) > 1: + # Create a new class Meta that combines all the base classes + # meta = type(name + 'Meta', tuple(set([base.Meta for base in bases])), {}) + # Create a new options class that combines all the base classes + combined_opts = type(name + 'Opts', tuple(set([base.OPTIONS_CLASS for base in bases])), {}) + # Instantiate the options class as we normally do + klass.opts = combined_opts(meta, ordered=ordered) + else: + # Set klass.opts in __new__ rather than __init__ so that it is accessible in + # get_declared_fields + klass.opts = klass.OPTIONS_CLASS(meta, ordered=ordered) + # Add fields specified in the `include` class Meta option cls_fields += list(klass.opts.include.items()) @@ -146,7 +160,7 @@ def get_declared_fields( """ return dict_cls(inherited_fields + cls_fields) - def __init__(cls, name, bases, attrs): + def __init__(cls, name, bases, attrs, combine_opts=False): super().__init__(name, bases, attrs) if name and cls.opts.register: class_registry.register(name, cls) diff --git a/tests/test_schema.py b/tests/test_schema.py index 2be35b775..b3649f851 100644 --- a/tests/test_schema.py +++ b/tests/test_schema.py @@ -18,6 +18,7 @@ INCLUDE, RAISE, class_registry, + SchemaOpts ) from marshmallow.exceptions import ( ValidationError, @@ -44,9 +45,9 @@ Blog, ) - random.seed(1) + # Run tests with both verbose serializer and 'meta' option serializer @pytest.mark.parametrize("SchemaClass", [UserSchema, UserMetaSchema]) def test_serializing_basic_object(SchemaClass, user): @@ -637,7 +638,6 @@ def test_invalid_dict_but_okay(): def test_json_module_is_deprecated(): with pytest.deprecated_call(): - class UserJSONSchema(Schema): name = fields.String() @@ -780,7 +780,6 @@ class ErrorSchema(Schema): def test_error_raised_if_fields_option_is_not_list(): with pytest.raises(ValueError): - class BadSchema(Schema): name = fields.String() @@ -790,7 +789,6 @@ class Meta: def test_error_raised_if_additional_option_is_not_list(): with pytest.raises(ValueError): - class BadSchema(Schema): name = fields.String() @@ -1583,8 +1581,8 @@ def test_meta_fields_mapping(user): assert type(s.fields["time_registered"]._field_cache[fields.Time]) == fields.Time assert type(s.fields["birthdate"]._field_cache[fields.Date]) == fields.Date assert ( - type(s.fields["since_created"]._field_cache[fields.TimeDelta]) - == fields.TimeDelta + type(s.fields["since_created"]._field_cache[fields.TimeDelta]) + == fields.TimeDelta ) @@ -1607,7 +1605,6 @@ def test_exclude_fields(user): def test_fields_option_must_be_list_or_tuple(): with pytest.raises(ValueError): - class BadFields(Schema): class Meta: fields = "name" @@ -1615,7 +1612,6 @@ class Meta: def test_exclude_option_must_be_list_or_tuple(): with pytest.raises(ValueError): - class BadExclude(Schema): class Meta: exclude = "name" @@ -1697,7 +1693,6 @@ def test_additional(user): def test_cant_set_both_additional_and_fields(user): with pytest.raises(ValueError): - class BadSchema(Schema): name = fields.String() @@ -2494,6 +2489,7 @@ def test_inheritance_follows_mro(self): ("field_d", fields.String()), ] ) + # Diamond inheritance graph # MRO: D -> B -> C -> A @@ -2766,3 +2762,47 @@ class Meta: dumped = OSchema().dump({"foo": 42, "bar": 24}) assert isinstance(dumped, OrderedDict) assert "bar" not in dumped + + +class TestCombineOpts: + def test_basic(self): + """ + Create two options classes, each belonging to a schema, then combine the two schemas using inheritance + When combine_opts == True, we should have both options being stored in self.opts. Otherwise, we won't get any + """ + + class Opts1(SchemaOpts): + def __init__(self, meta, **kwargs): + super().__init__(meta, **kwargs) + self.opt_1 = getattr(meta, "opt_1", None) + + class Schema1(Schema): + OPTIONS_CLASS = Opts1 + Field1 = fields.String() + + class Opts2(SchemaOpts): + def __init__(self, meta, **kwargs): + super().__init__(meta, **kwargs) + self.opt_2 = getattr(meta, "opt_2", None) + + class Schema2(Schema): + OPTIONS_CLASS = Opts2 + Field2 = fields.String() + + class NotCombined(Schema1, Schema2, combine_opts=False): + class Meta: + opt_1 = True + opt_2 = True + + class Combined(Schema1, Schema2, combine_opts=True): + class Meta: + opt_1 = True + opt_2 = True + + # Without using combine_opts, we should only get options defined by the first options class + assert NotCombined().opts.opt_1 + assert not hasattr(NotCombined().opts, 'opt_2') + + # However when we use combine_opts=True, we should get options defined by both options classes + assert Combined().opts.opt_1 + assert Combined().opts.opt_2 From 326e7b88f39d3efca481166275c832933527a161 Mon Sep 17 00:00:00 2001 From: Michael Milton Date: Mon, 21 Oct 2019 14:48:31 +1100 Subject: [PATCH 2/5] Remove dead code --- src/marshmallow/schema.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/marshmallow/schema.py b/src/marshmallow/schema.py index 89f6d5915..0a47516a1 100644 --- a/src/marshmallow/schema.py +++ b/src/marshmallow/schema.py @@ -115,8 +115,6 @@ def __new__(mcs, name, bases, attrs, combine_opts=False): # Instantiate the options class using class Meta meta = klass.Meta if combine_opts and len(bases) > 1: - # Create a new class Meta that combines all the base classes - # meta = type(name + 'Meta', tuple(set([base.Meta for base in bases])), {}) # Create a new options class that combines all the base classes combined_opts = type(name + 'Opts', tuple(set([base.OPTIONS_CLASS for base in bases])), {}) # Instantiate the options class as we normally do From 8140003d8c8ca0e206f949d7349c62c5a0c8deb3 Mon Sep 17 00:00:00 2001 From: Michael Milton Date: Mon, 21 Oct 2019 14:52:38 +1100 Subject: [PATCH 3/5] Clean up whitespace --- tests/test_schema.py | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/tests/test_schema.py b/tests/test_schema.py index b3649f851..e65b31ad6 100644 --- a/tests/test_schema.py +++ b/tests/test_schema.py @@ -45,8 +45,8 @@ Blog, ) -random.seed(1) +random.seed(1) # Run tests with both verbose serializer and 'meta' option serializer @pytest.mark.parametrize("SchemaClass", [UserSchema, UserMetaSchema]) @@ -638,6 +638,7 @@ def test_invalid_dict_but_okay(): def test_json_module_is_deprecated(): with pytest.deprecated_call(): + class UserJSONSchema(Schema): name = fields.String() @@ -780,6 +781,7 @@ class ErrorSchema(Schema): def test_error_raised_if_fields_option_is_not_list(): with pytest.raises(ValueError): + class BadSchema(Schema): name = fields.String() @@ -789,6 +791,7 @@ class Meta: def test_error_raised_if_additional_option_is_not_list(): with pytest.raises(ValueError): + class BadSchema(Schema): name = fields.String() @@ -1581,8 +1584,8 @@ def test_meta_fields_mapping(user): assert type(s.fields["time_registered"]._field_cache[fields.Time]) == fields.Time assert type(s.fields["birthdate"]._field_cache[fields.Date]) == fields.Date assert ( - type(s.fields["since_created"]._field_cache[fields.TimeDelta]) - == fields.TimeDelta + type(s.fields["since_created"]._field_cache[fields.TimeDelta]) + == fields.TimeDelta ) @@ -1605,6 +1608,7 @@ def test_exclude_fields(user): def test_fields_option_must_be_list_or_tuple(): with pytest.raises(ValueError): + class BadFields(Schema): class Meta: fields = "name" @@ -1612,6 +1616,7 @@ class Meta: def test_exclude_option_must_be_list_or_tuple(): with pytest.raises(ValueError): + class BadExclude(Schema): class Meta: exclude = "name" @@ -1693,6 +1698,7 @@ def test_additional(user): def test_cant_set_both_additional_and_fields(user): with pytest.raises(ValueError): + class BadSchema(Schema): name = fields.String() @@ -2489,7 +2495,6 @@ def test_inheritance_follows_mro(self): ("field_d", fields.String()), ] ) - # Diamond inheritance graph # MRO: D -> B -> C -> A From b2ef0f130450f34e7b73a02071e547734146113a Mon Sep 17 00:00:00 2001 From: Michael Milton Date: Mon, 21 Oct 2019 15:06:45 +1100 Subject: [PATCH 4/5] Make pre-commit happy --- src/marshmallow/schema.py | 5 ++++- tests/test_schema.py | 16 ++++++++++------ 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/src/marshmallow/schema.py b/src/marshmallow/schema.py index 0a47516a1..92eb82997 100644 --- a/src/marshmallow/schema.py +++ b/src/marshmallow/schema.py @@ -89,6 +89,7 @@ class SchemaMeta(type): names to field objects. Also sets the ``opts`` class attribute, which is the Schema class's ``class Meta`` options. """ + def __new__(mcs, name, bases, attrs, combine_opts=False): """ :param combine_opts: If true, create a new SchemaOpts and Meta subclass from both parents @@ -116,7 +117,9 @@ def __new__(mcs, name, bases, attrs, combine_opts=False): meta = klass.Meta if combine_opts and len(bases) > 1: # Create a new options class that combines all the base classes - combined_opts = type(name + 'Opts', tuple(set([base.OPTIONS_CLASS for base in bases])), {}) + combined_opts = type( + name + "Opts", tuple({base.OPTIONS_CLASS for base in bases}), {} + ) # Instantiate the options class as we normally do klass.opts = combined_opts(meta, ordered=ordered) else: diff --git a/tests/test_schema.py b/tests/test_schema.py index e65b31ad6..15f724803 100644 --- a/tests/test_schema.py +++ b/tests/test_schema.py @@ -18,7 +18,7 @@ INCLUDE, RAISE, class_registry, - SchemaOpts + SchemaOpts, ) from marshmallow.exceptions import ( ValidationError, @@ -2772,8 +2772,10 @@ class Meta: class TestCombineOpts: def test_basic(self): """ - Create two options classes, each belonging to a schema, then combine the two schemas using inheritance - When combine_opts == True, we should have both options being stored in self.opts. Otherwise, we won't get any + Create two options classes, each belonging to a schema, then combine the two + schemas using inheritance. When combine_opts == True, we should have both options + being stored in self.opts. Otherwise, we should only get options defined in the + first options class stored in self.opts. """ class Opts1(SchemaOpts): @@ -2804,10 +2806,12 @@ class Meta: opt_1 = True opt_2 = True - # Without using combine_opts, we should only get options defined by the first options class + # Without using combine_opts, we should only get options defined by the first + # options class assert NotCombined().opts.opt_1 - assert not hasattr(NotCombined().opts, 'opt_2') + assert not hasattr(NotCombined().opts, "opt_2") - # However when we use combine_opts=True, we should get options defined by both options classes + # However when we use combine_opts=True, we should get options defined by both + # options classes assert Combined().opts.opt_1 assert Combined().opts.opt_2 From 612798f11d611bb6d051931b3458511176a1612e Mon Sep 17 00:00:00 2001 From: Michael Milton Date: Mon, 21 Oct 2019 15:28:18 +1100 Subject: [PATCH 5/5] Add test for duplicate options classes --- tests/test_schema.py | 30 +++++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/tests/test_schema.py b/tests/test_schema.py index 15f724803..686e3c495 100644 --- a/tests/test_schema.py +++ b/tests/test_schema.py @@ -2770,7 +2770,7 @@ class Meta: class TestCombineOpts: - def test_basic(self): + def test_different_opts(self): """ Create two options classes, each belonging to a schema, then combine the two schemas using inheritance. When combine_opts == True, we should have both options @@ -2815,3 +2815,31 @@ class Meta: # options classes assert Combined().opts.opt_1 assert Combined().opts.opt_2 + + def test_same_opts(self): + """ + Ensure this behaviour still works if the two parent schemas have the same opts + class + """ + + class Opts1(SchemaOpts): + def __init__(self, meta, **kwargs): + super().__init__(meta, **kwargs) + self.opt_1 = getattr(meta, "opt_1", None) + + class Schema1(Schema): + OPTIONS_CLASS = Opts1 + Field1 = fields.String() + + class Schema2(Schema): + OPTIONS_CLASS = Opts1 + Field2 = fields.String() + + class Combined(Schema1, Schema2, combine_opts=True): + class Meta: + opt_1 = True + opt_2 = True + + # However when we use combine_opts=True, we should get options defined by both + # options classes + assert Combined().opts.opt_1