From 7cfc16ca58dfb22bc6e9cd519e6ecc7a10435fa1 Mon Sep 17 00:00:00 2001
From: Petr Viktorin <pviktori@redhat.com>
Date: Thu, 16 Feb 2012 07:11:56 -0500
Subject: [PATCH] Enforce that required attributes can't be set to None in CRUD
 Update

The `required` parameter attribute didn't distinguish between cases
where the parameter is not given and all, and where the parameter is
given but empty. The case of updating a required attribute couldn't
be validated properly, because when it is given but empty, validators
don't run.
This patch introduces a new flag, 'nonempty', that specifies the
parameter can be missing (if not required), but it can't be None.
This flag gets added automatically to required parameters in CRUD
Update.
---
 ipalib/crud.py       | 13 +++++++++++--
 ipalib/frontend.py   |  2 +-
 ipalib/parameters.py |  9 +++++----
 3 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/ipalib/crud.py b/ipalib/crud.py
index b9dfb025e..12edbf58a 100644
--- a/ipalib/crud.py
+++ b/ipalib/crud.py
@@ -186,20 +186,29 @@ class Update(PKQuery):
             for option in super(Update, self).get_options():
                 yield option
         for option in self.obj.params_minus_pk():
+            new_flags = option.flags
             attribute = 'virtual_attribute' not in option.flags
+            if option.required:
+                # Required options turn into non-required, since not specifying
+                # them means that they are not changed.
+                # However, they cannot be empty (i.e. explicitly set to None).
+                new_flags = new_flags.union(['nonempty'])
             if 'no_update' in option.flags:
                 continue
             if 'ask_update' in option.flags:
                 yield option.clone(
                     attribute=attribute, query=False, required=False,
-                    autofill=False, alwaysask=True
+                    autofill=False, alwaysask=True, flags=new_flags,
                 )
             elif 'req_update' in option.flags:
                 yield option.clone(
                     attribute=attribute, required=True, alwaysask=False,
+                    flags=new_flags,
                 )
             else:
-                yield option.clone(attribute=attribute, required=False, autofill=False)
+                yield option.clone(attribute=attribute, required=False,
+                    autofill=False, flags=new_flags,
+                )
         if not self.extra_options_first:
             for option in super(Update, self).get_options():
                 yield option
diff --git a/ipalib/frontend.py b/ipalib/frontend.py
index 028e17e79..da25b4c1a 100644
--- a/ipalib/frontend.py
+++ b/ipalib/frontend.py
@@ -651,7 +651,7 @@ class Command(HasParam):
         """
         for param in self.params():
             value = kw.get(param.name, None)
-            param.validate(value, self.env.context)
+            param.validate(value, self.env.context, supplied=param.name in kw)
 
     def verify_client_version(self, client_version):
         """
diff --git a/ipalib/parameters.py b/ipalib/parameters.py
index b1525b4d5..48155daf9 100644
--- a/ipalib/parameters.py
+++ b/ipalib/parameters.py
@@ -558,9 +558,9 @@ class Param(ReadOnly):
         else:
             value = self.convert(self.normalize(value))
         if hasattr(self, 'env'):
-            self.validate(value, self.env.context)  #pylint: disable=E1101
+            self.validate(value, self.env.context, supplied=self.name in kw)  #pylint: disable=E1101
         else:
-            self.validate(value)
+            self.validate(value, supplied=self.name in kw)
         return value
 
     def kw(self):
@@ -820,15 +820,16 @@ class Param(ReadOnly):
             error=ugettext(self.type_error),
         )
 
-    def validate(self, value, context=None):
+    def validate(self, value, context=None, supplied=None):
         """
         Check validity of ``value``.
 
         :param value: A proposed value for this parameter.
         :param context: The context we are running in.
+        :param supplied: True if this parameter was supplied explicitly.
         """
         if value is None:
-            if self.required:
+            if self.required or (supplied and 'nonempty' in self.flags):
                 if context == 'cli':
                     raise RequirementError(name=self.cli_name)
                 else: