Skip to content
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

SHOULD throw if time has an invalid value? #386

Open
c-pius opened this issue Feb 17, 2021 · 5 comments
Open

SHOULD throw if time has an invalid value? #386

c-pius opened this issue Feb 17, 2021 · 5 comments

Comments

@c-pius
Copy link

c-pius commented Feb 17, 2021

Describe the Bug
If an invalid value for time is given, HTTP.toEvent silently defaults it to the current time in UTC.

Steps to Reproduce
Use the following snippet:

const event = HTTP.toEvent({
  headers: {
    'Content-Type': 'application/json',
    'ce-id': '0815',
    'ce-specversion': '1.0',
    'ce-type': 'my.event.type',
    'ce-source': 'my.event.source',
    'ce-time': 'some invalid time string',
  },
  body: {}
})

Check the resulting event:

{
  //...
  id: '0815',
  specversion: '1.0',
  type: 'my.event.type',
  source: 'my.event.source',
  time: 2021-02-16T17:27:34+01:00 // defaulted date.now
}

Expected Behavior
As with the other issues I opened regarding (silent) defaulting, I think HTTP.toEvent should not do this. I think it would be more appropriate to either fail fast on invalid input or take the original value with a subsequent event.validate() failing (maybe also thinking about strict mode as for instance in event.cloneWith).

In case of new CloudEvent(...) I agree that it makes sense to provide defaults.

Additional context
Origin of this behavior seems to be here:

export class DateParser extends Parser {

@github-actions
Copy link
Contributor

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

@black-snow
Copy link

In case of new CloudEvent(...) I agree that it makes sense to provide defaults.

I don't. I was suprised to find this. The field is optional - silently setting it is absolutely wrong imho and other SDKs don't do it (at least the Java one).

@github-actions
Copy link
Contributor

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

@black-snow
Copy link

not stale

@github-actions
Copy link
Contributor

github-actions bot commented Apr 1, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants