Add abbility to serialize and deserialize messages #84

sebwoj · 2018-09-25T19:58:08Z

Signed-off-by: Sebastian Wojciechowski s.wojciechowski89@gmail.com

codecov-io · 2018-09-25T20:40:27Z

Codecov Report

Merging #84 into master will increase coverage by 0.18%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #84      +/-   ##
==========================================
+ Coverage   97.19%   97.38%   +0.18%     
==========================================
  Files          11       11              
  Lines         927      993      +66     
  Branches      127      129       +2     
==========================================
+ Hits          901      967      +66     
  Misses         17       17              
  Partials        9        9

Impacted Files	Coverage Δ
fedora_messaging/message.py	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 337f04c...241304a. Read the comment docs.

jeremycline

Thanks for this. I left a lot of comments, but none of them are major.

jeremycline · 2018-09-27T14:51:52Z

fedora_messaging/message.py

+        str: Serialized messages.
+
+    Raises:
+        MsgDumpError: If at least one message is not instance of Message class or subclass, or


I think it'd be best to not have our own exception class for this. Since the messages can't pass validation if they're not valid JSON, the programmer has messed up here and shouldn't be handling this sort of exception. Best to let them just get a JSON encoding error directly.

jeremycline · 2018-09-27T14:52:33Z

fedora_messaging/message.py

+            serialized_messages.append(message_json)
+    except AttributeError:
+        _log.error("Improper object for messages serialization.")
+        raise MsgDumpError("Message have to be instance of Message class or subclass.")


Probably best to just raise a "TypeError" here

jeremycline · 2018-09-27T14:55:14Z

fedora_messaging/message.py

+
+    try:
+        return json.dumps(serialized_messages)
+    except Exception as e:


Since I recommend we don't raise a MsgDumpError, you can take this out of the try/except. Just as a note, though, it's best not to except Exception unless you really do mean to handle all exceptions (often done at the entry point to a program). Here we only would want to handle JSON-related errors since that makes it explicit what problems we're expecting to encounter here.

jeremycline · 2018-09-27T14:56:54Z

fedora_messaging/message.py

+        list: Deserialized message objects.
+
+    Raises:
+        MsgLoadError: Loading serialized from JSON is imposiible.


Same note here about just letting json raise its exception as above

jeremycline · 2018-09-27T14:58:31Z

fedora_messaging/message.py

+    serialized_messages = []
+    try:
+        for message in messages:
+            message_json = message.dump()


dump() returns a dictionary so this variable name is a bit confusing - a reader might think this is actually a JSON string.

jeremycline · 2018-09-27T14:59:39Z

fedora_messaging/message.py

