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

Add support for LocalDate as Path-/QueryParam #15451

Merged
merged 1 commit into from Mar 12, 2021
Merged

Add support for LocalDate as Path-/QueryParam #15451

merged 1 commit into from Mar 12, 2021

Conversation

ghost
Copy link

@ghost ghost commented Mar 3, 2021

closes #5860

@quarkus-bot quarkus-bot bot added the area/rest label Mar 3, 2021
@ghost ghost changed the title Feat/paramconverter for localdate ParamConverter for LocalDate Mar 3, 2021
@ghost ghost changed the title ParamConverter for LocalDate Add ParamConverter for LocalDate Mar 3, 2021
@geoand
Copy link
Contributor

geoand commented Mar 4, 2021

Thanks for this!

I just now (sorry about that...) realized that using such parameter converters causes a performance penalty, which in this case we can avoid - but it requires a deeper integration into Quarkus.
If you are up for a slightly bigger challenge, I can tell you what needs to be updated.

@ghost
Copy link
Author

ghost commented Mar 4, 2021

@geoand
If there is no hurry in getting this implemented you can give further instructions.

@geoand
Copy link
Contributor

geoand commented Mar 5, 2021

Sure, I'll try to write so pointers over the weekend

@geoand
Copy link
Contributor

geoand commented Mar 8, 2021

Here is what needs to be done at a high lever:

Add a similar check like is done at:

https://github.com/quarkusio/quarkus/blob/master/independent-projects/resteasy-reactive/common/processor/src/main/java/org/jboss/resteasy/reactive/common/processor/EndpointIndexer.java#L884

where you will check to see if the type is LocalDate.
If it is, you do pretty much the same thing as is done for PATH_SEGMENT, except that you need to implement a new org.jboss.resteasy.reactive.server.core.parameters.converters named LocalDataParamConverter. The implementation should be straightforward if you see what is done for in PathSegmentParamConverter.

That should be enough to get the LocalDate handling registered in RESTEasy Reactive registered without any performance penantly

@ghost
Copy link
Author

ghost commented Mar 11, 2021

@geoand
Thanks for the pointers! Locally, it is working now. Could you please review the code?

Do you have a good template for the unit tests? And where should the tests be placed?

I think it makes sense to test at least the following scenarios:

  1. LocalDate is working out-of-the-box with ISO-8601 format as PathParam and QueryParam
  2. If there exists a user-defined ParamConverter for LocalDate (e.g. for ISO_WEEK_DATE) this one wins

@ghost ghost changed the title Add ParamConverter for LocalDate Add support for LocalDate as Path-/QueryParam Mar 12, 2021
@geoand
Copy link
Contributor

geoand commented Mar 12, 2021

Also, when you are done, please squash the commits

@ghost
Copy link
Author

ghost commented Mar 12, 2021

@geoand
Added tests for LocalDate as PathParam and QueryParam. Also added a test-case with a custom ParamConverterProvider for LocalDate in week-year format.

Regarding Native:
The other tests do not seem to have tests for the native-image. This can be skipped for this feature?
At least I tested the changes locally with a native runnable (Linux) and everything worked fine.

Squashed the commits locally and force-pushed. Is this okay?

Anything left?

@ghost ghost marked this pull request as ready for review March 12, 2021 10:05
Copy link
Contributor

@geoand geoand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks a lot!

@geoand geoand added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Mar 12, 2021
@geoand geoand merged commit ee1fafc into quarkusio:master Mar 12, 2021
@quarkus-bot quarkus-bot bot added this to the 1.13 - master milestone Mar 12, 2021
@ghost ghost deleted the feat/paramconverter-for-localdate branch March 12, 2021 18:50
@famod famod removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Nov 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot deserialize LocalDate in @QueryParam due to missing ParamConverterProvider
2 participants