Improve error if invalid CloudEvent in Message<string?, byte[]>.ToCloudEvent()

[markhoratiowalmsley]

The extension method to create a CloudEvent from a kafka message currently throws an ArgumentException if the message is not a CloudEvent type event, or if the headers fail validation. This means that you need to interrogate the Exception if you wish to handle this type of poisonous message explicitly.

System.ArgumentException: Unknown CloudEvents spec version '0.1' (Parameter 'message')
at CloudNative.CloudEvents.Kafka.KafkaExtensions.ToCloudEvent(Message2 message, CloudEventFormatter formatter, IEnumerable1 extensionAttributes)
at CloudNative.CloudEvents.Kafka.KafkaExtensions.ToCloudEvent(Message2 message, CloudEventFormatter formatter, CloudEventAttribute[] extensionAttributes) at JustEat.OpaOrderEventsWorker.Worker.EventConsumer.CloudEventKafkaEventWorker1.ConsumeEvents(CancellationToken cancellationToken) in /_/src/Worker/EventConsumer/CloudEventKafkaEventWorker`1.cs:line 52

I think it would be worthwhile to create a custom Exception and throw this if the message is invalid.

[jskeet]

Hmm. I do feel your pain. In some ways I wonder whether it would be best to have a TryConvert method, rather than you catching a specific exception type - but that's probably tricky to thread all the way through.

If we do this, I think we should do it for all formats and protocols... and try really, really hard to make sure that we never throw any other kind of exception (in our code at least).

For the moment, you could just wrap this call in a way that catches ArgumentException and propagates its own custom exception, until we get to doing this properly.

[markhoratiowalmsley]

Yup exactly! We are essentially wrapping the extension method at the moment in order to throw something a bit 'nicer' to interrogate.

[markhoratiowalmsley]

Just done a bit more digging, the issue we have is that there is an invalid value in ce_specversion due to the message being generated without using the official package. So when we consume the defensive code using IsCloudEvent will just check that the header is populated.
Then here
.ToCloudEvent ensures that it is a valid specversion and throws the exception.

[jskeet]

Just as a side-note - 0.1 is a valid value for ce_specversion in itself, but the C# SDK doesn't handle anything before 1.0.

Looking at the code, there's a significant problem in trying to convert everything to a new custom exception - we already use subclasses of ArgumentException, including ArgumentNullException and ArgumentOutOfRangeException. We only explicitly use ArgumentOutOfRangeException when serializing with an unknown content mode (I think) but I suspect ArgumentNullException is more of an issue.

We could potentially change everywhere that we're throwing ArgumentException directly to throw a InvalidCloudEventArgumentException or something similar (at least for things that are due to violations of the CE spec)... but I'm still somewhat nervous about it. It's hard to put my finger on why it feels wrong, but it does.

I'm still thinking about it, but will want to get more opinions too. @captainsafia any thoughts on this one?

[captainsafia]

I'm still thinking about it, but will want to get more opinions too. @captainsafia any thoughts on this one?

I'd support throwing a custom exception type for things that are specifically spec-related violations. Throwing argument exceptions for things that are ultimately constraint violations feels a little bit weird to me.

I recognize that this is probably going to be an expensive code change and it might be worthwhile to do an audit here to see what we are getting ourselves into.

My preference for ways to solve this problem in order:

1. Create custom exception type for errors that are spec violations.
2. Expose a TryConvert method and wire that through.
3. Do nothing.