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

Timestamps have wrong type #487

Open
simhnna opened this issue Aug 17, 2020 · 25 comments
Open

Timestamps have wrong type #487

simhnna opened this issue Aug 17, 2020 · 25 comments
Labels
lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness.

Comments

@simhnna
Copy link

simhnna commented Aug 17, 2020

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.

@brendandburns
Copy link
Contributor

brendandburns commented Aug 17, 2020

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?

@simhnna
Copy link
Author

simhnna commented Aug 19, 2020

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.

@Agreon
Copy link
Contributor

Agreon commented Oct 13, 2020

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

@paolomainardi
Copy link
Contributor

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|...',

@fejta-bot
Copy link

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

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 10, 2021
@simhnna
Copy link
Author

simhnna commented Mar 18, 2021

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

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 18, 2021
@DocX
Copy link
Contributor

DocX commented Mar 29, 2021

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

@asmeikal
Copy link

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

@k8s-triage-robot
Copy link

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-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 20, 2021
@simhnna
Copy link
Author

simhnna commented Aug 30, 2021

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

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 30, 2021
@k8s-triage-robot
Copy link

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-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 28, 2021
@k8s-triage-robot
Copy link

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-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Dec 28, 2021
@asmeikal
Copy link

/remove-lifecycle rotten

@k8s-ci-robot k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Dec 28, 2021
@Maistho
Copy link

Maistho commented Mar 28, 2022

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

@k8s-triage-robot
Copy link

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-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 26, 2022
@Maistho
Copy link

Maistho commented Jun 26, 2022

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 26, 2022
@k8s-triage-robot
Copy link

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-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 24, 2022
@Maistho
Copy link

Maistho commented Sep 24, 2022

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 24, 2022
@brendandburns
Copy link
Contributor

/lifecycle frozen

@k8s-ci-robot k8s-ci-robot added the lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. label Dec 16, 2022
@Maistho
Copy link

Maistho commented Dec 26, 2022

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

@brendandburns
Copy link
Contributor

@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.

@Maistho
Copy link

Maistho commented Dec 30, 2022

@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: OpenAPITools/openapi-generator#926 (comment)

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.

@brendandburns
Copy link
Contributor

brendandburns commented Dec 30, 2022

@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.

@Maistho
Copy link

Maistho commented Jun 2, 2023

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)

@bilalshaikh42
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness.
Projects
None yet
Development

No branches or pull requests