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

Fixed conversion string to temporalAmount - Duration or Period #11581

Merged

Conversation

altro3
Copy link
Contributor

@altro3 altro3 commented Feb 10, 2025

Fixed #11577

@graemerocher
Copy link
Contributor

@altro3 can you target 4.8.x?

@sdelamo
Copy link
Contributor

sdelamo commented Feb 10, 2025

@altro3 can you target 4.8.x?

I asked @altro3 to target 4.7.x as it was a bug introduced in 4.7.x. I will merge up @graemerocher

@sdelamo sdelamo requested a review from dstepanov February 10, 2025 12:57
@sdelamo sdelamo added the type: bug Something isn't working label Feb 10, 2025
@dstepanov
Copy link
Contributor

Why conversionService.convert("string", Duration) is triggering TemporalAccessor converter?

@altro3
Copy link
Contributor Author

altro3 commented Feb 11, 2025

@dstepanov @graemerocher @sdelamo I would like to clarify that there is currently a bug in the Cookie interface in the maxAge method.

The argument is of the wrong type - TemporalAmount , but it should be Duration , because we are interested in the value in seconds, and Period stores the timestamp in days. From the TemporalAmount documentation:

* There are two common implementations.
* {@link Period} is a date-based implementation, storing years, months and days.
* {@link Duration} is a time-based implementation, storing seconds and nanoseconds,
* but providing some access using other duration based units such as minutes,
* hours and fixed 24-hour days.

I don't understand why TemporalAmount is there.

@sdelamo
Copy link
Contributor

sdelamo commented Feb 11, 2025

I would like to clarify that there is currently a bug in the Cookie interface in the maxAge method.
The argument is of the wrong type - TemporalAmount , but it should be Duration , because we are interested in the value in seconds, and Period stores the timestamp in days. From the TemporalAmount documentation:

but this should be a different pr right?

@altro3
Copy link
Contributor Author

altro3 commented Feb 11, 2025

@sdelamo as you wish - I can add this correction to this PR too. :-) . Current fix - solve the problem. I just wanted to say that it is actually not correct to use TemporalAmount in this case

@altro3 altro3 force-pushed the temporal-amount-to-duration branch from 45592b0 to e747ff3 Compare February 14, 2025 13:41
@altro3
Copy link
Contributor Author

altro3 commented Feb 14, 2025

@sdelamo added this fix here. Don't think that we need to create different PR for it

@altro3 altro3 force-pushed the temporal-amount-to-duration branch from e747ff3 to 2fd3e10 Compare February 14, 2025 14:49
@altro3
Copy link
Contributor Author

altro3 commented Feb 14, 2025

@sdelamo can't fix it here, becuse also need to change micronaut-session module for it

@sdelamo sdelamo merged commit 8db544d into micronaut-projects:4.7.x Feb 17, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants