Bug: time attribute is always a string, yet is type casted to string | Date

[moltar]

It appears that time constructor attribute value, which can be Date is always converted to a string:

sdk-javascript/src/event/cloudevent.ts

Lines 91 to 95 in 73f0bec

	if (!this.#_time) {
	this.#_time = new Date().toISOString();
	} else if (this.#_time instanceof Date) {
	this.#_time = this.#_time.toISOString();
	}

The setter also converts to a string:

sdk-javascript/src/event/cloudevent.ts

Lines 130 to 132 in 73f0bec

	set time(val: string | Date) {
	this.#_time = new Date(val).toISOString();
	}

Yet, the value of the getter is type casted to string | Date. This is actually confusing and leads to buggy behavior.

This led me to a wrong conclusion, that if I pass a Date it will remain a Date.

sdk-javascript/src/event/cloudevent.ts

Line 126 in 73f0bec

get time(): string | Date {

[lance]

@moltar - good catch. All of this conversion with the time attribute is done because the spec for JSON representation of an event specifically requires a string. But I think this is highlighting a deeper issue related to the JSON representation.

When we refactored this repository into TypeScript, there was an effort to create the CloudEvent class in such a way that simply doing console.log(event) would result in a JSON representation adhering to the spec. This breaks down in a few places and is why you see some attribute transformation as in this case.

I think we need to consider the fact that the CloudEvent class has to be explicitly transformed via toString() and JSON.stringify() in order to have a valid JSON representation of an event. We are working on some big API changes for the 4.0.0 version, and I think that this question should be addressed in 4.0. Resolving this, would result in a fix for what you highlight here, as well.

@cloudevents/sdk-javascript-maintainers any thoughts on this?

[lholmquist]

If we are adding in a toString method for the CloudEvent class, then i don't think we need to wait until 4.0 since it would just be additive and would really break existing stuff.

[lance]

If we are adding in a toString method for the CloudEvent class, then i don't think we need to wait until 4.0 since it would just be additive and would really break existing stuff.

@lholmquist You are probably correct, but I worry the changes would be bigger than that for TypeScript users, since we will be potentially changing function type signatures.

[lance]

I've been thinking about this, poking around some of the code, and reading some TypeScript issues. At first, I thought that the easiest and most straightforward way to handle this would be to just change the getters and setters so that the setter takes string | Date and the getter returns only string.

https://github.com/cloudevents/sdk-javascript/blob/main/src/event/cloudevent.ts#L126-L132

However, I ran into the problem noted in this issue, that the types for the getter and setter must match. That issue was originally created 5 years ago, and it's been discussed a lot. This is not going to change.

So... we have a couple of choices.

1. We can continue to have the time attribute support both - string | Date and return exactly what the user provided in the constructor. This would mean that we need to modify the toJSON() method to handle Date to string conversion, because the spec requires the JSON representation to be a string in a particular ISO format. There are other places in the code where the time attribute as accessed directly that would need to have the same conversion take place at that point (e.g. when transforming to/from a Message, and when validating the event against the schema). We would need to do this because the getter is what currently does the conversion, but if we change that to always return what the user provided to the constructor, the conversion needs to happen at the point of access. This option is nice for the user, since if they have a time string, they can use it - and if they have a Date object, they can use that - either way. But it's still potentially a bit confusing with all of the conversion happening.

2. We can have time support only Date. Here we would need to go through the code and make the same conversion modifications as noted in 1 above. This option is nice for the user, since what they pass to the constructor is what they get back, and it's always a Date.

3. We can have time support only String. Here we would need to do format the string properly in the constructor (this.time = new Date(options.time).toISOString()). This option is nice because it's probably the cleanest way to have a spec compliant JSON representation while keeping it clear to the user what's going on.

I'm leaning towards 3 but would like to hear other's thoughts. /cc @cloudevents/sdk-javascript-maintainers

[grant]

@moltar - good catch. All of this conversion with the time attribute is done because the spec for JSON representation of an event specifically requires a string. But I think this is highlighting a deeper issue related to the JSON representation.

I think we probably don't need to convert/touch the time value whenever it is set or retrieved. I think the assumption for mapping to a string is only needed when transporting the CE (JSON doesn't support Date).

The JSON format is only needed for transport, not internal representation.

Javascript natively maps types to JSON like Date, so we shouldn't have custom conversion logic.

JS will always JSON stringify values. The ISO string may lead to weird behavior. We should probably just not touch the CE#time imo. It'll always be compliant to the spec.

[lance]

Javascript natively maps types to JSON like Date, so we shouldn't have custom conversion logic.

I generally agree, but from MDN.

Until ECMAScript 2018 (edition 9), the format of the string returned by Date.prototype.toString was implementation dependent. Therefore it should not be relied upon to be in the specified format.

So there is that to think about.

JS will always JSON stringify values. The ISO string may lead to weird behavior. We should probably just not touch the CE#time imo. It'll always be compliant to the spec.

It's not as cut and dried as that, unfortunately. The Timestamp attribute per the spec MUST be in RFC3339 format, which is not what Date.prototype.toString() produces.

let d = new Date()
d.toString() // 'Fri Aug 28 2020 13:27:13 GMT-0400 (Eastern Daylight Time)'
d.toISOString() // '2020-08-28T17:27:13.043Z'

This is a problem with accepting timestamps as a string because the user could do something like this. I'm not saying it's smart, but they could do it and break compatibility unless we manipulate the incoming string.

const date = new Date();
const ce = new CloudEvent({
  source: '/',
  type: 'example',
  time: date.toString()
});

This is why we have the special logic specifically for the time attribute now.

Fortunately, JSON.stringify(new Date()) does produce the string in ISO format. So this is good.

Given all of the above, I think we should probably declare that only a Date is valid in the constructor, getters and setters. If we do that, then we can do as you suggest @grant and not do any special handling of the time attribute and just let JSON.stringify() do its thing.

[grant]

All good points.

The main points I want to communicate:

• When creating/receiving a CloudEvent, the #date value should not be modified as provided. You should be able to set and get the date hundreds of times in different formats without changes being observed due to getters/setters. Date or string both sound reasonable.
  • The loose validation should allow even invalid strings (strict validation obviously checks ifValid).
• When validating a CloudEvent, check the date to see if RFC3339 is met, whatever the internal representation is.
• When sending a CloudEvent, send the date in RFC3339 format.

[moltar]

Thanks! :)