Repository: tonioo/sievelib

Issue:
Factory method FiltersSet.__create_filter does not accept non-string parameters
I have been attempting to create a vacation filter using the add_to_filterset function, as per the following example. I have reduced the actual parameters for the purpose of this issue, but the error is still replicated:

        from sievelib import factory
        
        filter_set = factory.FiltersSet
        
        filter_set.addfilter(
            "test_autoresponder", [(
                "Subject", ":matches", "*"
            )], [(
                "vacation",
                ":subject", "Example Autoresponder Subject",
                ":days", 7,
                ":mime",
                "Example Autoresponder Body"
            )]
        )
        
This gives an AttributeError: *AttributeError: 'int' object has no attribute 'startswith'*

The offending code appears to be this line (https://github.com/tonioo/sievelib/blob/1.2.1/sievelib/factory.py#L260):

    if arg.startswith(":"):
    
Obviously the startswith method is not present on Python integers. In addition, attempting to cast the int to a string results in a commands.BadValue Exception being raised.

Delving into the commands.py module, it is clear the VacationCommand definition is accepting a argument of type "number" (https://github.com/tonioo/sievelib/blob/1.2.1/sievelib/commands.py#L1026), which according to the above code reference, seems as if it can never be passed in via the factory.

I believe the factory.py method should be updated to check for numeric arguments, and pass in the appropriate "number" type via atype = "number".

I have only just started to use sievelib so please forgive any ignorance in this issue, but I would be happy to submit a pull request if required.
    

Discussion:

Patch:
diff --git a/sievelib/commands.py b/sievelib/commands.py
index 5a254da..4ad5609 100644
--- a/sievelib/commands.py
+++ b/sievelib/commands.py
@@ -39,7 +39,7 @@ class UnknownCommand(CommandError):
         self.name = name
 
     def __str__(self):
-        return "unknown command %s" % self.name
+        return "unknown command '%s'" % self.name
 
 
 class BadArgument(CommandError):
@@ -224,7 +224,7 @@ class Command:
                     if not value.startswith('"') and not value.startswith("["):
                         target.write("\n")
                 else:
-                    target.write(value)
+                    target.write(str(value))
 
         if not self.accept_children:
             if self.get_type() != "test":
@@ -998,6 +998,7 @@ class CurrentdateCommand(TestCommand):
 
 
 class VacationCommand(ActionCommand):
+    extension = "vacation"
     args_definition = [
         {
             "name": "subject",
@@ -1093,11 +1094,14 @@ def get_command_instance(
     """
     cname = "%sCommand" % name.lower().capitalize()
     gl = globals()
-    condition = cname not in gl or (
+    condition = cname not in gl
+    if condition:
+        raise UnknownCommand(name)
+    condition = (
         checkexists
         and gl[cname].extension
         and gl[cname].extension not in RequireCommand.loaded_extensions
     )
     if condition:
-        raise UnknownCommand(name)
+        raise ExtensionNotLoaded(gl[cname].extension)
     return gl[cname](parent)
diff --git a/sievelib/factory.py b/sievelib/factory.py
index 72a5fd1..294fb29 100644
--- a/sievelib/factory.py
+++ b/sievelib/factory.py
@@ -153,8 +153,14 @@ class FiltersSet:
             tag = condition[1]
         cmd = commands.get_command_instance("header", parent)
         cmd.check_next_arg("tag", tag)
-        cmd.check_next_arg("string", self.__quote_if_necessary(condition[0]))
-        cmd.check_next_arg("string", self.__quote_if_necessary(condition[2]))
+        if isinstance(condition[0], list):
+            cmd.check_next_arg("stringlist", [self.__quote_if_necessary(c) for c in condition[0]])
+        else:
+            cmd.check_next_arg("string", self.__quote_if_necessary(condition[0]))
+        if isinstance(condition[2], list):
+            cmd.check_next_arg("stringlist", [self.__quote_if_necessary(c) for c in condition[2]])
+        else:
+            cmd.check_next_arg("string", self.__quote_if_necessary(condition[2]))
         return cmd
 
     def __create_filter(
@@ -188,7 +194,7 @@ class FiltersSet:
         ifcontrol = commands.get_command_instance("if")
         mtypeobj = commands.get_command_instance(matchtype, ifcontrol)
         for c in conditions:
-            if c[0].startswith("not"):
+            if not isinstance(c[0], list) and c[0].startswith("not"):
                 negate = True
                 cname = c[0].replace("not", "", 1)
             else:
@@ -281,7 +287,11 @@ class FiltersSet:
                 self.require(action.extension)
             for arg in actdef[1:]:
                 self.check_if_arg_is_extension(arg)
-                if arg.startswith(":"):
+                if isinstance(arg, int):
+                    atype = "number"
+                elif isinstance(arg, list):
+                    atype = "stringlist"
+                elif arg.startswith(":"):
                     atype = "tag"
                 else:
                     atype = "string"
@@ -563,7 +573,7 @@ class FiltersSet:
             cpt += 1
         return False  # raise not found
 
-    def dump(self):
+    def dump(self, target=sys.stdout):
         """Dump this object
 
         Available for debugging purposes
@@ -572,13 +582,14 @@ class FiltersSet:
         cmd = self.__gen_require_command()
         if cmd:
             print("Dumping requirements")
-            cmd.dump()
-            print
+            cmd.dump(target=target)
+            target.write("\n")
 
         for f in self.filters:
-            print("Filter Name: %s" % f["name"])
-            print("Filter Description: %s" % f["description"])
-            f["content"].dump()
+            target.write("Filter Name: %s\n" % f["name"])
+            if "description" in f:
+                target.write("Filter Description: %s\n" % f["description"])
+            f["content"].dump(target=target)
 
     def tosieve(self, target=sys.stdout):
         """Generate the sieve syntax corresponding to this filters set


Test patch:
diff --git a/sievelib/tests/test_factory.py b/sievelib/tests/test_factory.py
index 7c581cc..006faa5 100644
--- a/sievelib/tests/test_factory.py
+++ b/sievelib/tests/test_factory.py
@@ -573,6 +573,69 @@ if anyof (currentdate :zone "+0100" :value "gt" "date" ["2019-02-26"]) {
 """,
         )
 
+    def test_vacation(self):
+        self.fs.addfilter(
+            "test",
+            [("Subject", ":matches", "*")],
+            [("vacation", ":subject", "Example Autoresponder Subject", ":days", 7, ":mime", "Example Autoresponder Body")],
+        )
+        output = io.StringIO()
+        self.fs.tosieve(output)
+        self.assertEqual(
+            output.getvalue(),
+            """require ["vacation"];
+
+# Filter: test
+if anyof (header :matches "Subject" "*") {
+    vacation :subject "Example Autoresponder Subject" :days 7 :mime "Example Autoresponder Body";
+}
+""",
+        )
+
+    def test_dump(self):
+        self.fs.addfilter(
+            "test",
+            [("Subject", ":matches", "*")], [("vacation", ":subject", "Example Autoresponder Subject", ":days", 7, ":mime", "Example Autoresponder Body")]
+        )
+        output = io.StringIO()
+        self.fs.dump(output)
+        self.assertEqual(
+            output.getvalue(),
+            """require (type: control)
+    [vacation]
+
+Filter Name: test
+if (type: control)
+    anyof (type: test)
+        header (type: test)
+            :matches
+            "Subject"
+            "*"
+    vacation (type: action)
+        :subject
+        "Example Autoresponder Subject"
+        :days
+        7
+        :mime
+        "Example Autoresponder Body"
+""",
+        )
+
+    def test_stringlist_condition(self):
+        self.fs.addfilter(
+            "test",
+            [(["X-Foo", "X-Bar"], ":contains", ["bar", "baz"])],
+            [],
+        )
+        output = io.StringIO()
+        self.fs.tosieve(output)
+        self.assertEqual(
+            output.getvalue(),
+            """# Filter: test
+if anyof (header :contains ["X-Foo", "X-Bar"] ["bar", "baz"]) {
+}
+"""
+            )
 
 if __name__ == "__main__":
     unittest.main()
diff --git a/sievelib/tests/test_parser.py b/sievelib/tests/test_parser.py
index 78c34c9..c0f3e8d 100644
--- a/sievelib/tests/test_parser.py
+++ b/sievelib/tests/test_parser.py
@@ -950,7 +950,18 @@ if header :is "Sender" "owner-ietf-mta-filters@imc.org"
         }
 """
         )
-        self.assertEqual(self.parser.error, "line 4: unknown command fileinto")
+        self.assertEqual(self.parser.error, "line 4: extension 'fileinto' not loaded")
+
+    def test_unknown_command(self):
+        self.compilation_ko(
+            b"""require ["mailbox"];
+if header :is "Sender" "owner-ietf-mta-filters@imc.org"
+        {
+        foobar :create "filter";  # move to "filter" mailbox
+        }
+"""
+        )
+        self.assertEqual(self.parser.error, "line 4: unknown command 'foobar'")
 
     def test_exists_get_string_or_list(self):
         self.compilation_ok(