microsoft / kiota-java

Java libraries for Kiota-generated API clients.
https://aka.ms/kiota/docs
MIT License
27 stars 29 forks source link

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

Closed baywet closed 1 month ago

baywet commented 2 months ago

related https://github.com/std-uritemplate/std-uritemplate/pull/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)

espenvis commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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:

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 commented 1 month ago

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

https://github.com/microsoft/kiota-java/blob/855baeb30b947abfbd9e493c46856f472b5eacfa/components/abstractions/src/main/java/com/microsoft/kiota/RequestInformation.java#L459

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

https://github.com/microsoft/kiota-java/blob/855baeb30b947abfbd9e493c46856f472b5eacfa/components/abstractions/src/test/java/com/microsoft/kiota/RequestInformationTest.java#L55

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

espenvis commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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?

baywet commented 1 month ago

Thank you for your patience. Yes, here are the formats / type mapping in Java

OAS format Python Type
date LocalDate
date-time OffsetDateTime
duration PeriodAndDuration
time LocalTime

Thanks for the great work! Let us know if you have any additional comments or questions.