Should throw if specversion attribute is missing #332

satazor · 2020-09-02T10:10:12Z

Describe the Bug

Does not throw if event doesn't have a specversion attribute, and fallbacks to 1.0. The spec says that the specversion attribute is mandatory, so I think this library should fail when it's missing, instead of "magically" falling back.

Steps to Reproduce

Receiver.accept({ 'Content-Type': 'application/cloudevents+json' }, { id: 'foo', source: 'foo', type: 'foo' });
Prints console.error saying it will fallback to 1.0
Validation passes

Expected Behavior
It should throw, since the specversion attribute is missing

The text was updated successfully, but these errors were encountered:

satazor · 2020-09-02T10:13:16Z

https://runkit.com/embed/chsthvg529ba

lance · 2020-09-02T16:43:49Z

@satazor thanks for pointing this out. I will look into dealing with this in #328

lance · 2020-09-03T20:30:48Z

@satazor in this PR, there was discussion around loose validation on receipt of a CloudEvent. Based on input from the community, noted in that comment, receipt of a CloudEvent should have loose validation, since it should be possible to receive and recover from poorly formed/invalid events. The example you provide is when receiving events on the wire - just such a scenario. If you would like to check whether the received event is valid, you can call CloudEvent#validate().

That method returns a boolean. I think it might be nice to see what the validation errors actually were, and we don't really have a convenience function for that. In any case, this is expected behavior.

lance · 2020-09-03T20:32:17Z

Oh - but I see the problem here. Since it's defaulting the specversion, property, by the time it's unmarshalled as a CloudEvent, it is valid, so calling ce.validate() won't help.

lance · 2020-09-03T20:35:09Z

The other required attribute that we supply a default for if it's not provided is id. The same would happen for this.

github-actions · 2020-10-04T00:02:33Z

This issue is stale because it has been open 30 days with no activity.

lholmquist · 2020-10-07T17:49:38Z

I know this has been open for a while, but the idea is that we shouldn't be defaulting the specversion and id fields?

This would be a good one to get into the 4.0 release. i can assign myself to take of it

lance · 2020-10-07T18:27:07Z

I know this has been open for a while, but the idea is that we shouldn't be defaulting the specversion and id fields?

I think defaulting them on creation with new CloudEvent() is fine. But when receiving events, we should not just assume that they are v1.0. That's what we are doing here.

sdk-javascript/src/message/http/index.ts

Lines 58 to 60 in f6be285

    
           if (version !== Version.V03 && version !== Version.V1) { 
        
             console.error(`Unknown spec version ${version}. Default to ${Version.V1}`); 
        
             version = Version.V1;

Throwing here would also remove that console.error() and resolve #333

lholmquist · 2020-10-07T18:30:02Z

@lance that sounds good. i'll take of it

* When receiving an event, we should not default the specversion to 1.0 if no specverion field is detected. * If no specversion is detected or the specversion does not conform to a currently supported version, we will now throw a ValidationError This is a BREAKING_CHANGE fixes cloudevents#332, cloudevents#333 Signed-off-by: Lucas Holmquist <lholmqui@redhat.com>

c-pius · 2021-02-04T15:13:05Z

I think defaulting them on creation with new CloudEvent() is fine. But when receiving events, we should not just assume that they are v1.0. That's what we are doing here.

I think this should also be the same for the id attribute, right?

As of now, HTTP.toEvent() will also assign a new uuidv4 to an event if passed in structured mode with no id present. This time even implicitly without a log message that it was defaulted.

parseStructured will call the CloudEvent constructor with eventObj missing the id attribute and generate it.

sdk-javascript/src/message/http/index.ts

Line 231 in 6be3b27

return new CloudEvent(eventObj as CloudEventV1 | CloudEventV03, false);

sdk-javascript/src/event/cloudevent.ts

Line 61 in 6be3b27

this.id = (properties.id as string) || uuidv4();

PS: the same would also happen with the time attribute. Maybe less important though because not a required attribute.

c-pius · 2021-02-11T13:11:14Z

FYI, there is another effect that when calling HTTP.toEvent with a structured mode event and a specversion value different from 1.0 or 0.3 (e.g. 2), than the defaulting here is applied:

sdk-javascript/src/message/http/index.ts

Line 58 in 6be3b27

if (version !== Version.V03 && version !== Version.V1) {

but the parsed event is actually getting the original value (e.g. 2) assigned by this block:

sdk-javascript/src/message/http/index.ts

Line 208 in 6be3b27

for (const key in parserMap) {

in binary mode this is prevented by this block:

sdk-javascript/src/message/http/index.ts

Line 129 in 6be3b27

if (

no sure if therefore this block for structured should use the body attributes instead (I think sending ce-header even though structured mode is used is valid as long as the context attributes are present in the body)?

sdk-javascript/src/message/http/index.ts

Line 189 in 6be3b27

if (

github-actions · 2021-03-14T00:02:33Z

This issue is stale because it has been open 30 days with no activity.

lance · 2021-05-17T21:53:40Z

@lholmquist ping - were you still thinking you'd take this? If not, I'll pick it up. Running into this as an issue now with function runtime.

This commit adds type definitions for the following: * `Invokable`: The user provided function * `Context`: The invocation context * `CloudEventResponse`: Convenience class for constructing events * `Logger`: The logging interface * `LogLevel`: Logging enumeration * `start`: A function to start the invoker * `InvokerOptions`: Options for `start` The definitions are statically tested with `tsd`. These type definitions are not actually used by the JS files. This does not convert the module to TypeScript; it just adds these type definitions to our exports. This will allow us to create TypeScript function templates without a full conversion of this code base to TypeScript. Ultimately, I think it's probably best to make the change. This change also bumps to `cloudevents@4.0.2` which does have a breaking change. See: cloudevents/sdk-javascript#332 As a result, we accept all versions of CloudEvents. Actually, I think this is probably a fine change in the long run. Loose validation upon receipt of an event allows functions to handle event sources that may be producing malformed events. I've commented out the test for now. If we decide it makes sense to leave this as-is instead of waiting for a fix from upstream, then we should remove the test and the existing logic for dealing with this, since it's no longer necessary. Finally, I've really simplified the `start` funtion interface. It now just takes a function and a bag of options. It doesn't deal with callbacks anymore. Promises are used to deal with async server startup. Signed-off-by: Lance Ball <lball@redhat.com>

lholmquist · 2021-05-18T14:02:02Z

@lance it isn't on my radar atm, so feel free to pick it up

Even if the specversion is totally invalid, we should not change the value received in an incoming `Message`. Previously we defaulted to 1.0 if we did not recognize the version number. This commit changes that, leaving the value unmodified. We default to parsing this mystery event with the 1.0 spec. When the event is validated with `event.validate()` we return `false`. One additional small change to eliminate a prettier warning about `parer` being previously declared. Fixes: cloudevents#332 Fixes: cloudevents#333 Signed-off-by: Lance Ball <lball@redhat.com>

Even if the specversion is totally invalid, we should not change the value received in an incoming `Message`. Previously we defaulted to 1.0 if we did not recognize the version number. This commit changes that, leaving the value unmodified. We default to parsing this mystery event with the 1.0 spec. When the event is validated with `event.validate()` we return `false`. One additional small change to eliminate a prettier warning about `parer` being previously declared. Fixes: #332 Fixes: #333 Signed-off-by: Lance Ball <lball@redhat.com>

Even if the specversion is totally invalid, we should not change the value received in an incoming `Message`. Previously we defaulted to 1.0 if we did not recognize the version number. This commit changes that, leaving the value unmodified. We default to parsing this mystery event with the 1.0 spec. When the event is validated with `event.validate()` we return `false`. One additional small change to eliminate a prettier warning about `parer` being previously declared. Fixes: cloudevents#332 Fixes: cloudevents#333 Signed-off-by: Lance Ball <lball@redhat.com>

lance added module/lib Related to the main source code module/transport/http Issues related to the HTTP transport protocol implementation labels Sep 2, 2020

lance self-assigned this Sep 2, 2020

github-actions bot added the status/no-issue-activity label Oct 4, 2020

lance mentioned this issue Oct 7, 2020

chore: release 4.0.0 #348

Closed

lholmquist assigned lholmquist and unassigned lance Oct 7, 2020

lholmquist mentioned this issue Oct 7, 2020

src(receiver): don't default specversion on receiving if there isn't one. #349

Closed

lance mentioned this issue Nov 5, 2020

Proposal for 4.0.0 Release #359

Closed

github-actions bot removed the status/no-issue-activity label Jan 23, 2021

github-actions bot added the status/no-issue-activity label Mar 14, 2021

lance mentioned this issue May 17, 2021

feat: add TypeScript type definitions nodeshift/faas-js-runtime#90

Merged

github-actions bot removed the status/no-issue-activity label May 19, 2021

lance mentioned this issue May 21, 2021

fix: do not modify incoming event's specversion #419

Merged

lance closed this as completed in #419 May 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Should throw if specversion attribute is missing #332

Should throw if specversion attribute is missing #332

satazor commented Sep 2, 2020 •

edited

Loading

satazor commented Sep 2, 2020

lance commented Sep 2, 2020

lance commented Sep 3, 2020

lance commented Sep 3, 2020

lance commented Sep 3, 2020

github-actions bot commented Oct 4, 2020

lholmquist commented Oct 7, 2020

lance commented Oct 7, 2020

lholmquist commented Oct 7, 2020

c-pius commented Feb 4, 2021

c-pius commented Feb 11, 2021

github-actions bot commented Mar 14, 2021

lance commented May 17, 2021

lholmquist commented May 18, 2021

Should throw if specversion attribute is missing #332

Should throw if specversion attribute is missing #332

Comments

satazor commented Sep 2, 2020 • edited Loading

satazor commented Sep 2, 2020

lance commented Sep 2, 2020

lance commented Sep 3, 2020

lance commented Sep 3, 2020

lance commented Sep 3, 2020

github-actions bot commented Oct 4, 2020

lholmquist commented Oct 7, 2020

lance commented Oct 7, 2020

lholmquist commented Oct 7, 2020

c-pius commented Feb 4, 2021

c-pius commented Feb 11, 2021

github-actions bot commented Mar 14, 2021

lance commented May 17, 2021

lholmquist commented May 18, 2021

satazor commented Sep 2, 2020 •

edited

Loading