+    """
+    try:
+        deserialized_messages = [
+            message_str for message_str in json.loads(serialized_messages)


Since we dumped an array, shouldn't this just be message_dicts = json.loads(serialized_messages)?

jeremycline · 2018-09-27T15:00:49Z

fedora_messaging/message.py

+        try:
+            headers = msg_json["headers"]
+        except KeyError:
+            _log.error("Message saved without headers.")


I'm inclined to say this should just be an exception - whatever got handed in is not in the format we expect.

jeremycline · 2018-09-27T15:01:35Z

fedora_messaging/message.py

+        try:
+            MessageClass = get_class(headers["fedora_messaging_schema"])
+        except KeyError:
+            _log.error("Message (headers=%r) saved without a schema header.", headers)


Again, just let this be an exception. Whatever we got aren't properly serialized messages.

jeremycline · 2018-09-27T15:03:05Z

fedora_messaging/message.py

+        Returns:
+            dict: A dictionary of messge attributes.
+        """
+        return {


It'd probably be nice to save the message's id as well, so test writers who want to use this functionality can rely on stable message ids.

sebwoj · 2018-09-27T21:06:39Z

PR is updated.
Do you think that exceptions generated by json.dumps(ValueError, TypeError and OverflowError) in
f-m.message.dumps should be mentioned in method docstring?

jeremycline

Sorry about leaving this so long, I've been neglecting my code review emails!

There are a few more cases where I don't think we should handle errors and set defaults on loading, but otherwise I think this looks solid. Can you write unit tests for the new functions, as well?

Thanks!

jeremycline · 2018-10-05T14:12:06Z

fedora_messaging/message.py

+
+        try:
+            body = message_dict["body"]
+        except KeyError:


I don't think we should default this to something, just let the KeyError propagate.

jeremycline · 2018-10-05T14:12:50Z

fedora_messaging/message.py

+        try:
+            id = message_dict["id"]
+        except KeyError:
+            _log.error("Message saved without id.")


Also here, messages should always have an id so this should just let the KeyError get raised.

jeremycline · 2018-10-05T14:13:13Z

fedora_messaging/message.py

+            queue = message_dict["queue"]
+        except KeyError:
+            _log.error("Message saved without queue.")
+            queue = {}


This should probably default to None

jeremycline · 2018-10-05T14:13:39Z

fedora_messaging/message.py

+        try:
+            topic = message_dict["topic"]
+        except KeyError:
+            _log.error("Message saved without body.")


This should just let the KeyError get raised

jeremycline · 2018-10-05T14:15:29Z

fedora_messaging/message.py

+        try:
+            severity = headers["fedora_messaging_severity"]
+        except KeyError:
+            _log.error("Message saved without a severity. Defaulting to INFO.")


I think we should let the KeyError raise here as well. The reason the other code defaults it is because right now all the messages are being sent in fedmsg without it, so we fix things up for that.

jeremycline · 2018-10-05T14:17:10Z

fedora_messaging/message.py

+            raise ValidationError(e)
+
+        message.queue = queue
+        if id:


If we assume id will always be set, you can drop this if

Signed-off-by: Sebastian Wojciechowski <s.wojciechowski89@gmail.com>

sebwoj · 2018-10-11T15:24:20Z

@jeremycline Do you know how to fix "MessageLoadsTests.test_validation_failure"?
I was struggling with this, but i cant find out what is wrong.

Don't rely on the equality check implementation of ValidationError Signed-off-by: Jeremy Cline <jcline@redhat.com>

jeremycline · 2018-10-15T14:52:06Z

@sebwoj, hey, my guess is the equality method on the exceptions isn't implemented. I don't recall what Python does off the top of my head, but doing Exception() == Exception() results in False so it could just be checking to see if they are the same instance of the object. I added a commit to just check the message. It looks like there are a couple other test failures, though.

Signed-off-by: Sebastian Wojciechowski <s.wojciechowski89@gmail.com>

jeremycline

There's a minor spelling mistake and I'm curious about your thoughts on the interface, but this looks really good.

jeremycline · 2018-10-17T19:27:37Z

fedora_messaging/message.py

+        Dump message attributes.
+
+        Returns:
+            dict: A dictionary of messge attributes.


"messge" -> "message"

jeremycline · 2018-10-17T19:32:34Z

fedora_messaging/message.py

@@ -510,3 +618,18 @@ def flatpaks(self):
            list(str): A list of affected flatpaks names.
        """
        return []
+
+    def dump(self):


I wonder if this is going to be an API most people want to use, or if they should only use the "dumps" and "loads" interfaces outside the class. Maybe we should mark this as private (call it _dump) initially so we don't pollute the namespace with stuff most people won't want to use. What do you think?

As far as I know(c++ exp) private methods are not available outside the class(except friends). So, if we are using "Message.dump" in "dumps" function, it means that we have to allow access from outside. I don't have a big experience in python and I know that underscore(even dunder) can't prevent clients from access private methods, so I'm not sure about final solution. I would like to let you decide in this topic.

Ah, yes, Python has no way to force clients to not touch internals of objects, it's more like guidelines. I think we should make it private (to begin with, anyway) since I can't think of a reason people should be using it.

Signed-off-by: Sebastian Wojciechowski <s.wojciechowski89@gmail.com>

jeremycline

✨ 🍰 ✨

…_messages'

sebwoj requested a review from a team as a code owner September 25, 2018 19:58

sebwoj force-pushed the Add_abbility_to_serialize_and_deserialize_messages branch from f511800 to c827d42 Compare September 25, 2018 20:31

jeremycline requested changes Sep 27, 2018

View reviewed changes

sebwoj force-pushed the Add_abbility_to_serialize_and_deserialize_messages branch from c827d42 to b711de7 Compare September 27, 2018 21:00

sebwoj force-pushed the Add_abbility_to_serialize_and_deserialize_messages branch from b711de7 to ca2fa95 Compare September 27, 2018 21:10

jeremycline requested changes Oct 5, 2018

View reviewed changes

sebwoj added 2 commits October 11, 2018 08:53

Add abbility to serialize and deserialize messages

d87cde3

Signed-off-by: Sebastian Wojciechowski <s.wojciechowski89@gmail.com>

Add unittests to message serialization and deserialization

17b16c4

Signed-off-by: Sebastian Wojciechowski <s.wojciechowski89@gmail.com>

sebwoj force-pushed the Add_abbility_to_serialize_and_deserialize_messages branch from ca2fa95 to 17b16c4 Compare October 11, 2018 15:20

Just compare the messages

92a8192

Don't rely on the equality check implementation of ValidationError Signed-off-by: Jeremy Cline <jcline@redhat.com>

sebwoj added 2 commits October 16, 2018 23:41

Compatibility for python 2 in message serialization

3e84587

Signed-off-by: Sebastian Wojciechowski <s.wojciechowski89@gmail.com>

Correct message serialization unittest to meet black criteria

988a91b

Signed-off-by: Sebastian Wojciechowski <s.wojciechowski89@gmail.com>

jeremycline reviewed Oct 17, 2018

View reviewed changes

Make Message.dump method private

241304a

Signed-off-by: Sebastian Wojciechowski <s.wojciechowski89@gmail.com>

jeremycline approved these changes Oct 18, 2018

View reviewed changes

Merge branch 'master' into 'Add_abbility_to_serialize_and_deserialize…

d67c623

…_messages'

mergify bot merged commit 3a1f528 into fedora-infra:master Oct 18, 2018

sebwoj deleted the Add_abbility_to_serialize_and_deserialize_messages branch October 23, 2018 15:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add abbility to serialize and deserialize messages #84

Add abbility to serialize and deserialize messages #84

sebwoj commented Sep 25, 2018

codecov-io commented Sep 25, 2018 •

edited

Loading

jeremycline left a comment

jeremycline Sep 27, 2018

jeremycline Sep 27, 2018

jeremycline Sep 27, 2018

jeremycline Sep 27, 2018

jeremycline Sep 27, 2018

jeremycline Sep 27, 2018

jeremycline Sep 27, 2018

jeremycline Sep 27, 2018

jeremycline Sep 27, 2018

sebwoj commented Sep 27, 2018

jeremycline left a comment

jeremycline Oct 5, 2018

jeremycline Oct 5, 2018

jeremycline Oct 5, 2018

jeremycline Oct 5, 2018

jeremycline Oct 5, 2018

jeremycline Oct 5, 2018

sebwoj commented Oct 11, 2018

jeremycline commented Oct 15, 2018

jeremycline left a comment

jeremycline Oct 17, 2018

jeremycline Oct 17, 2018

sebwoj Oct 18, 2018

jeremycline Oct 18, 2018

jeremycline left a comment

Add abbility to serialize and deserialize messages #84

Add abbility to serialize and deserialize messages #84

Conversation

sebwoj commented Sep 25, 2018

codecov-io commented Sep 25, 2018 • edited Loading

Codecov Report

jeremycline left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebwoj commented Sep 27, 2018

jeremycline left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sebwoj commented Oct 11, 2018

jeremycline commented Oct 15, 2018

jeremycline left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremycline left a comment

Choose a reason for hiding this comment

codecov-io commented Sep 25, 2018 •

edited

Loading