Skip to content

Loose event validation #325

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
lance opened this issue Aug 24, 2020 · 2 comments · Fixed by #328
Closed

Loose event validation #325

lance opened this issue Aug 24, 2020 · 2 comments · Fixed by #328
Assignees
Labels
module/lib Related to the main source code type/discussion Issues that need to be decided/debated/discussed type/enhancement New feature or request version/4.x Issues related to the 4.0 release of this library

Comments

@lance
Copy link
Member

lance commented Aug 24, 2020

Is your feature request related to a problem? Please describe.
Currently, when creating a CloudEvent instance, one of the final things that happens in the constructor before freezing the object is this.validate() is called. The validate() function will throw an exception if the event is invalid. This effectively means that there is no way to create an event that's invalid. In most cases, this is probably for the best. But what happens if you have a system with a known bug that's emitting invalid events, and some JavaScript service using this module is receiving them. There's no way to actually do that without forking this repo.

Describe the solution you would like to see

  • Optionally, don't call this.validate() in the constructor based on an added boolean parameter (strict: boolean = true by default).
  • Change Detector interface from feat: introduce Message, Serializer, Deserializer and Binding interfaces #324 to use an optional parameter for loose validation.
  • Change the Deserializer interface which converts from a Message to a CloudEvent to use an optional parameter for loose validation.

Additional context
This is all in the context of creating events manually, or deserializing an incoming Message into a CloudEvent. I haven't thought much about the serialization step. If a user manually creates an invalid event, and then serializes it to a Message, should that throw an exception? I think maybe it should, but interested in other's opinions.

@lance lance added type/enhancement New feature or request type/discussion Issues that need to be decided/debated/discussed module/lib Related to the main source code version/4.x Issues related to the 4.0 release of this library labels Aug 24, 2020
@lholmquist
Copy link
Contributor

When i was first adding the validation to the CloudEvent constructor, i contemplated adding what you are talking about in. My main reason was because of how the test were written.

I can see this being a feature, but my only concern is if a user has a service where they send cloud events, and turn off the event validation. then another user uses that service to receive events, how will they know that the service has the validation turned off. They will probably get errors when trying to "accept" the event and think that maybe there is a bug in this library.

@lance lance self-assigned this Aug 26, 2020
lance added a commit to lance/sdk-javascript that referenced this issue Aug 26, 2020
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.

Fixes: cloudevents#325

Signed-off-by: Lance Ball <[email protected]>
@grant
Copy link
Member

grant commented Aug 26, 2020

Thanks for creating the issue!

According to the SDK spec, there are various methods described. One of them is creation and one is validation.
https://github.com/cloudevents/spec/blob/master/SDK.md#technical-requirements

I would assume the composition method and validation method are separate and folks can use them separately as they wish for whatever reason they want.

As such, I think the validation method should not be called by default within the composition method. (Maybe we can have it for convenience.) It should be default false. This is important because consumers of CloudEvent from major CE producers can expect to have a valid event and don't need this.

i.e. we don't need new CloudEvent({...}, false) everywhere.

lance added a commit to lance/sdk-javascript that referenced this issue Aug 28, 2020
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.

Fixes: cloudevents#325

Signed-off-by: Lance Ball <[email protected]>
@lance lance closed this as completed in #328 Sep 8, 2020
lance added a commit that referenced this issue Sep 8, 2020
* feat: add a constructor parameter for loose validation

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.

This commit also modifies the ValidationError class so that the error message
string includes the JSON.stringified version of any schema validation
errors. It also makes the HTTP.toEvent() function create CloudEvent
objects with loose/no validation.

Incorporates comments from #328

Fixes: #325

Signed-off-by: Lance Ball <[email protected]>
lholmquist pushed a commit to lholmquist/sdk-javascript that referenced this issue Sep 8, 2020
* feat: add a constructor parameter for loose validation

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.

This commit also modifies the ValidationError class so that the error message
string includes the JSON.stringified version of any schema validation
errors. It also makes the HTTP.toEvent() function create CloudEvent
objects with loose/no validation.

Incorporates comments from cloudevents#328

Fixes: cloudevents#325

Signed-off-by: Lance Ball <[email protected]>
lance added a commit that referenced this issue Sep 9, 2020
* feat: add a constructor parameter for loose validation

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.

This commit also modifies the ValidationError class so that the error message
string includes the JSON.stringified version of any schema validation
errors. It also makes the HTTP.toEvent() function create CloudEvent
objects with loose/no validation.

Incorporates comments from #328

Fixes: #325

Signed-off-by: Lance Ball <[email protected]>
lance added a commit to lance/sdk-javascript that referenced this issue Sep 9, 2020
* chore(example): Replaced body parser with express JSON parser (cloudevents#334)

Signed-off-by: Philip Hayes <[email protected]>

Co-authored-by: Philip Hayes <[email protected]>

* fix: upgrade cloudevents from 3.0.1 to 3.1.0 (cloudevents#335)

Snyk has created this PR to upgrade cloudevents from 3.0.1 to 3.1.0.

See this package in npm:
https://www.npmjs.com/package/cloudevents

See this project in Snyk:
https://app.snyk.io/org/lance/project/cb2960b0-db0c-4e77-9ab2-e78efded812e?utm_source=github&utm_medium=upgrade-pr

Co-authored-by: snyk-bot <[email protected]>
Signed-off-by: Lucas Holmquist <[email protected]>

* feat: add a constructor parameter for loose validation (cloudevents#328)

* feat: add a constructor parameter for loose validation

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.

This commit also modifies the ValidationError class so that the error message
string includes the JSON.stringified version of any schema validation
errors. It also makes the HTTP.toEvent() function create CloudEvent
objects with loose/no validation.

Incorporates comments from cloudevents#328

Fixes: cloudevents#325

Signed-off-by: Lance Ball <[email protected]>

Co-authored-by: Philip Hayes <[email protected]>
Co-authored-by: Philip Hayes <[email protected]>
Co-authored-by: snyk-bot <[email protected]>
Co-authored-by: Lance Ball <[email protected]>
lance added a commit to lance/sdk-javascript that referenced this issue Sep 9, 2020
* feat: add a constructor parameter for loose validation

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.

This commit also modifies the ValidationError class so that the error message
string includes the JSON.stringified version of any schema validation
errors. It also makes the HTTP.toEvent() function create CloudEvent
objects with loose/no validation.

Incorporates comments from cloudevents#328

Fixes: cloudevents#325

Signed-off-by: Lance Ball <[email protected]>
lance added a commit that referenced this issue Sep 11, 2020
* feat: add a constructor parameter for loose validation

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.

This commit also modifies the ValidationError class so that the error message
string includes the JSON.stringified version of any schema validation
errors. It also makes the HTTP.toEvent() function create CloudEvent
objects with loose/no validation.

Incorporates comments from #328

Fixes: #325

Signed-off-by: Lance Ball <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module/lib Related to the main source code type/discussion Issues that need to be decided/debated/discussed type/enhancement New feature or request version/4.x Issues related to the 4.0 release of this library
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants