New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deserialize ZMQMessage bodies when they're unicode #464

Merged
merged 2 commits into from Aug 14, 2017

Conversation

Projects
None yet
3 participants
@jeremycline
Member

jeremycline commented Aug 9, 2017

See #463

@codecov-io

This comment has been minimized.

codecov-io commented Aug 9, 2017

Codecov Report

Merging #464 into develop will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #464   +/-   ##
========================================
  Coverage    58.91%   58.91%           
========================================
  Files           29       29           
  Lines         1835     1835           
  Branches       303      303           
========================================
  Hits          1081     1081           
  Misses         667      667           
  Partials        87       87

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 ba516c1...1265eaf. Read the comment docs.

jeremycline added a commit that referenced this pull request Aug 9, 2017

Deserialize message bodies when they're unicode
The validate API should deserialize message bodies of ZMQMessages if
they're six.text_type. Additionally, we'll (for backwards compatibility)
attempt to decode bytes to UTF-8 and log an error since this is not a
safe API and is deprecated.

This is a cherry-pick of #464

Signed-off-by: Jeremy Cline <jeremy@jcline.org>
with a dictionary value.
Raises:
RuntimeWarning: If the message is not valid.

This comment has been minimized.

@bowlofeggs

bowlofeggs Aug 11, 2017

Member

Would ValueError make sense instead of RuntimeWarning? If not, what about RuntimeError instead of RuntimeWarning?

Also, you could also document the potential encoding exception if the message fails to decode with the UTF-8 codec.

This comment has been minimized.

@jeremycline

jeremycline Aug 14, 2017

Member

I can think of a number of better exception types, but I'm just documenting the existing interface.

I added the decode error exception though, good catch.

# that that was the encoding used. This API should eventually only
# accept unicode strings inside messages. If a UnicodeDecodeError
# happens, let that bubble up.
self.log.error('Message body is not unicode; this is deprecated')

This comment has been minimized.

@bowlofeggs

bowlofeggs Aug 11, 2017

Member

I recommend using the warnings module here to issue a deprecation warning, rather than logging an error. After all, using deprecated code isn't actually an error… yet ☺

This comment has been minimized.

@jeremycline

jeremycline Aug 14, 2017

Member

I guess I'll switch it, but I don't know that deprecated is the right word for this either. It seemed easier than saying "this is probably not going to work in the way you expect and produce garbage data because I don't know what the encoding is".

😞

self.assertRaises(RuntimeWarning, self.consumer.validate, message)
def test_no_topic_in_body(self):
"""Assert the API accepts dictionary messages."""

This comment has been minimized.

@bowlofeggs

bowlofeggs Aug 11, 2017

Member

The name of this test and this comment don't seem to match. The comment seems copied from test_valid_signature above?

This comment has been minimized.

@jeremycline

jeremycline Aug 14, 2017

Member

You caught me, I copy and paste test stubs a lot :(

@@ -270,6 +272,7 @@ class CheckTests(unittest.TestCase):
"""Tests for the :class:`fedmsg.commands.check.CheckCommand`."""
def setUp(self):
self.maxDiff = None

This comment has been minimized.

@bowlofeggs

bowlofeggs Aug 11, 2017

Member

Do you want this change to be here? Might have just been for debugging…

@@ -1,4 +1,6 @@
import resource

This comment has been minimized.

@bowlofeggs

bowlofeggs Aug 11, 2017

Member

Not related to this PR, but it might be good to give this file a Copyright block.

jeremycline added some commits Aug 9, 2017

Deserialize message bodies when they're unicode
The validate API should deserialize message bodies of ZMQMessages if
they're six.text_type. Additionally, we'll (for backwards compatibility)
attempt to decode bytes to UTF-8 and log an error since this is not a
safe API and is deprecated.

Signed-off-by: Jeremy Cline <jeremy@jcline.org>
Much more reliable check tests
Use an IPC socket rather than a TCP socket for monitoring and don't
violate thread-safety requirements of ZMQ sockets.

Signed-off-by: Jeremy Cline <jeremy@jcline.org>

@jeremycline jeremycline force-pushed the jeremycline:fix-463 branch from fe4e65f to 1265eaf Aug 14, 2017

@jeremycline jeremycline merged commit 9a35bd4 into fedora-infra:develop Aug 14, 2017

3 checks passed

codecov/patch Coverage not affected when comparing ba516c1...1265eaf
Details
codecov/project 58.91% remains the same compared to ba516c1
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jeremycline jeremycline deleted the jeremycline:fix-463 branch Aug 14, 2017

@jeremycline

This comment has been minimized.

Member

jeremycline commented Aug 14, 2017

This patch was backported to 0.19.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment