Avoid parsing nil msg.Value#575
Conversation
| var schemaPayload []byte | ||
| var err error | ||
| if p.options.Schema != nil { | ||
| if p.options.Schema != nil && msg.Value != nil { |
There was a problem hiding this comment.
should this be checked within in the if statement and then log an error and return?
if p.options.Schema != nil {
if msg.Value == nil {
p.log.WithError(err).Errorf("Schema encode message failed nil message")
return
}
}
Or should the ecoders be able to handle a nil value and return an error?
There was a problem hiding this comment.
No, the assumption is that if value is null, a payload is already encoded.
There was a problem hiding this comment.
Is that a safe assumption to make? What happens on the consumer side if that assumption does not hold?
I think we should at least add a comment explaining this because from looking at the code it's not obvious to me.
There was a problem hiding this comment.
Sure - great call. Docs added.
There was a problem hiding this comment.
A few more questions. I'm trying to figure out how to best solve this from an api perspective because if this goes in we are changing the api and will have to support it.
It seems like it's possible to pass a custom implementation of a schema is there a reason this can't solve the issue?
Why is app encoding the message before hand?
If the app is bypassing the internal encoder when producing does it need to bypass internal decoder on the consumer side as well?
There was a problem hiding this comment.
I believe you can always bypass it on the consumer side if you don't call it and just read the Payload. You could implement a custom schema, maybe, but it would still be "hacky" for the use case I am proposing which is as a watermill adapter. Their interface requires payload to be a []byte, so we would have to set that to nil, then add the actual value to an untyped property.
There was a problem hiding this comment.
If the intention is to bypass the schema encoding/decoding why set the schema on the producer and or topic?
There was a problem hiding this comment.
I still want to take advantage of pushing schema to pulsar topic for schema validation
Motivation
Today, you are forced to use internal schema encoding when sending messages to a topic that has a schema defined. If we do message encoding (into bytes) before hand, set that as a payload but leave value empty, the internal schema encoder fails the message since
Valueisn't set. This change makes it possible to easily skip this logic without resorting to hacking a Schema interface or other things like double encoding.This change would also make it a lot easier to get a watermill adapter in place because watermill assumes message encoding happens in "userland".
Modifications
A simple check to ensure
msg.Valueis not nilVerifying this change
This change is already covered by existing tests
Does this pull request potentially affect one of the following parts: