feat: add a constructor parameter for loose validation

[lance]

Proposed Changes

• add a second, optional, boolean parameter called strict to CloudEvent constructor, defaulting to true.
• add a second, optional, boolean parameter called strict to CloudEvent.cloneWith() defaulting to true
• skip all validation steps if strict is false
• add tests for these changes

Description

This commit adds a second, optional boolean parameter to the CloudEvent
constructor. When false is provided, the event constructor will not
perform validation of the event properties, values and extension names.

As I am writing this, I wonder if it will be less error prone to default to false
and call the parameter looseValidation.

• Fixes: Loose event validation #325
• Version: 3.1.0

[lholmquist]

As I am writing this, I wonder if it will be less error prone to default to false and call the parameter looseValidation.

Or maybe it is just called validate? what ever it is called, Validation should happen in the default state and an action should be needed to turn it off

[grant]

I'd rather not have implicit validation unless explicitly requested by the user.
It would be great if a user could just call a method to validate a CE at any time if they want.

[lholmquist]

I'd rather not have implicit validation unless explicitly requested by the user.

Then what's the point of having a module that is trying to adhere to a spec if it is off by default?

[grant]

I'd rather not have implicit validation unless explicitly requested by the user.

Then what's the point of having a module that is trying to adhere to a spec if it is off by default?

I would expect a separate method for validation. Or at least be optional.
I wouldn't expect it to be coupled with event creation.

Spec: https://github.com/cloudevents/spec/blob/master/SDK.md#compose-an-event

A common way to do this would be:

const ce = new CloudEvent({...});
const isValid = ce.isValid() // true or false
if (!isValid) { 
  console.log('Not a valid CloudEvent');
}
// or...
try {
  ce.validate();
} catch (e) {
  console.error(e);
}

I would not imagine myself doing:

let ce;
try {
  ce = new CloudEvent({...}, true);
} catch (e) {
  console.log(e)
}

---

There's still lots of reason to use the module like calling calling ce.validate(), ce.toJSON(), etc. Hope the above makes sense.

[lance]

Then what's the point of having a module that is trying to adhere to a spec if it is off by default?

I tend to agree with you in principle here @lholmquist.

That said, a close reading of the spec makes me think that it's acceptable to default to not be strict. Specifically, https://github.com/cloudevents/spec/blob/master/SDK.md#validation only mentions that an SDK must make it possible to validate a given Event per a spec. This implies that having invalid events is a possibility, and potentially expected. When I think about possible friction that a developer might experience when starting to work with this SDK, I can see how easy it might be to get confused or frustrated if you're testing things with an invalid event and just don't know it - and strict validation is on by default, but there's no obvious way to know this.

In spite of my tendency to prefer strict adherence to the spec always, I think maybe in this case, making the easy path as easy as possible, while providing for a simple way to remain strict could be a good thing. I don't like what it means for our tests though. By defaulting to loose validation, I think most if not all of our existing tests would need to be modified.

Maybe we can get a discussion going in the #cloudeventssdk slack to see what others think.

[lholmquist]

Maybe we can get a discussion going in the #cloudeventssdk slack to see what others think.

That sounds reasonable

[grant]

Yeah, its not a clear-cut problem. I don't know what the spec says about passing invalid CEs (they should probably work unless there is a fatal issue).

Maybe we can get a discussion going in the #cloudeventssdk slack to see what others think.

Checking others with slack sgtm.

Another rationale for separate validation is because we may have already validated the CloudEvent (maybe from a different sdk, when cloning, already validated in the server, etc).

[lance]

OK - I brought this issue to attention at the CE community Zoom call today and got some good feedback. Here's a proposal.

• Creating a CloudEvent manually using the constructor, e.g. const ce = new CloudEvent({}); should be strict mode
• Receiving a Message over HTTP or other transport and converting to a CloudEvent via HTTP.toEvent() should be loose mode with no explicit validation
• Creating a Message from a CloudEvent via HTTP.binary(event) or HTTP.structured(event) should validate before creating the Message instance

This accomplishes a few things

• Users have to explicitly intend to create a bad CloudEvent manually (note: I think we should improve our error messages in this case, so that it's clear when an exception is being thrown, why exactly something went wrong)
• Allow receipt of events from external systems that may be generating bad/non-conformant events
• Prevent a user from sending bad events using this SDK, ensuring that events coming from systems using this SDK are interoperable with other conformant systems

[grant]

Thanks for investigating :) Sounds fine.

My opinion is a little different (validation should be explicitly requested by the user, not done in certain cases), but the above proposal sounds fine so long as folks don't get unexpected errors.

[lance]

@cloudevents/sdk-javascript-maintainers I have made the changes noted in the comments. You can see the Message changes tested here: https://github.com/cloudevents/sdk-javascript/pull/328/files#diff-8b36d95f2fcd988540ae11f4206992cfR53-R87 and the CloudEvent changes tested here https://github.com/cloudevents/sdk-javascript/pull/328/files#diff-9f6cd81066208bf471aae16589fa3389R24-R38. I have also modified the ValidationError class to provide much more information in the error message when an event instance is invalid https://github.com/cloudevents/sdk-javascript/pull/328/files#diff-43312dc1cddf7a559d27b62f8d838c03R11-R22. PTAL

[lholmquist]

@lance I think this is ready to be merged?