MAINT: refactor _Messages class

Summary

For caosdb-server#286 (closed)

• rename _Messages to Messages since it is public API
• change parent class to list since Multiple Messages with same type and code may occur
• deprecate lots of stuff that makes usage fuzzy (e.g. three different ways to supply arguments) and concentrate on behavior that is close to that of lists (which is what users know)

Focus

It is quite hard to figure out what the current behavior is. However, I preserved it for now and added lots of deprecation warnings.

Test Environment

unit and int test

Check List for the Author

Please, prepare your MR for a review. Be sure to write a summary and a focus and create gitlab comments for the reviewer. They should guide the reviewer through the changes, explain your changes and also point out open questions. For further good practices have a look at our review guidelines

• All automated tests pass
• Reference related issues
• Up-to-date CHANGELOG.md (or not necessary)
• Up-to-date JSON schema (or not necessary)
• Appropriate user and developer documentation (or not necessary)
  • How do I use the software? Assume "stupid" users.
  • How do I develop or debug the software? Assume novice developers.
• Annotations in code (Gitlab comments)
  • Intent of new code
  • Problems with old code
  • Why this implementation?

Check List for the Reviewer

• I understand the intent of this MR
• All automated tests pass
• Up-to-date CHANGELOG.md (or not necessary)
• Appropriate user and developer documentation (or not necessary)
• The test environment setup works and the intended behavior is reproducible in the test environment
• In-code documentation and comments are up-to-date.
• Check: Are there specifications? Are they satisfied?

For further good practices have a look at our review guidelines.

src/caosdb/common/models.py

@@ -32,8 +32,8 @@ All additional classes are either important for the entities or the
 transactions.
 """
 
-from __future__ import print_function, unicode_literals
 from __future__ import annotations  # Can be removed with 3.10.
+from __future__ import print_function, unicode_literals
 
 import re
 import sys
@@ -51,8 +51,8 @@ from warnings import warn
 from caosdb.common.datatype import (BOOLEAN, DATETIME, DOUBLE, INTEGER, TEXT,
                                     is_list_datatype, is_reference)
 from caosdb.common.state import State
-from caosdb.common.utils import uuid, xml2str
 from caosdb.common.timezone import TimeZone
+from caosdb.common.utils import uuid, xml2str
 from caosdb.common.versioning import Version
 from caosdb.configuration import get_config
 from caosdb.connection.connection import get_connection
@@ -112,7 +112,7 @@ class Entity:
         self.__datatype = None
         self.datatype = datatype
         self.value = value
-        self.messages = _Messages()
+        self.messages = Messages()
         self.properties = _Properties()
         self.parents = _ParentList()
         self.path = None
@@ -419,7 +419,7 @@ class Entity:
             self.acl.is_permitted(permission=permission)
 
     def get_all_messages(self):
-        ret = _Messages()
+        ret = Messages()
         ret.append(self.messages)
 
         for p in self.properties:
@@ -682,7 +682,8 @@ class Entity:
         if msg is not None:
             pass
         else:
-            msg = Message(type, code, description, body)
+            msg = Message(description=description, type=type, code=code,
+                          body=body)
         self.messages.append(msg)
 
         return self
@@ -1056,7 +1057,7 @@ out: List[Entity]
     def get_messages(self):
         """Get all messages of this entity.
 
-        @return: _Messages(list)
+        @return: Messages(list)
         """
 
         return self.messages
@@ -1064,9 +1065,9 @@ out: List[Entity]
     def get_warnings(self):
         """Get all warning messages of this entity.
 
-        @return _Messages(list): Warning messages.
+        @return Messages(list): Warning messages.
         """
-        ret = _Messages()
+        ret = Messages()
 
         for m in self.messages:
             if m.type.lower() == "warning":
@@ -1077,9 +1078,9 @@ out: List[Entity]
     def get_errors(self):
         """Get all error messages of this entity.
 
-        @return _Messages(list): Error messages.
+        @return Messages(list): Error messages.
         """
-        ret = _Messages()
+        ret = Messages()
 
         for m in self.messages:
             if m.type.lower() == "error":
@@ -1599,7 +1600,7 @@ class QueryTemplate():
         self._cuid = None
         self.value = None
         self.datatype = None
-        self.messages = _Messages()
+        self.messages = Messages()
         self.properties = None
         self.parents = None
         self.path = None
@@ -1719,7 +1720,7 @@ class QueryTemplate():
         return self.id is not None
 
     def get_errors(self):
-        ret = _Messages()
+        ret = Messages()
 
         for m in self.messages:
             if m.type.lower() == "error":
@@ -1870,12 +1871,10 @@ class Property(Entity):
 
 class Message(object):
 
-    # @ReservedAssignment
-
-    def __init__(self, type, code=None, description=None, body=None):  # @ReservedAssignment
-        self.type = type
-        self.code = code
+    def __init__(self, type=None, code=None, description=None, body=None):  # @ReservedAssignment
         self.description = description
+        self.type = type if type is not None else "Info"
+        self.code = int(code) if code is not None else None
         self.body = body
 
     def to_xml(self, xml=None):
@@ -1898,11 +1897,13 @@ class Message(object):
 
     def __eq__(self, obj):
         if isinstance(obj, Message):
-            return self.type == obj.type and self.code == obj.code
+            return self.type == obj.type and self.code == obj.code and self.description == obj.description
 
         return False
 
     def get_code(self):
+        warn(("get_code is deprecated and will be removed in future. "
+              "Use self.code instead."), DeprecationWarning)
         return int(self.code)
 
 
@@ -2399,10 +2400,9 @@ class _ParentList(list):
         raise KeyError(str(parent) + " not found.")
 
 
-class _Messages(dict):
-
-    """This 'kind of dictionary' stores error, warning, info, and other
-    messages. The mentioned three messages types are messages of special use.
+class Messages(list):
+    """This specialization of list stores error, warning, info, and other
+    messages. The mentioned three messages types play a special role.
     They are generated by the client and the server while processing the entity
     to which the message in question belongs. It is RECOMMENDED NOT to specify
     such messages manually. The other messages are ignored by the server unless
@@ -2413,25 +2413,18 @@ class _Messages(dict):
 
     <$Type code=$code description=$description>$body</$Type>
 
-    Messages are treated as 'equal' if and only if both they have the same type (case-insensitive),
-    and the same code (or no code). Every message
-    MUST NOT occur more than once per entity (to which the message in question belongs).
-
-    If a message m2 is added while a messages m1 is already in this _Message object m2 will
-    OVERRIDE m1.
-
     Error, warning, and info messages will be deleted before any transaction.
 
     Examples:
-    <<< msgs = _Messages()
+    <<< msgs = Messages()
 
     <<< # create Message
     <<< msg = Message(type="HelloWorld", code=1, description="Greeting the world", body="Hello, world!")
 
-    <<< # append it to the _Messages
+    <<< # append it to the Messages
     <<< msgs.append(msg)
 
-    <<< # use _Messages as list of Message objects
+    <<< # use Messages as list of Message objects
     <<< for m in msgs:
     ...     assert isinstance(m,Message)
 
@@ -2442,29 +2435,12 @@ class _Messages(dict):
     <<< msgs.append(msg)
     <<< # get it back via get(...) and the key tuple (type, code)
     <<< assert id(msgs.get("HelloWorld",1))==id(msg)
-
-    <<< # delete Message via remove and the (type,code) tuple
-    <<< msgs.remove("HelloWorld",1)
-    <<< assert msgs.get("HelloWorld",1) == None
-
-    <<< # short version of adding/setting/resetting a new Message
-    <<< msgs["HelloWorld",2] = "Greeting the world in German", "Hallo, Welt!"
-    <<< assert msgs["HelloWorld",2] == ("Greeting the world in German","Hallo, Welt!")
-    <<< msgs["HelloWorld",2] = "Greeting the world in German", "Huhu, Welt!"
-    <<< assert msgs["HelloWorld",2] == ("Greeting the world in German","Huhu, Welt!")
-    <<< del msgs["HelloWorld",2]
-    <<< assert msgs.get("HelloWorld",2) == None
-
-    # this Message has no code and no description (make easy things easy...)
-    <<<
-    <<< msgs["HelloWorld"] = "Hello!"
-    <<< assert msgs["HelloWorld"] == "Hello!"
-
-    (to be continued...)
     """
 
     def clear_server_messages(self):
-        """Removes all error, warning and info messages."""
+        """Removes all messages of type error, warning and info. All other
+        messages types are custom types which should be handled by custom
+        code."""
         rem = []
 
         for m in self:
@@ -2474,9 +2450,18 @@ class _Messages(dict):
         for m in rem:
             self.remove(m)
 
-        return self
-
+    #######################################################################
+    # can be removed after 01.07.24
+    # default implementation of list is sufficient
     def __setitem__(self, key, value):  # @ReservedAssignment
+        if not isinstance(value, Message):
+            warn("__setitem__ will in future only accept Message objects as second argument. "
+                 "You will no longe be"
+                 " able to pass bodys such that Message object is created on the fly",
+                 DeprecationWarning)
+        if not isinstance(key, int):
+            warn("__setitem__ will in future only accept int as first argument",
+                 DeprecationWarning)
         if isinstance(key, tuple):
             if len(key) == 2:
                 type = key[0]  # @ReservedAssignment
@@ -2487,7 +2472,7 @@ class _Messages(dict):
             else:
                 raise TypeError(
                     "('type', 'code'), ('type'), or 'type' expected.")
-        elif isinstance(key, _Messages._msg_key):
+        elif isinstance(key, Messages._msg_key):
             type = key._type  # @ReservedAssignment
             code = key._code
         else:
@@ -2508,13 +2493,19 @@ class _Messages(dict):
         if isinstance(value, Message):
             body = value.body
             description = value.description
+            m = Message
         else:
             body = value
             description = None
-        m = Message(type=type, code=code, description=description, body=body)
-        dict.__setitem__(self, _Messages._msg_key(type, code), m)
+            m = Message(type=type, code=code, description=description, body=body)
+        if isinstance(key, int):
+            super().__setitem__(key, m)
+        else:
+            self.append(m)
 
     def __getitem__(self, key):
+        if not isinstance(key, int):
+            warn("__getitem__ only supports integer keys in future.", DeprecationWarning)
         if isinstance(key, tuple):
             if len(key) == 2:
                 type = key[0]  # @ReservedAssignment
@@ -2525,113 +2516,118 @@ class _Messages(dict):
             else:
                 raise TypeError(
                     "('type', 'code'), ('type'), or 'type' expected.")
-        elif isinstance(key, int) and int(key) >= 0:
-            for m in self.values():
-                if key == 0:
-                    return m
-                else:
-                    key -= 1
-            type = key  # @ReservedAssignment
-            code = None
+        elif isinstance(key, int) and key >= 0:
+            return super().__getitem__(key)
         else:
             type = key  # @ReservedAssignment
             code = None
-        m = dict.__getitem__(self, _Messages._msg_key(type, code))
-
+        m = self.get(type, code)
+        if m is None:
+            raise KeyError()
         if m.description:
             return (m.description, m.body)
         else:
             return m.body
 
-    def __init__(self):
-        dict.__init__(self)
-
     def __delitem__(self, key):
         if isinstance(key, tuple):
-            if len(key) == 2:
-                type = key[0]  # @ReservedAssignment
-                code = key[1]
-            elif len(key) == 1:
-                type = key[0]  # @ReservedAssignment
-                code = None
-            else:
-                raise TypeError(
-                    "('type', 'code'), ('type'), or 'type' expected.")
+            warn("__delitem__ only supports integer keys in future.", DeprecationWarning)
+            if self.get(key[0], key[1]) is not None:
+                self.remove(self.get(key[0], key[1]))
         else:
-            type = key  # @ReservedAssignment
-            code = None
-
-        return dict.__delitem__(self, _Messages._msg_key(type, code))
+            super().__delitem__(key)
 
     def remove(self, obj, obj2=None):
-        if isinstance(obj, Message):
-            return dict.__delitem__(self, _Messages._msg_key.get(obj))
+        if obj2 is not None:
+            warn("Supplying a second argument to remove is deprecated.",
+                 DeprecationWarning)
+            super().remove(self.get(obj, obj2))
+        else:
+            super().remove(obj)
 
-        return self.__delitem__((obj, obj2))
+    def append(self, msg):
+        if isinstance(msg, Messages) or isinstance(msg, list):
+            warn("Supplying a list-like object to append is deprecated. Please use extend"
+                 " instead.", DeprecationWarning)
+            for m in msg:
+                self.append(m)
+            return
 
-    def get(self, type, code=None, default=None):  # @ReservedAssignment
-        try:
-            return dict.__getitem__(self, _Messages._msg_key(type, code))
-        except KeyError:
-            return default
+        super().append(msg)
 
-    def extend(self, messages):
-        self.append(messages)
+    @staticmethod
+    def _hash(t, c):
+        return hash(str(t).lower() + (str(",") + str(c) if c is not None else ''))
+    # end remove
+    #######################################################################
 
-        return self
+    def get(self, type, code=None, default=None, exact=False):  # @ReservedAssignment
+        """
+        returns a message from the list that kind of matches type and code
 
-    def append(self, msg):
-        if hasattr(msg, "__iter__"):
-            for m in msg:
-                self.append(m)
+        case and types (str/int) are ignored
 
-            return self
+        If no suitable message is found, the default argument is returned
+        If exact=True, the message has to match code and type exactly
+        """
+        if not exact:
+            warn("The fuzzy mode (exact=False) is deprecated. Please use exact in future.",
+                 DeprecationWarning)
+
+        for msg in self:
+            if exact:
+                if msg.type == type and msg.code == code:
+                    return msg
+            else:
+                if self._hash(msg.type, msg.code) == self._hash(type, code):
+                    return msg
 
-        if isinstance(msg, Message):
-            dict.__setitem__(self, _Messages._msg_key.get(msg), msg)
+        return default
 
-            return self
-        else:
-            raise TypeError("Argument was not a Message")
+    def to_xml(self, add_to_element):
+        for m in self:
+            melem = m.to_xml()
+            add_to_element.append(melem)
 
-        return self
+    def __repr__(self):
+        xml = etree.Element("Messages")
+        self.to_xml(xml)
 
-    def __iter__(self):
-        return dict.values(self).__iter__()
+        return xml2str(xml)
 
+    #######################################################################
+    # can be removed after 01.07.24
     class _msg_key:
 
         def __init__(self, type, code):  # @ReservedAssignment
+            warn("This class is deprecated.", DeprecationWarning)
             self._type = type
             self._code = code
 
         @staticmethod
         def get(msg):
-            return _Messages._msg_key(msg.type, msg.code)
+            return Messages._msg_key(msg.type, msg.code)
 
         def __eq__(self, obj):
             return self.__hash__() == obj.__hash__()
 
         def __hash__(self):
-            return hash(str(self._type).lower() + (str(",") +
-                                                   str(self._code) if self._code is not None else ''))
+            return hash(str(self._type).lower() + (str(",") + str(self._code)
+                                                   if self._code is not None else ''))
 
         def __repr__(self):
             return str(self._type) + (str(",") + str(self._code)
                                       if self._code is not None else '')
+    # end remove
+    #######################################################################
 
-    def to_xml(self, add_to_element):
-        for m in self:
-            melem = m.to_xml()
-            add_to_element.append(melem)
-
-        return self
 
-    def __repr__(self):
-        xml = etree.Element("Messages")
-        self.to_xml(xml)
-
-        return xml2str(xml)
+class _Messages(Messages):
+    def __init__(self, *args, **kwargs):
+        warn("_Messages is deprecated. "
+             "Use class Messages instead and beware of the slightly different API of the new"
+             " Messages class", DeprecationWarning)
+        super().__init__(*args, **kwargs)
 
 
 def _basic_sync(e_local, e_remote):
@@ -2834,7 +2830,7 @@ class Container(list):
         list.__init__(self)
         self._timestamp = None
         self._srid = None
-        self.messages = _Messages()
+        self.messages = Messages()
 
     def extend(self, entities):
         """Extend this Container by appending all single entities in the given
@@ -2927,11 +2923,11 @@ class Container(list):
     def get_errors(self):
         """Get all error messages of this container.
 
-        @return _Messages: Error messages.
+        @return Messages: Error messages.
         """
 
         if self.has_errors():
-            ret = _Messages()
+            ret = Messages()
 
             for m in self.messages:
                 if m.type.lower() == "error":
@@ -2944,11 +2940,11 @@ class Container(list):
     def get_warnings(self):
         """Get all warning messages of this container.
 
-        @return _Messages: Warning messages.
+        @return Messages: Warning messages.
         """
 
         if self.has_warnings():
-            ret = _Messages()
+            ret = Messages()
 
             for m in self.messages:
                 if m.type.lower() == "warning":
@@ -2959,7 +2955,7 @@ class Container(list):
             return None
 
     def get_all_messages(self):
-        ret = _Messages()
+        ret = Messages()
 
         for e in self:
             ret.extend(e.get_all_messages())
@@ -3142,7 +3138,7 @@ class Container(list):
                     msg = "Request was not unique. CUID " + \
                         str(local_entity._cuid) + " was found " + \
                         str(len(sync_remote_entities)) + " times."
-                    local_entity.add_message(Message("Error", None, msg))
+                    local_entity.add_message(Message(description=msg, type="Error"))
 
                     if raise_exception_on_error:
                         raise MismatchingEntitiesError(msg)
@@ -3167,7 +3163,7 @@ class Container(list):
                     msg = "Request was not unique. ID " + \
                         str(local_entity.id) + " was found " + \
                         str(len(sync_remote_entities)) + " times."
-                    local_entity.add_message(Message("Error", None, msg))
+                    local_entity.add_message(Message(description=msg, type="Error"))
 
                     if raise_exception_on_error:
                         raise MismatchingEntitiesError(msg)
@@ -3197,7 +3193,7 @@ class Container(list):
                     msg = "Request was not unique. Path " + \
                         str(local_entity.path) + " was found " + \
                         str(len(sync_remote_entities)) + " times."
-                    local_entity.add_message(Message("Error", None, msg))
+                    local_entity.add_message(Message(description=msg, type="Error"))
 
                     if raise_exception_on_error:
                         raise MismatchingEntitiesError(msg)
@@ -3227,7 +3223,7 @@ class Container(list):
                     msg = "Request was not unique. Name " + \
                         str(local_entity.name) + " was found " + \
                         str(len(sync_remote_entities)) + " times."
-                    local_entity.add_message(Message("Error", None, msg))
+                    local_entity.add_message(Message(description=msg, type="Error"))
 
                     if raise_exception_on_error:
                         raise MismatchingEntitiesError(msg)
@@ -3246,7 +3242,7 @@ class Container(list):
             msg = "Request was not unique. There are " + \
                 str(len(sync_remote_entities)) + \
                 " entities which could not be matched to one of the requested ones."
-            remote_container.add_message(Message("Error", None, msg))
+            remote_container.add_message(Message(description=msg, type="Error"))
 
             if raise_exception_on_error:
                 raise MismatchingEntitiesError(msg)
@@ -4309,7 +4305,7 @@ class Query():
         The query string.
     flags : dict of str
         A dictionary of flags to be send with the query request.
-    messages : _Messages()
+    messages : Messages()
         A container of messages included in the last query response.
     cached : bool
         indicates whether the server used the query cache for the execution of
@@ -4332,7 +4328,7 @@ class Query():
 
     def __init__(self, q):
         self.flags = dict()
-        self.messages = _Messages()
+        self.messages = Messages()
         self.cached = None
         self.etag = None
 
@@ -4456,6 +4452,10 @@ def execute_query(q, unique=False, raise_exception_on_error=True, cache=True, fl
 
 
 class DropOffBox(list):
+    def __init__(self, *args, **kwargs):
+        warn(DeprecationWarning(
+                "The DropOffBox is deprecated and will be removed in future."))
+        super().__init__(*args, **kwargs)
 
     path = None
 
@@ -4499,7 +4499,7 @@ class UserInfo():
 class Info():
 
     def __init__(self):
-        self.messages = _Messages()
+        self.messages = Messages()
         self.sync()
 
     def sync(self):
@@ -4646,7 +4646,7 @@ def _parse_single_xml_element(elem):
     elif elem.tag.lower() == 'stats':
         counts = elem.find("counts")
 
-        return Message(type="Counts", body=counts.attrib)
+        return Message(type="Counts", description=None, body=counts.attrib)
     elif elem.tag == "EntityACL":
         return ACL(xml=elem)
     elif elem.tag == "Permissions":