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

MicroTime serialises incorrectly #3240

Closed
LiamClarkeNZ opened this issue Jun 14, 2021 · 2 comments · Fixed by #3299
Closed

MicroTime serialises incorrectly #3240

LiamClarkeNZ opened this issue Jun 14, 2021 · 2 comments · Fixed by #3299
Assignees
Milestone

Comments

@LiamClarkeNZ
Copy link

LiamClarkeNZ commented Jun 14, 2021

As per my investigation recorded on this ticket here, io.fabric8.kubernetes.api.model.MicroTime serialises incorrectly.

When creating an Event, fabric8 serialises MicroTime as an object but the k8s server is expecting it as a string (in RFC-3339 format).

Looking into how the generated models are derived (Go struct public members reflected -> JSON schema -> jsonschema2pojo) I can see how this arose, MicroTime is indeed a struct with a field called Time, but it serialises to/from a string on the server.

I have a workaround, which is to subclass MicroTime (thank you so much for not making it final!) and override the JSON serialisation logic (I won't embed it inline for the purposes of conciseness)

https://gist.github.com/LiamClarkeNZ/65f0888d2bcb1dfe9807c873ff4f18cb

Who would fix this?

I'd be very happy to submit a PR once I understand what a good solution looks like from the maintainers' point of view.

The question is, what does that good solution look like?

The current Go struct -> JSON schema -> pojo seems to work ideally for 99.9% of cases. v1.MicroTime looks to be a rather special case. So I can see several options.

  1. Modify the Go struct -> JSON schema step to take advantage of Go types exposing OpenAPISchemaType() and OpenAPISchemaFormat() to ensure that the Go type and the serialised type align.
  2. Special case MicroTime - exclude it from generation, and rely on a hand-crafted copy.

Special casing always feels like a bad choice, but if this struct/serialisation representation mismatch only affects one type, it might be the easiest approach. It also offers the advantage that a hand-crafted model could make it easier in converting a Java date-time representation to a K8s one.

E.g., in my Gist, the workaround requires a ZonedDateTime (as one is needed to serialise to a format with TZ) and formats the string to ensure that K8s can deserialise it, removing some burden on end users.

TL;DR - MicroTime serialisation is currently not working, I'm very happy to submit a PR to fix it, I just want to find out what maintainers think is the best way to do so :)

@rohanKanojia
Copy link
Member

rohanKanojia commented Jul 6, 2021

@LiamClarkeNZ : Thanks a lot for your detailed bug report!

You're right we can easily fix this issue by providing a hand-crafted copy of MicroTime with custom Serializers/Deserializers. When I was reading your bug report this was the solution I wanted to go forward with. We've done this in past too for classes like IntOrString

Modify the Go struct -> JSON schema step to take advantage of Go types exposing OpenAPISchemaType() and OpenAPISchemaFormat() to ensure that the Go type and the serialized type align.

This approach sounds interesting. If we can invoke this method via reflection to get openApiSchemaType that would be really nice. However, I think at this point I wonder if it can introduce some breaking changes. Do you think we should create a separate issue for discussing using OpenAPISchemaType() in generator scipt?

rohanKanojia added a commit to rohanKanojia/kubernetes-client that referenced this issue Jul 6, 2021
Add Custom MicroTime with custom Serializer/Deserializers in order to
convert to a timestamp string

Signed-off-by: Rohan Kumar <[email protected]>
rohanKanojia added a commit to rohanKanojia/kubernetes-client that referenced this issue Jul 6, 2021
Add Custom MicroTime with custom Serializer/Deserializers in order to
convert to a timestamp string

Signed-off-by: Rohan Kumar <[email protected]>
rohanKanojia added a commit to rohanKanojia/kubernetes-client that referenced this issue Jul 6, 2021
Add Custom MicroTime with custom Serializer/Deserializers in order to
convert to a timestamp string

Signed-off-by: Rohan Kumar <[email protected]>
rohanKanojia added a commit to rohanKanojia/kubernetes-client that referenced this issue Jul 6, 2021
Add Custom MicroTime with custom Serializer/Deserializers in order to
convert to a timestamp string

Signed-off-by: Rohan Kumar <[email protected]>
@LiamClarkeNZ
Copy link
Author

@rohanKanojia Yep, I think using OpenAPISchemaType() is a far more wide-ranging change in type generation, so needs more investigation. Thanks for the PR!

rohanKanojia added a commit to rohanKanojia/kubernetes-client that referenced this issue Jul 7, 2021
Add Custom MicroTime with custom Serializer/Deserializers in order to
convert to a timestamp string

Signed-off-by: Rohan Kumar <[email protected]>
rohanKanojia added a commit to rohanKanojia/kubernetes-client that referenced this issue Jul 7, 2021
Add Custom MicroTime with custom Serializer/Deserializers in order to
convert to a timestamp string

Signed-off-by: Rohan Kumar <[email protected]>
rohanKanojia added a commit to rohanKanojia/kubernetes-client that referenced this issue Jul 7, 2021
Add Custom MicroTime with custom Serializer/Deserializers in order to
convert to a timestamp string

Signed-off-by: Rohan Kumar <[email protected]>
rohanKanojia added a commit to rohanKanojia/kubernetes-client that referenced this issue Jul 21, 2021
Add Custom MicroTime with custom Serializer/Deserializers in order to
convert to a timestamp string

Signed-off-by: Rohan Kumar <[email protected]>
@manusa manusa added this to the 5.6.0 milestone Jul 21, 2021
manusa pushed a commit that referenced this issue Jul 21, 2021
Add Custom MicroTime with custom Serializer/Deserializers in order to
convert to a timestamp string

Signed-off-by: Rohan Kumar <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants