Skip to content
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

Support non-UTF8 payloads (per MQTT specification) #29

Merged
merged 3 commits into from
Feb 27, 2022

Conversation

jmacd
Copy link
Contributor

@jmacd jmacd commented Feb 27, 2022

What are you trying to achieve?

I'm testing this MQTT broker service with the use of Sparkplug-B payloads. The payloads are Google protobuf encoded, are not and should not be expected to be valid UTF8. The is-valid-UTF8 test was causing my device's connection to fail during setup because the Will Message was invalid protobuf data.

The MQTT specification does not require UTF8 for []byte payloads, only for certain string fields, as I understand it.

Note: While testing, I discovered that ErrOffsetBytesOutOfRange was not being used. The decodeBytes() method was returning the error for string decoding. I have updated/improved the tests of the decode methods to test for expected errors explicitly and added an invalid-UTF8 test case.

Why are you doing this?

Correcting the use of errors in codec.go won't actually help propagate the reason for the failure back to my device. In packets.go, there is a block of code that discards the lower-level error:

		pk.WillMessage, offset, err = decodeBytes(buf, offset)
		if err != nil {
			return ErrMalformedWillMessage
		}

I'm not sure what the authors wishes are here -- discarding the low-level error is certainly efficient but makes diagnostics more difficult. I would prefer to see

		pk.WillMessage, offset, err = decodeBytes(buf, offset)
		if err != nil {
			return fmt.Errorf("%s: %w", err, ErrMalformedWillMessage)
		}

This returns the high-level error type information to the caller while also returning the low-level error explanation to the user. The cost, of course, is more allocation. Since errors should be unexpected, I prefer to think of this as not a performance hit.

If you approve, I would be glad to apply the above suggestion throughout packets.go in a future PR.

@codecov-commenter
Copy link

Codecov Report

Merging #29 (b0dcaab) into master (460f0ef) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #29   +/-   ##
=======================================
  Coverage   98.86%   98.86%           
=======================================
  Files          19       19           
  Lines        2028     2028           
=======================================
  Hits         2005     2005           
  Misses         20       20           
  Partials        3        3           
Impacted Files Coverage Δ
server/internal/packets/packets.go 100.00% <ø> (ø)
server/internal/packets/codec.go 100.00% <100.00%> (ø)

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 460f0ef...b0dcaab. Read the comment docs.

@jmacd
Copy link
Contributor Author

jmacd commented Feb 27, 2022

Additional cleanups: I've removed the result field from test expectations where shouldFail is non-nil, since the unused result field in those cases is misleading. The TestDecodeString method's result type changes, only the first element of the slice was being used.

@mochi-co mochi-co added the enhancement New feature or request label Feb 27, 2022
@mochi-co mochi-co changed the base branch from master to v1.1.2 February 27, 2022 08:28
@mochi-co
Copy link
Collaborator

I'm not sure what the authors wishes are here
That would be me! The answer is twofold - I was simply trying to be consistent in returns, and because it never really came up.

Now that it has I am grateful for your contribution! I think this is an excellent idea and I'd like to include it in v1.1.2, as well as any relevant changes to packets.go. Big fan of %w, too.

I'll merge this to the v1.1.2 branch 👍🏻

@mochi-co mochi-co merged commit 9c6f602 into mochi-mqtt:v1.1.2 Feb 27, 2022
@jmacd jmacd deleted the jmacd/payload_not_utf8 branch February 28, 2022 05:16
@mochi-co
Copy link
Collaborator

mochi-co commented Mar 1, 2022

If you approve, I would be glad to apply the above suggestion throughout packets.go in a future PR.

@jmacd If you're still up for it, I will leave v1.1.2 until this is added for the sake of completeness 👍🏻

@jmacd
Copy link
Contributor Author

jmacd commented Mar 1, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants