Repository: collective/icalendar

Issue:
[BUG] Component._encode ignores parameters if value is native
<!-- This template is there to guide you and help us. If you can not complete everything here, that is fine. -->
## Describe the bug
<!-- A clear and concise description of what the bug is. -->
`Component._encode` ignores the given parameters if the value is already of a native type (see [code in question](https://github.com/collective/icalendar/blob/df769f88694745d3babb87457bc3080965aa62c4/src/icalendar/cal.py#L139-L141)). IMO this is unexpected behavior. This also seems unexpected for the dependent library caldav, where it leads to a RELTYPE to be fully ignored if the passed UID is of type vText (see python-caldav/caldav#334).

## To Reproduce
<!-- Please add the neccesary code here to reproduce the problem on your machine. -->
(I’m not familiar with this library as I only discovered this issue through discovering python-caldav/caldav#334, so the reproducing code is not fully complete.)

```
import icalendar

todo_1  # assume minimal working vTODO
todo_2  # assume other minimal working vTODO

todo_2.add(
    "RELATES-TO",
    todo_1["uid"],
    parameters={"RELTYPE": "DEPENDS-ON"},
)
print(todo_2.to_ical())
```

In the output, the `RELATES-TO` is added, as specified.
However, its missing the parameter `RELTYPE=DEPENDS-ON`.

## Expected behavior

Use the specified parameters (replace or append already existing ones on the native type) or, at least, throw a warning or error that parameter will be ignored.

## Environment
<!-- please complete the following information: -->
 - [x] OS: Debian GNU/Linux trixie/sid
 - [x] Python version: Python 3.11.5
 - [x] `icalendar` version: 5.0.8

## Additional context
<!-- Add any other context about the problem here, related issues and pull requests. -->
- [x] I tested it with the latest version `pip3 install https://github.com/collective/icalendar.git`
- [x] I attached the ICS source file or there is no ICS source file


Discussion:

Patch:
diff --git a/CHANGES.rst b/CHANGES.rst
index 2d81830..0ae1673 100644
--- a/CHANGES.rst
+++ b/CHANGES.rst
@@ -18,6 +18,9 @@ New features:
 
 Bug fixes:
 
+- Component._encode stops ignoring parameters argument on native values, now merges them
+  Fixes: #557
+  [zocker1999net]
 - ...
 
 5.0.9 (2023-09-24)
diff --git a/docs/credits.rst b/docs/credits.rst
index 1343193..eedcc24 100644
--- a/docs/credits.rst
+++ b/docs/credits.rst
@@ -70,6 +70,7 @@ icalendar contributors
 - `Natasha Mattson <https://github.com/natashamm`_
 - `NikEasY <https://github.com/NikEasY>`_
 - Matt Lewis <git@semiprime.com>
+- Felix Stupp <felix.stupp@banananet.work>
 
 Find out who contributed::
 
diff --git a/src/icalendar/cal.py b/src/icalendar/cal.py
index 4088971..2535680 100644
--- a/src/icalendar/cal.py
+++ b/src/icalendar/cal.py
@@ -113,7 +113,8 @@ class Component(CaselessDict):
     #############################
     # handling of property values
 
-    def _encode(self, name, value, parameters=None, encode=1):
+    @staticmethod
+    def _encode(name, value, parameters=None, encode=1):
         """Encode values to icalendar property values.
 
         :param name: Name of the property.
@@ -138,17 +139,19 @@ class Component(CaselessDict):
             return value
         if isinstance(value, types_factory.all_types):
             # Don't encode already encoded values.
-            return value
-        klass = types_factory.for_property(name)
-        obj = klass(value)
+            obj = value
+        else:
+            klass = types_factory.for_property(name)
+            obj = klass(value)
         if parameters:
-            if isinstance(parameters, dict):
-                params = Parameters()
-                for key, item in parameters.items():
-                    params[key] = item
-                parameters = params
-            assert isinstance(parameters, Parameters)
-            obj.params = parameters
+            if not hasattr(obj, "params"):
+                obj.params = Parameters()
+            for key, item in parameters.items():
+                if item is None:
+                    if key in obj.params:
+                        del obj.params[key]
+                else:
+                    obj.params[key] = item
         return obj
 
     def add(self, name, value, parameters=None, encode=1):


Test patch:
diff --git a/src/icalendar/tests/test_issue_557_encode_native_parameters.py b/src/icalendar/tests/test_issue_557_encode_native_parameters.py
new file mode 100644
index 0000000..99d9f05
--- /dev/null
+++ b/src/icalendar/tests/test_issue_557_encode_native_parameters.py
@@ -0,0 +1,152 @@
+"""These are tests for Issue #557
+
+TL;DR: Component._encode lost given parameters
+if the object to encode was already of native type,
+making its behavior unexpected.
+
+see https://github.com/collective/icalendar/issues/557"""
+
+
+import unittest
+
+from icalendar.cal import Component
+
+
+class TestComponentEncode(unittest.TestCase):
+    def test_encode_non_native_parameters(self):
+        """Test _encode to add parameters to non-natives"""
+        self.__assert_native_content(self.summary)
+        self.__assert_native_kept_parameters(self.summary)
+
+    def test_encode_native_keep_params_None(self):
+        """_encode should keep parameters on natives
+        if parameters=None
+        """
+        new_sum = self.__add_params(
+            self.summary,
+            parameters=None,
+        )
+        self.__assert_native_content(new_sum)
+        self.__assert_native_kept_parameters(new_sum)
+
+    def test_encode_native_keep_params_empty(self):
+        """_encode should keep paramters on natives
+        if parameters={}
+        """
+        new_sum = self.__add_params(
+            self.summary,
+            parameters={},
+        )
+        self.__assert_native_content(new_sum)
+        self.__assert_native_kept_parameters(new_sum)
+
+    def test_encode_native_append_params(self):
+        """_encode should append paramters on natives
+        keeping old parameters
+        """
+        new_sum = self.__add_params(
+            self.summary,
+            parameters={"X-PARAM": "Test123"},
+        )
+        self.__assert_native_content(new_sum)
+        self.__assert_native_kept_parameters(new_sum)
+        self.assertParameter(new_sum, "X-PARAM", "Test123")
+
+    def test_encode_native_overwrite_params(self):
+        """_encode should overwrite single parameters
+        if they have the same name as old ones"""
+        new_sum = self.__add_params(
+            self.summary,
+            parameters={"LANGUAGE": "de"},
+        )
+        self.__assert_native_content(new_sum)
+        self.assertParameter(new_sum, "LANGUAGE", "de")
+
+    def test_encode_native_remove_params(self):
+        """_encode should remove single parameters
+        if they are explicitly set to None"""
+        new_sum = self.__add_params(
+            self.summary,
+            parameters={"LANGUAGE": None},
+        )
+        self.__assert_native_content(new_sum)
+        self.assertParameterMissing(new_sum, "LANGUAGE")
+
+    def test_encode_native_remove_already_missing(self):
+        """_encode should ignore removing a parameter
+        that was already missing"""
+        self.assertParameterMissing(self.summary, "X-MISSING")
+        new_sum = self.__add_params(
+            self.summary,
+            parameters={"X-MISSING": None},
+        )
+        self.__assert_native_content(new_sum)
+        self.__assert_native_kept_parameters(new_sum)
+        self.assertParameterMissing(self.summary, "X-MISSING")
+
+    def test_encode_native_full_test(self):
+        """full test case with keeping, overwriting & removing properties"""
+        # preperation
+        orig_sum = self.__add_params(
+            self.summary,
+            parameters={
+                "X-OVERWRITE": "overwrite me!",
+                "X-REMOVE": "remove me!",
+                "X-MISSING": None,
+            },
+        )
+        # preperation check
+        self.__assert_native_content(orig_sum)
+        self.__assert_native_kept_parameters(orig_sum)
+        self.assertParameter(orig_sum, "X-OVERWRITE", "overwrite me!")
+        self.assertParameter(orig_sum, "X-REMOVE", "remove me!")
+        self.assertParameterMissing(orig_sum, "X-MISSING")
+        # modification
+        new_sum = self.__add_params(
+            orig_sum,
+            parameters={
+                "X-OVERWRITE": "overwritten",
+                "X-REMOVE": None,
+                "X-MISSING": None,
+            },
+        )
+        # final asserts
+        self.__assert_native_content(new_sum)
+        self.__assert_native_kept_parameters(new_sum)
+        self.assertParameter(new_sum, "X-OVERWRITE", "overwritten")
+        self.assertParameterMissing(new_sum, "X-REMOVE")
+        self.assertParameterMissing(new_sum, "X-MISSING")
+
+    def setUp(self):
+        self.summary = self.__gen_native()
+
+    def __assert_native_kept_parameters(self, obj):
+        self.assertParameter(obj, "LANGUAGE", "en")
+
+    def __assert_native_content(self, obj):
+        self.assertEqual(obj, "English Summary")
+
+    def __add_params(self, obj, parameters):
+        return Component._encode(
+            "SUMMARY",
+            obj,
+            parameters=parameters,
+            encode=True,
+        )
+
+    def __gen_native(self):
+        return Component._encode(
+            "SUMMARY",
+            "English Summary",
+            parameters={
+                "LANGUAGE": "en",
+            },
+            encode=True,
+        )
+
+    def assertParameterMissing(self, obj, name):
+        self.assertNotIn(name, obj.params)
+
+    def assertParameter(self, obj, name, val):
+        self.assertIn(name, obj.params)
+        self.assertEqual(obj.params[name], val)