Repository: packit-service/packit
Pull request #854 (2020-06-02 17:23:14)

Allow inheriting from MM23Schema for each config schema
In packit-service I need to dump and load only the JobConfig, but the JobConfigSchema did not inherit from MM23Schema, so it was not compatible with Marshmallow 2. When the JobConfigSchema inherits from MM23Schema and it overrides  `dump` and `load` methods with the same names, then when the PackageConfig is loaded, it is not working, because when getting nested JobConfig, our overridden methods are called and in Marshmallow 2 it causes problems.  Therefore I used different names for the `dump` and `load` method (then when getting nested JobConfig, Marshmallow manages dumping and loading itself and does not call our overridden methods).

packit-as-a-service-stg[bot]: Congratulations! One of the builds has completed. :champagne:

You can install the built RPMs by following these steps:

* `sudo yum install -y dnf-plugins-core` on RHEL 8
* `sudo dnf install -y dnf-plugins-core` on Fedora
* `dnf copr enable packit/packit-service-packit-854-stg`
* And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.
softwarefactory-project-zuul[bot]: Build succeeded.

- [packit-pre-commit ](https://softwarefactory-project.io/zuul/t/packit-service/build/bb03273c0a0a4014b571c8783378371a) : SUCCESS in 2m 34s
- [packit-tests-rpm ](https://softwarefactory-project.io/zuul/t/packit-service/build/648a31174269448fa3a0dd6391c37a2d) : SUCCESS in 7m 46s
- [packit-tests-pip-deps ](https://softwarefactory-project.io/zuul/t/packit-service/build/53b91a903d0e4c5b9c5fe1cde772c650) : SUCCESS in 8m 32s
- [packit-tests-git-master ](https://softwarefactory-project.io/zuul/t/packit-service/build/7b55022899dc40a2a80ec4acee9a4e0e) : SUCCESS in 7m 22s
- [packit-tests-rpm-sess-rec ](https://softwarefactory-project.io/zuul/t/packit-service/build/016f891935df4bf1be2dfff912e8829b) : SUCCESS in 2m 05s
- [packit-tests-pip-deps-sess-rec ](https://softwarefactory-project.io/zuul/t/packit-service/build/660e4f5fc9e34cd7895fdbb824bf8551) : SUCCESS in 2m 43s
- [packit-tests-git-master-sess-rec ](https://softwarefactory-project.io/zuul/t/packit-service/build/d653822fe772487fb5e1d7dec6c657ea) : SUCCESS in 3m 00s
- [reverse-dep-packit-service-tests ](https://softwarefactory-project.io/zuul/t/packit-service/build/a2628d86466c479398496660f752307a) : SUCCESS in 4m 17s

csomh: > ... our overridden methods are called and in Marshmallow 2 it causes problems.

What are these problems? 

lbarcziova: > What are there problems?

As the load/dump method in Marshmallow 2 returns tuple (data, errors), it is probably getting the data internally from the nested schema and then in our function it tries again to unpack the data (in tests I  got `VallueError: too many values to unpack`)

Patch:
diff --git a/packit/config/config.py b/packit/config/config.py
index df77a0d3..f54a11dc 100644
--- a/packit/config/config.py
+++ b/packit/config/config.py
@@ -137,7 +137,7 @@ class Config:
         # required to avoid cyclical imports
         from packit.schema import UserConfigSchema
 
-        config = UserConfigSchema().load(raw_dict)
+        config = UserConfigSchema().load_config(raw_dict)
         logger.debug(f"Loaded config: {config}")
 
         return config
diff --git a/packit/config/job_config.py b/packit/config/job_config.py
index c4befbae..b7dd1ab7 100644
--- a/packit/config/job_config.py
+++ b/packit/config/job_config.py
@@ -132,12 +132,9 @@ class JobConfig:
     @classmethod
     def get_from_dict(cls, raw_dict: dict) -> "JobConfig":
         # required to avoid cyclical imports
-        from packit.schema import JobConfigSchema, MM3
+        from packit.schema import JobConfigSchema
 
-        if MM3:
-            config = JobConfigSchema().load(raw_dict)
-        else:  # v2
-            config = JobConfigSchema(strict=True).load(raw_dict).data
+        config = JobConfigSchema().load_config(raw_dict)
         logger.debug(f"Loaded config: {config}")
 
         return config
diff --git a/packit/config/package_config.py b/packit/config/package_config.py
index 0921f95e..35eef8cf 100644
--- a/packit/config/package_config.py
+++ b/packit/config/package_config.py
@@ -184,7 +184,7 @@ class PackageConfig:
         if config_file_path and not raw_dict.get("config_file_path", None):
             raw_dict.update(config_file_path=config_file_path)
 
-        package_config = PackageConfigSchema().load(raw_dict)
+        package_config = PackageConfigSchema().load_config(raw_dict)
 
         if not getattr(package_config, "specfile_path", None):
             if spec_file_path:
diff --git a/packit/config/sync_files_config.py b/packit/config/sync_files_config.py
index 0f7c0ed4..09c24de9 100644
--- a/packit/config/sync_files_config.py
+++ b/packit/config/sync_files_config.py
@@ -50,12 +50,9 @@ class SyncFilesConfig:
     @classmethod
     def get_from_dict(cls, raw_dict: dict) -> "SyncFilesConfig":
         # required to avoid cyclical imports
-        from packit.schema import SyncFilesConfigSchema, MM3
+        from packit.schema import SyncFilesConfigSchema
 
-        if MM3:
-            config = SyncFilesConfigSchema().load(raw_dict)
-        else:  # v2
-            config = SyncFilesConfigSchema(strict=True).load(raw_dict).data
+        config = SyncFilesConfigSchema().load_config(raw_dict)
         logger.debug(f"Loaded config: {config}")
 
         return config
diff --git a/packit/schema.py b/packit/schema.py
index 9527cc51..c7b90420 100644
--- a/packit/schema.py
+++ b/packit/schema.py
@@ -155,14 +155,14 @@ class MM23Schema(Schema):
         else:  # v2
             super().__init__(strict=True, **kwargs)
 
-    def load(self, *args, **kwargs):
+    def load_config(self, *args, **kwargs):
         if MM3:
             result = super().load(*args, **kwargs)
         else:  # v2
             result = super().load(*args, **kwargs).data
         return result
 
-    def dump(self, *args, **kwargs):
+    def dump_config(self, *args, **kwargs):
         if MM3:
             result = super().dump(*args, **kwargs)
         else:  # v2
@@ -174,8 +174,7 @@ class MM23Schema(Schema):
         return {key: value for key, value in data.items() if value is not None}
 
 
-# TODO: inherit from MM23Schema
-class SyncFilesConfigSchema(Schema):
+class SyncFilesConfigSchema(MM23Schema):
     """
     Schema for processing SyncFilesConfig config data.
     """
@@ -202,7 +201,7 @@ class SyncFilesConfigSchema(Schema):
             return data
 
 
-class JobMetadataSchema(Schema):
+class JobMetadataSchema(MM23Schema):
     """ Jobs metadata. """
 
     targets = fields.List(fields.String())
@@ -232,13 +231,8 @@ class JobMetadataSchema(Schema):
     def make_instance(self, data, **_):
         return JobMetadataConfig(**data)
 
-    @post_dump
-    def remove_none_values(self, data, **kwargs):
-        return {key: value for key, value in data.items() if value is not None}
-
 
-# TODO: inherit from MM23Schema
-class JobConfigSchema(Schema):
+class JobConfigSchema(MM23Schema):
     """
     Schema for processing JobConfig config data.
     """
@@ -253,7 +247,7 @@ class JobConfigSchema(Schema):
         return JobConfig(**data)
 
 
-class PullRequestNotificationsSchema(Schema):
+class PullRequestNotificationsSchema(MM23Schema):
     """ Configuration of commenting on pull requests. """
 
     successful_build = fields.Bool(default=True)
@@ -263,7 +257,7 @@ class PullRequestNotificationsSchema(Schema):
         return PullRequestNotificationsConfig(**data)
 
 
-class NotificationsSchema(Schema):
+class NotificationsSchema(MM23Schema):
     """ Configuration of notifications. """
 
     pull_request = fields.Nested(PullRequestNotificationsSchema)


Test patch:
diff --git a/tests/unit/test_config.py b/tests/unit/test_config.py
index 0899cb67..349769d3 100644
--- a/tests/unit/test_config.py
+++ b/tests/unit/test_config.py
@@ -35,6 +35,7 @@ from packit.config import (
     JobConfigTriggerType,
 )
 from packit.config.job_config import JobMetadataConfig
+from packit.schema import JobConfigSchema
 
 
 def get_job_config_dict_simple():
@@ -211,3 +212,13 @@ def test_user_config_fork_token(tmpdir, recwarn):
     Config.get_user_config()
     w = recwarn.pop(UserWarning)
     assert "pagure_fork_token" in str(w.message)
+
+
+@pytest.mark.parametrize(
+    "config", [get_job_config_simple(), get_job_config_full()],
+)
+def test_serialize_and_deserialize_job_config(config):
+    schema = JobConfigSchema()
+    serialized = schema.dump_config(config)
+    new_config = schema.load_config(serialized)
+    assert new_config == config
diff --git a/tests/unit/test_package_config.py b/tests/unit/test_package_config.py
index 673cd08a..75155773 100644
--- a/tests/unit/test_package_config.py
+++ b/tests/unit/test_package_config.py
@@ -929,6 +929,6 @@ def test_get_local_specfile_path():
 )
 def test_serialize_and_deserialize(package_config):
     schema = PackageConfigSchema()
-    serialized = schema.dump(package_config)
-    new_package_config = schema.load(serialized)
+    serialized = schema.dump_config(package_config)
+    new_package_config = schema.load_config(serialized)
     assert package_config == new_package_config