Fixes and cleanup for tocimxml()

Details:
- Simplified the logic in CIMInstanceName.tocimxml(), based upon
  the logic that is used in CIMClassName.tocimxml().
- Cleaned up and corrected source code comments about bool and int
  checks in CIMInstanceName.tocimxml().
- Several cleanup changes in atomic_to_cim_xml():
  - Made the tests for CIM real values based upon their type rather
    than the value of their cimtype attribute.
  - Now raising a TypeError if the type is not otherwise recognized.
    Previously, that was left to str()/unicode().
  - Reordered the tests to check for the most common cases first.
  - Boolean values are now produces as upper case strings, following
    the recommendation in DSP0201.
  - As a consequence of the upper case boolean values, adjusted
    the testcases in testclient/...yaml accordingly.
- Fixed an error in cim_xml.QUALIFIER where values had been used as a
  CIM-XML string without first converting them using atomic_to_cim_xml().
  They are now converted properly.
- Simplified the pywbem.tocimxml() function to use atomic_to_cim_xml()
  instead of re-implementing most of it. Invalid types are now
  surfaced with a TypeError instead of a ValueError.

Signed-off-by: Andreas Maier <maiera@de.ibm.com>

docs/changes.rst

@@ -116,6 +116,13 @@ Incompatible changes
   because they are required to be `CIMProperty` objects anyway. A `TypeError`
   is now raised if that is detected.
 
+* The `atomic_to_cim_xml()` function now raises `TypeError` if it cannot
+  convert the input value. Previously, it used `str()` on the input value
+  as a last resort.
+
+* The global `tocimxml()` function now raises `TypeError` if it cannot
+  convert the input value. Previously, it raised `ValueError`.
+
 Deprecations
 ^^^^^^^^^^^^
 
@@ -202,6 +209,10 @@ Bug fixes
   missing enumeration_context and eos would pass one of the internal tests.
   See issue #844
 
+* Fixed an error in the CIM-XML representation of qualifier values where
+  the values were not properly converted to CIM-XML. They are now properly
+  converted using `atomic_to_cim_xml()`.
+
 * Docs: Clarified that the return type of `BaseOperationRecorder.open_file()`
   is a file-like object and that the caller is responsible for closing that
   file.
@@ -211,6 +222,10 @@ Cleanup
 
 * Removed the unimplemented and unused `popitem()` method of `NocaseDict`.
 
+* The `atomic_to_cim_xml()` function and any generated CIM-XML now generates
+  boolean values in upper case 'TRUE' and 'FALSE', following the recommendation
+  in DSP0201.
+
 Build, test, quality
 ^^^^^^^^^^^^^^^^^^^^
 

pywbem/cim_obj.py

@@ -1306,15 +1306,15 @@ def tocimxml(self):
                 continue
 
             if isinstance(value, bool):
+                # Note: Bool is a subtype of int, therefore bool is tested
+                # before int.
                 type_ = 'boolean'
                 if value:
                     value = 'TRUE'
                 else:
                     value = 'FALSE'
             elif isinstance(value, number_types):
                 # Numeric CIM data types derive from Python number types.
-                # Note: int is a subtype of bool, but bool is already
-                # tested further up.
                 type_ = 'numeric'
                 value = str(value)
             elif isinstance(value, six.string_types):
@@ -1331,27 +1331,25 @@ def tocimxml(self):
 
         instancename_xml = cim_xml.INSTANCENAME(self.classname, kbs)
 
-        # Instance name plus namespace = LOCALINSTANCEPATH
+        if self.namespace is not None:
+
+            localnsp = cim_xml.LOCALNAMESPACEPATH(
+                [cim_xml.NAMESPACE(ns)
+                 for ns in self.namespace.split('/')])
+
+            if self.host is not None:
+
+                # Instancename + namespace + host = INSTANCEPATH
 
-        if self.host is None and self.namespace is not None:
-            return cim_xml.LOCALINSTANCEPATH(
-                cim_xml.LOCALNAMESPACEPATH(
-                    [cim_xml.NAMESPACE(ns)
-                     for ns in self.namespace.split('/')]),
-                instancename_xml)
+                return cim_xml.INSTANCEPATH(
+                    cim_xml.NAMESPACEPATH(cim_xml.HOST(self.host), localnsp),
+                    instancename_xml)
 
-        # Instance name plus host and namespace = INSTANCEPATH
+            # Instancename + namespace = LOCALINSTANCEPATH
 
-        if self.host is not None and self.namespace is not None:
-            return cim_xml.INSTANCEPATH(
-                cim_xml.NAMESPACEPATH(
-                    cim_xml.HOST(self.host),
-                    cim_xml.LOCALNAMESPACEPATH([
-                        cim_xml.NAMESPACE(ns)
-                        for ns in self.namespace.split('/')])),
-                instancename_xml)
+            return cim_xml.LOCALINSTANCEPATH(localnsp, instancename_xml)
 
-        # Just a regular INSTANCENAME
+        # Just Instancename = INSTANCENAME
 
         return instancename_xml
 
@@ -1989,8 +1987,9 @@ def tocimxml(self):
         if self.path is None:
             return instance_xml
 
-        return cim_xml.VALUE_NAMEDINSTANCE(self.path.tocimxml(),
-                                           instance_xml)
+        return cim_xml.VALUE_NAMEDINSTANCE(
+            self.path.tocimxml(),
+            instance_xml)
 
     def tocimxmlstr(self, indent=None):
         """
@@ -4818,10 +4817,11 @@ def tocimxml(self):
         value = None
 
         if isinstance(self.value, list):
-            value = cim_xml.VALUE_ARRAY([cim_xml.VALUE(v) for v in self.value])
+            value = cim_xml.VALUE_ARRAY(
+                [cim_xml.VALUE(atomic_to_cim_xml(v)) for v in self.value])
         elif self.value is not None:
             # used as VALUE.ARRAY and the as VALUE
-            value = cim_xml.VALUE(self.value)
+            value = cim_xml.VALUE(atomic_to_cim_xml(self.value))
 
         return cim_xml.QUALIFIER(self.name,
                                  self.type,
@@ -5460,51 +5460,31 @@ def tomof(self):
 
 
 def tocimxml(value):
+    # pylint: disable=line-too-long
     """
-    Return the CIM-XML representation of the CIM object or CIM data type,
-    as an :term:`Element` object.
+    Return the CIM-XML representation of the input value, as an
+    :term:`Element` object.
 
     The returned CIM-XML representation is consistent with :term:`DSP0201`.
 
     Parameters:
 
-      value (:term:`CIM object` or :term:`CIM data type`):
-        The value.
+      value (:term:`CIM object`, :term:`CIM data type`, :term:`number`, :class:`py:datetime`, or tuple/list thereof):
+        The input value. May be `None`.
 
     Returns:
 
-        The CIM-XML representation of the specified value,
-        as an instance of an appropriate subclass of :term:`Element`.
+      The CIM-XML representation of the specified value, as an object of an
+      appropriate subclass of :term:`Element`.
+    """  # noqa: E501
 
-        The returned CIM-XML representation is consistent with :term:`DSP0201`.
-    """
-
-    # Python cim_obj object
+    if isinstance(value, (tuple, list)):
+        return cim_xml.VALUE_ARRAY([tocimxml(v) for v in value])
 
     if hasattr(value, 'tocimxml'):
         return value.tocimxml()
 
-    # CIMType or builtin type
-
-    if isinstance(value, (CIMType, int, six.text_type)):
-        return cim_xml.VALUE(six.text_type(value))
-
-    if isinstance(value, six.binary_type):
-        return cim_xml.VALUE(_ensure_unicode(value))
-
-    # TODO: Verify whether this is a bug to have this test after the one for
-    #       int. Bool is a subtype of int, so bool probably matches in the test
-    #       above.
-    if isinstance(value, bool):
-        return cim_xml.VALUE('TRUE') if value else cim_xml.VALUE('FALSE')
-
-    # Iterable of values
-
-    try:
-        return cim_xml.VALUE_ARRAY([tocimxml(v) for v in value])
-    except TypeError:
-        raise ValueError("Can't convert %s (%s) to CIM XML" %
-                         (value, builtin_type(value)))
+    return cim_xml.VALUE(atomic_to_cim_xml(value))
 
 
 def tocimxmlstr(value, indent=None):

pywbem/cim_types.py

