Prototype of a delegating CloudEventFormatter

[jskeet]

This is the proposed (but certainly not finalized) approach for #321.

[jskeet]

(I'd forgotten about signing-off the commit, but I haven't updated it, because we definitely don't want to merge it in the current state...)

[captainsafia]

Public extension methods + internal class seems like a good conservative base to start from. We can always expand the API surface later as needed.

[captainsafia]

I'm torn between the two but I think Delegating is the right way to go here. Decorating implies that data is modified/enhanced in some way whereas delegating does a better job of describing it's strictly behavioral, IMO.

[lailabougria]

@jskeet First of all, my apologies for taking so long. I basically dissapeared for a couple of weeks of back-to-back conferences.

I do have a couple of questions as I'm new to the code base and there aren't any tests that show how this API is to be used, so apologies if they're straightforward:

• Based on the fact that the CloudEventFormatters is public, my assumption is that this API is to be used by users to register validation actions?
• Does this mean the CreateExtension method would change, removing the validationAction from the current API?
• Currently it looks like a user would call WithValidation only once, and the action should contain all validations for that specific type of CloudEvent. Given that currently, validation actions are registered per extension, I was wondering what the underlying thought process was. As a user, I liked the seperation of validation action per extension as it keeps that code smaller and focussed, although the downside is that if there are cross-extension validations (validation applying to multiple extensions at a time) it becomes trickier.

[jskeet]

Based on the fact that the CloudEventFormatters is public, my assumption is that this API is to be used by users to register validation actions?

Well, to create new CloudEventFormatter instances which do perform validation. (It's not used to modify any existing instances.)

@captainsafia's suggestion is that the extension methods there should be the only public API surface, at least for the moment.

Does this mean the CreateExtension method would change, removing the validationAction from the current API?

No, they're performing different kinds of validation. CloudEventAttribute.CreateExtension can specify validation that is just at the scope of "is this value valid for this extension" whereas CloudEventFormatters.WithValidation is intended to validate an overall CloudEvent being formatted/parsed by a CloudEventFormatter.

Currently it looks like a user would call WithValidation only once, and the action should contain all validations for that specific type of CloudEvent.

Yes. The intention is there are two different type of validation:

• CloudEventAttribute-specific: "Is the given value valid for this CloudEvent"
• CloudEvent-wide: "Does this CloudEvent have everything I expect it to" - which would include whether "required" extensions are all present

If an extension attribute is required, that can only be validated if some validator knows about it - so if a CloudEventFormatter is provided with a CloudEvent which doesn't include the attribute, and asked to encode it, how could it know that the extension attribute which is missing should be present? I think that has to be known by the CloudEventFormatter.

For the very narrow situation of "I only care about decoding, and I only care about independently-required attributes" it would be possible to let CloudEventFormatter look at the extension attributes provided on the decode call - but that wouldn't help with cases such as "Either both X and Y must be specified, or neither" which are easliy handled by the CloudEvent-wide approach in this PR.

[lailabougria]

@jskeet Thanks for clarifying, that makes sense.

I appreciate keeping these types of validations seperate, that's ideal.
I look forward to see this being included as it would help to address the use cases I had in mind.

Thank you!

[jskeet]

@lailabougria Great, thanks. I'll turn this into a proper PR with documentation and tests when I have the time... but at the moment, that could be in quite a while.

Until I've got time to put this into a submittable state (unless you have the time to work on it yourself, of course) - you could potentially copy this code into your own codebase, and then just drop it when it's part of the SDK. It's only additive, and only uses public members as far as I can remember.

[lailabougria]

@jskeet My summer vacation is about to start, but I will look at it when I return if you haven't had the chance to start yet.