fix: properly (un)marshal custom time type

[cecobask]

Description

This PR fixes the bug #3.

I tested this change locally by adding a replace directive to the go.mod file on my feature branch:

replace github.com/open-sauced/go-api/client v0.0.0-20230825180028-30fe67759eff => github.com/cecobask/go-api/client v0.0.0-20230915140043-f201131f42d3

What type of PR is this? (check all applicable)

• 🍕 Feature
• 🐛 Bug Fix
• 📝 Documentation Update
• 🎨 Style
• 🧑‍💻 Code Refactor
• 🔥 Performance Improvements
• ✅ Test
• 🤖 Build
• 🔁 CI
• 📦 Chore (Release)
• ⏩ Revert

Related Tickets & Documents

Fixes #3.
Relates to: open-sauced/pizza-cli#38

Added tests?

• 👍 yes
• 🙅 no, because they aren't needed
• 🙋 no, because I need help

Added to documentation?

• 📜 README.md
• 📓 docs.opensauced.pizza
• 🍕 dev.to/opensauced
• 📕 storybook
• 🙅 no documentation needed

Are there any post-deployment tasks we need to perform?

Release a new version of the go-api client.

[cecobask]

Hello @jpmcb,
Please, review these changes when you get the chance.
Thank you!

[jpmcb]

Nice work tracking this down. I'm sure there are alot of sharp edges within the Go API client since it's being auto-generated out of the open-sauced/api repo via the swagger doc.

We should probably investigate if this is possible to fix in the upstream API swagger doc, which then lands in this generated API client code, instead of dropping a patch here directly in the client code (unless absolutely necessary). Although, it's not immediately clear to me how we should go about doing this since rolling the client already takes abit of hand rolling and we don't want to get bogged down with reinventing alot of tech to accurately represent the API.

Any ideas @cecobask ?

Let me take a deep dive on this and see if anything comes up in some investigation.

[jpmcb]

Isn't using only time.DateOnly pretty rigid? Right now I believe we only use this for PR "days" but in the future, if we use other "date-time" formats, which should follow the very general internet date/time format, this will force any date-time to always just be "days"?

[cecobask]

The parsing that I implemented doesn't account for time. I should have renamed the type to Date here.
If we go forward with the patching, I think it might be better to have 2 separate struct types - Date & DateTime.

[cecobask]

Nice work tracking this down. I'm sure there are alot of sharp edges within the Go API client since it's being auto-generated out of the open-sauced/api repo via the swagger doc.

We should probably investigate if this is possible to fix in the upstream API swagger doc, which then lands in this generated API client code, instead of dropping a patch here directly in the client code (unless absolutely necessary). Although, it's not immediately clear to me how we should go about doing this since rolling the client already takes abit of hand rolling and we don't want to get bogged down with reinventing alot of tech to accurately represent the API.

Any ideas @cecobask ?

Let me take a deep dive on this and see if anything comes up in some investigation.

PullRequestsServiceAPI.GetPullRequestInsights is the only endpoint that references the DateTime type. It was a manual patch done about a month ago.
I believe this manual patch was done for the following reasons:

Generating the swagger file based on this entity src/pull-requests/entities/pull-request-insight.entity.ts results in:

DbPRInsight:
  type: object
  properties:
    ...
    day:
      type: date-time
    ...

date-time is not a valid data type: https://swagger.io/docs/specification/data-models/data-types.

Afterward, when generating the go client, based on the swagger file that was previously generated results in:

type DbPRInsight struct {
  ...
  Day DateTime `json:"day"`
  ...
}

This is when the need for a custom type DateTime appears. Originally, this type was undefined as the open api generator doesn't know how to handle it.

I think it will be better if we define the day property in swagger as shown below:

DbPRInsight:
  type: object
  properties:
    ...
    day:
      type: string
      format: date
    ...

After this is done, I could come up with two options:

1. Use the generated go client as is. This means we have to get rid of the custom DateTime type. The open api generator defines the day property as type string. The responsibility for parsing the property to type time.Time will be shifted to the users of the go client
2. Define a custom type called Date and patch the generated go client to use the custom type where needed. This can be kind of hacky and difficult to maintain though

@jpmcb, let me know what you think is best here. I can implement these changes once I receive a confirmation 👍🏻

[jpmcb]

Great deep dive on this.

The original patch in the API was intended to align the TypeSciprt types with the Swagger decorates so that we could consume the swagger doc as a client generator.

date-time is a special string format that swagger uses to give a clue to clients as to what the type might be:
https://swagger.io/docs/specification/data-models/data-types/#String-Formats

String Formats

An optional format modifier serves as a hint at the contents and format of the string. OpenAPI defines the following built-in string formats:
...
date-time – the date-time notation as defined by RFC 3339, section 5.6, for example, 2017-07-21T17:32:28Z

And yes, you're right, because the swagger generator didn't intelligently convert the date-time string to a golang decorated time.Time, I made the custom type in order to satisfy the generated code bits.

think it will be better if we define the day property in swagger as shown below:

 type: string

The responsibility for parsing the property to type time.Time will be shifted to the users of the go client

That's probably more agnostic in the end anyways instead of trying to wrangle the go client bits to have aligned types with what's generated by the swagger doc.

Again, thanks so much for the deep dive on this, really really helpful as we iron out the kinks and the process with how the client gets generated. If you want to take a stab at this, go for it! Let me know what other sharp edges you hit so we can iterate on the client generation.

[cecobask]

Closing in favor of #5.