@@ -860,38 +860,42 @@ def type_from_name(type_name):
 
 def atomic_to_cim_xml(obj):
     """
-    Convert a value of an atomic scalar CIM data type to a CIM-XML string and
-    return that string.
+    Convert an atomic scalar value to a CIM-XML string and return that string.
 
-    TODO: Verify whether we can change this function to raise a ValueError in
-    case the value is not CIM typed.
+    The returned CIM-XML string is ready for use as the text of a CIM-XML
+    'VALUE' element.
 
     Parameters:
 
-      obj (atomic scalar CIM typed value):
-        The CIM typed value, including `None`. Must be a scalar (not an array).
-        Must be an atomic type (i.e. those listed in :ref:`CIM data types`,
-        except any :ref:`CIM objects`).
+      obj (:term:`CIM data type`, :term:`number`, :class:`py:datetime`):
+        The input value. May be `None`.
+
+        Must not be an array/list/tuple. Must not be a :ref:`CIM object`.
 
     Returns:
 
         A :term:`unicode string` object in CIM-XML value format representing
-        the CIM typed value. For a value of `None`, `None` is returned.
+        the input value. `None`, if the input value is `None`.
+
+    Raises:
+
+        TypeError
     """
-    # pylint: disable=too-many-return-statements
     from .cim_obj import _ensure_unicode  # due to cycles
-    if isinstance(obj, bool):
-        return u"true" if obj else u"false"
-    elif isinstance(obj, CIMDateTime):
+    if obj is None:
+        return obj
+    elif isinstance(obj, six.string_types):
+        return _ensure_unicode(obj)
+    elif isinstance(obj, bool):
+        return u'TRUE' if obj else u'FALSE'
+    elif isinstance(obj, (CIMInt, number_types, CIMDateTime)):
         return six.text_type(obj)
     elif isinstance(obj, datetime):
         return six.text_type(CIMDateTime(obj))
-    elif obj is None:
-        return obj
-    elif cimtype(obj) == 'real32':
+    elif isinstance(obj, Real32):
         return u'%.8E' % obj
-    elif cimtype(obj) == 'real64':
+    elif isinstance(obj, Real64):
         return u'%.16E' % obj
-    elif isinstance(obj, six.string_types):
-        return _ensure_unicode(obj)
-    return six.text_type(obj)  # e.g. int
+    else:
+        raise TypeError("Value %r has invalid type %s for conversion to a "
+                        "CIM-XML string" % (obj, type(obj)))

testsuite/testclient/README.md

@@ -74,7 +74,7 @@ The following YAML is an example for one testcase in such a file:
                           </INSTANCENAME>
                         </IPARAMVALUE>
                         <IPARAMVALUE NAME="LocalOnly">
-                          <VALUE>False</VALUE>
+                          <VALUE>FALSE</VALUE>
                         </IPARAMVALUE>
                       </IMETHODCALL>
                     </SIMPLEREQ>
@@ -253,7 +253,6 @@ Elements in `pywbem_response` element
     are being returned. The chile element to this element specifies the
     Python object to be constructed in the same manner as the child elements
     of `result`.
-
 
 Elements in `http_request` element
 ------------------------------------

testsuite/testclient/associatorinstances.yaml

@@ -112,7 +112,7 @@
                 <NAMESPACE NAME="cimv2"/>
               </LOCALNAMESPACEPATH>
               <IPARAMVALUE NAME="IncludeQualifiers">
-                <VALUE>True</VALUE>
+                <VALUE>TRUE</VALUE>
               </IPARAMVALUE>
               <IPARAMVALUE NAME="ObjectName">
                 <INSTANCENAME CLASSNAME="TST_Blah">
@@ -128,7 +128,7 @@
                 <VALUE>myResultRole</VALUE>
               </IPARAMVALUE>
               <IPARAMVALUE NAME="IncludeClassOrigin">
-                <VALUE>True</VALUE>
+                <VALUE>TRUE</VALUE>
               </IPARAMVALUE>
               <IPARAMVALUE NAME="Role">
                 <VALUE>myRole</VALUE>
@@ -274,7 +274,7 @@
                 <NAMESPACE NAME="cimv2"/>
               </LOCALNAMESPACEPATH>
               <IPARAMVALUE NAME="IncludeQualifiers">
-                <VALUE>True</VALUE>
+                <VALUE>TRUE</VALUE>
               </IPARAMVALUE>
               <IPARAMVALUE NAME="ObjectName">
                 <INSTANCENAME CLASSNAME="PG_ComputerSystem">
