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

Remove LocalDate and LocalDateTime converters #11381

Closed
wants to merge 2 commits into from

Conversation

radovanradic
Copy link
Contributor

I think this is causing the issue in micronaut-data micronaut-projects/micronaut-data#2096 and is discussed here micronaut-projects/micronaut-data#3241

@@ -209,8 +209,8 @@ public void register(MutableConversionService conversionService) {
addTemporalToDateConverter(conversionService, ZonedDateTime.class, ZonedDateTime::toInstant);
// these two are a bit icky, but required for yaml parsing compatibility
// TODO Micronaut 4 Consider deletion
addTemporalToDateConverter(conversionService, LocalDate.class, ld -> ld.atTime(0, 0).toInstant(ZoneOffset.UTC));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line is converting LocaleDate to Date using UTC and micronaut-data is using default timezone when converting back so we are getting date diff issues. I believe this should use default timezone as well.
There is also comment about removal of this conversion in Micronaut4. Can it be removed? If yes, then we can add converter needed in micronaut-data using default timezone and the issue would be fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yawkat Can we remove these conversions above, or change it to not use UTC but default timezone? It is causing issue in micronat-data explained in the description of this PR.

Copy link
Member

Choose a reason for hiding this comment

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

I disagree that this should be default TZ. Yes they should be removed, but using system TZ is worse.

Why does data need to convert these at all? java.sql.Date has actual well-defined conversions from and to LocalDate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

micronaut-data registers various conversions for dates and is always using timezones.
With this converter defined in micronaut-core, even if we register this in micronaut-data

conversionService.addConverter(java.sql.Date.class, LocalDate.class, date -> date.toLocalDate());

we will get wrong date when converted back.

I removed these converters from micronaut-core, hopefully nothing will break.

Copy link

sonarcloud bot commented Nov 25, 2024

@radovanradic radovanradic marked this pull request as ready for review November 25, 2024 08:55
@radovanradic radovanradic changed the title Fix LocalDate and LocalDateTime conversion to Date Remove LocalDate and LocalDateTime converters Nov 25, 2024
@yawkat
Copy link
Member

yawkat commented Nov 25, 2024

So the java.util.Date converter is taking priority over the java.sql.Date converter, even when using java.sql.Date? Seems like a bug in ConversionService

@radovanradic
Copy link
Contributor Author

So the java.util.Date converter is taking priority over the java.sql.Date converter, even when using java.sql.Date? Seems like a bug in ConversionService

No, I think this micronaut-data block is causing that

if (value instanceof Date date) {
                    return setDate(statement, index, date);
                } else {
                    return setDate(statement, index, convertRequired(value, Date.class));
                }

it is converting to java.util.Date and not java.sql.Date and this is why it finds this converter in core.

@yawkat
Copy link
Member

yawkat commented Nov 25, 2024

Then data should convert to the right type, or add special cases where it needs different conversions from core. It should not require a core change

@radovanradic
Copy link
Contributor Author

Then data should convert to the right type, or add special cases where it needs different conversions from core. It should not require a core change

Yes, I will close this PR as not needed and fix issue in micronaut-data. Thanks

@radovanradic radovanradic deleted the localdate-conversion branch November 25, 2024 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants