javascript icon indicating copy to clipboard operation
javascript copied to clipboard

Timestamps have wrong type

Open simhnna opened this issue 3 years ago • 25 comments

The timestamps have incorrect typescript types. For example https://github.com/kubernetes-client/javascript/blob/master/src/gen/model/v1ObjectMeta.ts#L32

creationTimestamp is of type Date | undefined, but the documentation says it's an RFC3339 formatted date aka a string.

Since this is generated code I'm not sure of where to fix this. Either the type has to change to string | undefined, or the string has to be parsed and passed on as a date object.

simhnna avatar Aug 17 '20 09:08 simhnna

I don't think this is a bug. creationTimestamp is a v1.Time which is marked with openapi format: date-time in the spec here: https://github.com/kubernetes/kubernetes/tree/master/api/openapi-spec

The swagger spec says: "Tools can use the format to validate the input or to map the value to a specific type in the chosen programming language." (https://swagger.io/docs/specification/data-models/data-types/)

So I think that this is a legit code generation given the openapi spec.

Is this causing problems for you?

brendandburns avatar Aug 17 '20 14:08 brendandburns

let pod: V1Pod;
console.log(pod.metadata?.creationTimestamp, typeof pod.metadata?.creationTimestamp);

outputs 2020-07-23T06:01:17Z string

the type of V1Pod.metadata.creationTimestamp however is Date | undefined and not string | undefined

Possible solutions are converting the string to an actual Date object or change the typing to be a string.

simhnna avatar Aug 19 '20 13:08 simhnna

@brendanburns We are also experiencing problems because of this. The spec also defines the type as string even if the format is date-time

Agreon avatar Oct 13 '20 21:10 Agreon

There is another scenario where the Date is broken: CoreV1Event.eventTime which is defined as a Date here, but the api server expects a metav1.MicroTime https://github.com/kubernetes/api/blob/master/events/v1beta1/types.go#L41

This what i have trying to post an event with eventTime set as a new Date() then converted into data.toISOString by models.js serialize method

message: 'Event in version "v1" cannot be handled as a Event: v1.Event.InvolvedObject: v1.ObjectReference.EventTime: unmarshalerDecoder: parsing time "2020-12-09T23:44:10.443Z" as "2006-01-02T15:04:05.000000Z07:00": cannot parse ".443Z" as ".000000", error found in #10 byte of ...|4:10.443Z","involved|..., bigger context ...|{"eventTime":"2020-12-09T23:44:10.443Z","involvedObject":{"kind":"Game","name":"monkey-is|...',

paolomainardi avatar Dec 09 '20 23:12 paolomainardi

Issues go stale after 90d of inactivity. Mark the issue as fresh with /remove-lifecycle stale. Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale

fejta-bot avatar Mar 10 '21 00:03 fejta-bot

@brendanburns could you please re-evaluate this? /remove-lifecycle stale

simhnna avatar Mar 18 '21 16:03 simhnna

Yep. The type declaration in typescript is wrong - it declares Date | undefined but the actual value (in runtime) is string | undefined...

It is difficult to work with, and caused a bug for us (we were expecting it to be Date, all tests pass, typescript checks pass, but in runtime it blows up, because it is string)

Either:

  • Change the typescript declaration to string | undefined
  • Or parse the date in the client and return actual Date object

DocX avatar Mar 29 '21 10:03 DocX

What reported @paolomainardi seems to be related to https://github.com/kubernetes/kubernetes/issues/89156: K8S could parse without problems Dates sent from the JavaScript client, but the format currently used to parse MicroTime is too strict.

asmeikal avatar May 22 '21 12:05 asmeikal

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot avatar Aug 20 '21 12:08 k8s-triage-robot

Would be nice if this gets fixed eventually... /remove-lifecycle stale

simhnna avatar Aug 30 '21 14:08 simhnna

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot avatar Nov 28 '21 14:11 k8s-triage-robot

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

k8s-triage-robot avatar Dec 28 '21 14:12 k8s-triage-robot

/remove-lifecycle rotten

asmeikal avatar Dec 28 '21 18:12 asmeikal

Would a PR that changes the types of any Dates to be strings be accepted?

Maistho avatar Mar 28 '22 11:03 Maistho

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot avatar Jun 26 '22 11:06 k8s-triage-robot

/remove-lifecycle stale

Maistho avatar Jun 26 '22 11:06 Maistho

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot avatar Sep 24 '22 12:09 k8s-triage-robot

/remove-lifecycle stale

Maistho avatar Sep 24 '22 16:09 Maistho

/lifecycle frozen

brendandburns avatar Dec 16 '22 01:12 brendandburns

We'd like to contribute to fix this issue. Would a PR that changes the types of any Dates to be strings be accepted?

Maistho avatar Dec 26 '22 16:12 Maistho

@Maistho unfortunately all of that code is generated. So you need to either change the openapi that Kubernetes generates, or you need to change the upstream openapi code generator (https://github.com/OpenAPITools/openapi-generator/tree/master/modules/openapi-generator/src/main/resources/typescript)

If you successfully modify the upstream code generator, then we can regenerate with the new type.

brendandburns avatar Dec 30 '22 00:12 brendandburns

@brendandburns I think it should be possible to change this in the generator by adding the setting date-time=string,date=string in the typeMappings section in the typescript.xml file?

https://github.com/kubernetes-client/gen/blob/ea5af85f5bb75934bf7f3eb1edab1643e8caab47/openapi/typescript.xml#L39

As suggested in the issue on the openapi-generator repo: https://github.com/OpenAPITools/openapi-generator/issues/926#issuecomment-417247296

I think this should also affect the existing type mapping from date-time-micro to V1MicroTime (as that is most likely also a string), but I haven't verified that.

It seems like this might be (partially?) fixed in the newer typescript generator, but until that has been implemented using a typeMapping should resolve the issue.

Maistho avatar Dec 30 '22 02:12 Maistho

@Maistho thanks for digging in.

If you want to try changing that and regenerating locally to validate that it works, we can see about getting it into the 0.19.0 release.

brendandburns avatar Dec 30 '22 15:12 brendandburns

Hey again @brendandburns, sorry for taking ages on this 😓

I did some testing tonight and changing the generator configuration like this results in dates being generated as strings instead, which is correct with how the data is actually sent from the API.

diff --git a/openapi/typescript.xml b/openapi/typescript.xml
index b33e5c9..8507e13 100644
--- a/openapi/typescript.xml
+++ b/openapi/typescript.xml
@@ -36,7 +36,7 @@
                                 <npmVersion>${generator.client.version}</npmVersion>
                                 <modelPropertyNaming>original</modelPropertyNaming>
                             </configOptions>
-                            <typeMappings>int-or-string=IntOrString,date-time-micro=V1MicroTime</typeMappings>
+                            <typeMappings>int-or-string=IntOrString,Date=string</typeMappings>
                         </configuration>
                     </execution>
                 </executions>

Removing date-time-micro=V1MicroTime is also needed since that's also sent as strings, and aren't deserialised into custom V1MicroTime objects. I'm not sure how that would factor into serialization.

Would you like me to open a pull request against the generator config repository with that change?

(I tested this on both the release-1.x branch and the master branch, both seems to yield correct results)

Maistho avatar Jun 02 '23 22:06 Maistho

This seems to still be an issue. Are there any plans to fix this or workarounds?

bilalshaikh42 avatar Mar 30 '24 01:03 bilalshaikh42