[WIP] Add Unix Timestamp support for last_read_at in notifications API

[sonjek]

Should fix 2nd point of the issue #24574

Now last_read_at parameter in notification API supports following value formats:

• 2006-01-02T15:04:05Z
• 2006-01-02T15:04:05.999Z
• 2006-01-02T15:04:05-07:00
• 2006-01-02T15:04:05.999-07:00
• 1136214245

[silverwind]

The swagger docs still specify this parameter as date-time format and normally I would put a oneOf around it but swagger 2.0 does not have this openapi 3.0 feature unfortunately.

[wxiaoguang]

IMO the new logic could be put into a separate util function, and use more specific tests to cover it. It would benefit other "date/time" parameters in the future.

[silverwind]

Yes, call it ParseDateGraceful or similar and add some tests. And yes, it could apply to all date-time parsing on the API, but that can be done in another PR.

[silverwind]

Also, I think it'd be better to support millisecond precision, e.g. parse via UnixMilli. This is also the default precision in JavaScript, fwiw. Should be noted in the API docs, e.g. update the field comment.

[wxiaoguang]

Also, I think it'd be better to support millisecond precision, e.g. parse via UnixMilli. This is also the default precision in JavaScript, fwiw. Should be noted in the API docs, e.g. update the field comment.

That needs more designs. The problem is: when you see an integer or number, what do you think it should be for a "time"?

I prefer to use International System of Units (SI) , eg: 1 means 1 second. 1.001 means 1 second and 1 millisecond, then microsecond and nanosecond could also be supported.

[silverwind]

I prefer to use International System of Units (SI) , eg: 1 means 1 second. 1.001 means 1 second and 1 millisecond, then microsecond and nanosecond could also be supported.

Floating point numbers are a bit unergonomic, but possible, yes. Whatever we do, we should specify in the API docs. I personally would still do milliseconds, it's just precise enough to be generally useful.

For example, Grafana also accepts unix timestamps as milliseconds as well as special identifiers like now or now-6h for relative time, which in theory could also be supported.

[wxiaoguang]

I have objection for doing so. Keeping "guessing the unit" is quite fragile.

Either it strictly follows the Golang's time (int64 as nanosecond)

Or it strictly follows SI, use second as its unit, and use "0.001" for 1 millisecond (I prefer this way). There is no "float number" problem, because the time could be passed as a string, and split it by the dot "." if the caller doesn't want to use float.

[wxiaoguang]

I personally would still do milliseconds, it's just precise enough to be generally useful.

I do not think it should introduce another time unit in Gitea. At the moment, Gitea only uses 2 kinds of units:

• The standard Golang time: int64 as nanosecond
• The timeutil.TimeStamp, int64 as second (unix timestamp)

Introducing more units only increases the difficulty of the maintainability

The SI unit (second) works well and is consistent with timeutil.TimeStamp, and it doesn't need to use float, the time "1.001" could be just passed as string, the parser can split it by the dot "."

[sonjek]

I personally would still do milliseconds, it's just precise enough to be generally useful.

I do not think it should introduce another time unit in Gitea. At the moment, Gitea only uses 2 kinds of units:

* The standard Golang time: int64 as nanosecond

* The `timeutil.TimeStamp`, int64 as second (unix timestamp)

Introducing more units only increases the difficulty of the maintainability

The SI unit (second) works well and is consistent with timeutil.TimeStamp, and it doesn't need to use float, the time "1.001" could be just passed as string, the parser can split it by the dot "."

Support for the unix timestamp (seconds) is useful because it's a common standard and it is easier to get the desired value than a formatted date in the form of a date.
Originally I plan to add a support for the timestamp in seconds only.

But in JavaScript standard for the timestamp is in milliseconds.
That is why somehow we need to check that the user filled in the value exactly in seconds.
So, if we can detect dimension of the timestamps, then we can handle them.

Ideally I'd limit to the extra unix timestamp and timestamp is in milliseconds support.

I looked at the GitHub API and they only use date in format: YYYY-MM-DDTHH:MM:SSZ.
Maybe we can let everything as it is.

[wxiaoguang]

I think the UnixMilli should also be removed. I don't see a real case why it is useful, it only introduces inconsistency.

[silverwind]

I'm fine with float seconds instead of int milliseconds I guess.

[sonjek]

I think the UnixMilli should also be removed. I don't see a real case why it is useful, it only introduces inconsistency.

I removed UnixMilli support.
I think now it's not critical to support this functionality.

From now last_read_at in notifications API will supports following value formats:

• 2006-01-02T15:04:05Z
• 2006-01-02T15:04:05.999Z
• 2006-01-02T15:04:05-07:00
• 2006-01-02T15:04:05.999-07:00
• 1136214245

[silverwind]

Would it be much work to have all date-time that the APIs accept parsed by this? I think if we do it, we should do it everywhere, not just on 1-2 cherry-picked APIs.

[wxiaoguang]

Overall LGTM. Some nits.

[wxiaoguang]

If there is an error, there shouldn't be expected (it can be covered by assert.True(t, actual.IsZero()))

[wxiaoguang]

Stale?

[wxiaoguang]

Stale. Feel free to reopen.