@@ -293,7 +293,7 @@
                 <VALUE>myResultRole</VALUE>
               </IPARAMVALUE>
               <IPARAMVALUE NAME="IncludeClassOrigin">
-                <VALUE>True</VALUE>
+                <VALUE>TRUE</VALUE>
               </IPARAMVALUE>
               <IPARAMVALUE NAME="Role">
                 <VALUE>myRole</VALUE>
@@ -532,7 +532,7 @@
                 <NAMESPACE NAME="cimv2"/>
               </LOCALNAMESPACEPATH>
               <IPARAMVALUE NAME="IncludeQualifiers">
-                <VALUE>True</VALUE>
+                <VALUE>TRUE</VALUE>
               </IPARAMVALUE>
               <IPARAMVALUE NAME="ObjectName">
                 <INSTANCENAME CLASSNAME="PG_ComputerSystem">
@@ -551,7 +551,7 @@
                 <VALUE>myResultRole</VALUE>
               </IPARAMVALUE>
               <IPARAMVALUE NAME="IncludeClassOrigin">
-                <VALUE>True</VALUE>
+                <VALUE>TRUE</VALUE>
               </IPARAMVALUE>
               <IPARAMVALUE NAME="Role">
                 <VALUE>myRole</VALUE>

testsuite/testclient/embeddedobjects.yaml

@@ -62,7 +62,7 @@
                       </INSTANCENAME>
                     </IPARAMVALUE>
                     <IPARAMVALUE NAME="LocalOnly">
-                      <VALUE>False</VALUE>
+                      <VALUE>FALSE</VALUE>
                     </IPARAMVALUE>
                   </IMETHODCALL>
                 </SIMPLEREQ>
@@ -163,7 +163,7 @@
                       </INSTANCENAME>
                     </IPARAMVALUE>
                     <IPARAMVALUE NAME="LocalOnly">
-                      <VALUE>False</VALUE>
+                      <VALUE>FALSE</VALUE>
                     </IPARAMVALUE>
                   </IMETHODCALL>
                 </SIMPLEREQ>
@@ -269,7 +269,7 @@
                       </INSTANCENAME>
                     </IPARAMVALUE>
                     <IPARAMVALUE NAME="LocalOnly">
-                      <VALUE>False</VALUE>
+                      <VALUE>FALSE</VALUE>
                     </IPARAMVALUE>
                   </IMETHODCALL>
                 </SIMPLEREQ>
@@ -588,13 +588,13 @@
                       <CLASSNAME NAME="PyWBEM_Person"/>
                     </IPARAMVALUE>
                     <IPARAMVALUE NAME="LocalOnly">
-                      <VALUE>False</VALUE>
+                      <VALUE>FALSE</VALUE>
                     </IPARAMVALUE>
                     <IPARAMVALUE NAME="IncludeQualifiers">
-                      <VALUE>True</VALUE>
+                      <VALUE>TRUE</VALUE>
                     </IPARAMVALUE>
                     <IPARAMVALUE NAME="IncludeClassOrigin">
-                      <VALUE>True</VALUE>
+                      <VALUE>TRUE</VALUE>
                     </IPARAMVALUE>
                     <IPARAMVALUE NAME="PropertyList">
                       <VALUE.ARRAY>
@@ -619,7 +619,7 @@
                       <CLASS NAME="PyWBEM_Person" SUPERCLASS="PyWBEM_Object">
                         <PROPERTY NAME="Name" TYPE="string">
                           <QUALIFIER NAME="Key" TYPE="boolean">
-                            <VALUE>True</VALUE>
+                            <VALUE>TRUE</VALUE>
                           </QUALIFIER>
                         </PROPERTY>
                         <PROPERTY NAME="Address" TYPE="string" EmbeddedObject="instance">
@@ -717,7 +717,7 @@
                       <CLASS NAME="PyWBEM_Person" SUPERCLASS="PyWBEM_Object">
                         <PROPERTY NAME="Name" TYPE="string">
                           <QUALIFIER NAME="Key" TYPE="boolean">
-                            <VALUE>True</VALUE>
+                            <VALUE>TRUE</VALUE>
                           </QUALIFIER>
                         </PROPERTY>
                         <PROPERTY NAME="Address" TYPE="string" EmbeddedObject="instance">