Support non-UTF8 payloads (per MQTT specification)

[jmacd]

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]

Codecov Report

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

@@           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]

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]

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]

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]

I'm still working on some diagnostics, maybe for a future release? I found at least one lost error that was responsible for making it unclear why a disconnect was happening. But on further investigation, the disconnect was voluntary by the client and nothing wrong was happening in the broker. So, not a high priority. I'll follow up. For what it's worth, OpenTelemetry (my main project at work) has a bunch of issues surrounding how to model tracing through a messaging system, e.g., see the discussion on open-telemetry/opentelemetry-specification#2278. I've begun thinking that this exercise to instrument mochi-co/mqtt will help me understand the problem, so you can expect me to send you diagnostics improvements in the future.

…

On Tue, Mar 1, 2022 at 8:11 AM JB ***@***.***> wrote: If you approve, I would be glad to apply the above suggestion throughout packets.go in a future PR. @jmacd <https://github.com/jmacd> If you're still up for it, I will leave v1.1.2 until this is added for the sake of completeness 👍🏻 — Reply to this email directly, view it on GitHub <#29 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA3WFCM72OPPFYR2XXCSISDU5Y6TVANCNFSM5POOPH2A> . You are receiving this because you were mentioned.Message ID: ***@***.***>