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

prepare for std uri template 2 and date time drop #1583

Open
baywet opened this issue Sep 24, 2024 · 8 comments · May be fixed by #1586
Open

prepare for std uri template 2 and date time drop #1583

baywet opened this issue Sep 24, 2024 · 8 comments · May be fixed by #1586
Labels
enhancement New feature or request WIP

Comments

@baywet
Copy link
Member

baywet commented Sep 24, 2024

related std-uritemplate/std-uritemplate#245
The following types need to the normalized to their RFC3339 representation before they are passed to std URI template when they are present are query or path parameter values
Date, OffsetDateTime their collection representations (array, List)

@baywet baywet added the enhancement New feature or request label Sep 24, 2024
@espenvis
Copy link

If your OpenAPI specification specifies date format for strings rather than date-time, Kiota will get a generated client using a date-only type, for instance the LocalDate type in Java. This is good.

However, std-uritemplate has historically in my projects required patchwork from the end-user to allow the generated Kiota code to function, because it does not have native support for any other formats besides date-time (in Java, OffsetDateTime) - thus the generated Kiota API will not function out of the box when an API specifies date as a path- or query parameter.

With the burden of transforming into the date-time format (i.e. OffsetDateTime.toString()) falling on Kiota, will we perhaps also see support for date natively in Kiota?

@baywet
Copy link
Member Author

baywet commented Sep 26, 2024

Thanks for joining the conservation.
Are you saying that local dates are not being normalized properly when they are query or path parameters in the URI today?

@espenvis
Copy link

espenvis commented Sep 27, 2024

Thanks for joining the conservation. Are you saying that local dates are not being normalized properly when they are query or path parameters in the URI today?

That is correct. std-uritemplate will thrown an exception when attempting to normalize LocalDate.

In std-uritemplate, it was a design decision to support only date-time, so an exception being thrown for LocalDate is expected behavior. However, this design decision is not compatible with OpenAPI's definition of supported string formats, where RFC 3339 full-date (OpenAPI's date) and RFC 3339 date-time (of the same name in OpenAPI) are specified.

It's accurate for Kiota to generate a client with the LocalDate type, but the above disparity causes erroneous behavior. So with the burden of normalization now falling on Kiota, we have a good opportunity to mend this.

For Java:

  • We use LocalDate to represent RFC 3339 full-date in accordance with OpenAPI's specification for the date format
  • We use OffsetDateTime to represent the RFC 3339 date-time in accordance with OpenAPI's own date-time
  • We may want to support Date seeing as this type was also supported in std-uritemplate.*

Both LocalDate and OffsetDateTime can be normalized with their respective toString methods, then they are presumably passed into std-uritemplate.

For Date, you can use the same implementation as std-uritemplate when support was maintained for it.

* This would be necessary only if Date may exist as a type used in Kiota code generation. That way we'd ensure newly generated clients of the same specification as an earlier won't have breaking changes. I'll leave it to you to figure out if this is necessary. I assume it may not be.

@baywet
Copy link
Member Author

baywet commented Sep 27, 2024

Thank you for the additional information.

That was an oversight from our side when moving over to std uri template. Thanks for catching that!

The implementation is here

private static Object getSanitizedValues(Object value) {

And unit tests like this one can be added to validate behaviour.

Is this something you'd like to submit a pull request for provided some guidance?

@espenvis
Copy link

I'm happy to make a pull request if it helps speed up things. Tell me if there's anything I should know, I'll clone the repository and have a look.

@baywet
Copy link
Member Author

baywet commented Sep 27, 2024

Besides needing gradle 8+ and JDK 21+ you should have everything you need.
Let us know if you have any additional comments or questions.

@espenvis
Copy link

I've gone ahead and implemented it. Added a test for LocalDate and kept the existing test for OffsetDateTime. Both tests pass.

Seems that a simple toString will resolve into an ISO 8601 string, and will always return the shortest possible compliant string which broke your test, so I went with ISO_OFFSET_DATE_TIME and ISO_LOCAL_DATE for OffsetDateTime and LocalDate respectively.

@espenvis
Copy link

Hello,

I recognize there might be more time-related types desirable to implement support for here, for example duration and time. I can add these too if it's of interest.

What's the scope of support for formats? Do you have any conception of which formats will be handled here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request WIP
Projects
Status: Todo 📃
Development

Successfully merging a pull request may close this issue.

2 participants