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 plain Time fields #28474

Merged
merged 1 commit into from
Jan 24, 2025

Conversation

OmarHawk
Copy link
Contributor

@OmarHawk OmarHawk commented Jan 14, 2025

Fix #28406


Please make sure the below checklist is followed for Pull Requests.

When you are still working on the PR, consider converting it to Draft (below reviewers) and adding skip-ci label, you can still see CI build result at your branch.

@OmarHawk OmarHawk force-pushed the feature/plain-time-fields branch from 2d7da77 to 27dd745 Compare January 15, 2025 12:43
@OmarHawk
Copy link
Contributor Author

OmarHawk commented Jan 15, 2025

The (new) test that are now failing are an inconsistency between the results of LocalTime.toString and how Spring formats this data type during output of DTOs within Rest Controllers. I'll investigate a bit more and see what a good approach would be for that.

E.g.:

Error:    FieldTestServiceClassAndJpaFilteringEntityResourceIT.getAllFieldTestServiceClassAndJpaFilteringEntitiesByIntegerRequiredBobIsLessThanOrEqualToSomething:1454->defaultFieldTestServiceClassAndJpaFilteringEntityFiltering:4177->defaultFieldTestServiceClassAndJpaFilteringEntityShouldBeFound:4219 JSON path "$.[*].localTimeBob"
Expected: a collection containing "12:00"
     but: mismatches were: [was "12:00:00"]

Expected => LocalTime.toString
Received => Spring Boot formatting of LocalTime (Jackson)

@OmarHawk OmarHawk force-pushed the feature/plain-time-fields branch from 126c0de to 1738615 Compare January 15, 2025 15:54
@@ -37,7 +43,14 @@ public class JacksonConfiguration {
*/
@Bean
public JavaTimeModule javaTimeModule() {
return new JavaTimeModule();
final JavaTimeModule javaTime = new JavaTimeModule();
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 is due to our ResourceIT compare the .toString() values and don't use a specific serializer for comparison of the inserted vs. returned values.
"Fixes" the issue, but maybe there is a cleaner solution to that. Anyway it actually does not make sense, what Jackson produces by default here, i.e. outputting seconds when the second value is 0, e.g. from 11:00 Jackson always produces 11:00:00.

@OmarHawk OmarHawk force-pushed the feature/plain-time-fields branch from 6cf2ac3 to 9aa75bb Compare January 15, 2025 16:09
@OmarHawk
Copy link
Contributor Author

Seems like React is still having some trouble. I'll generate a sample application locally tomorrow and see if I can debug this as the output of the failing tests do not really help here.

Angular is only red due to Sonar errors regarding complexity of methods, shoould be fine as this is nothing I can influence here. Rest is "green".

@OmarHawk OmarHawk force-pushed the feature/plain-time-fields branch from e0e3018 to 9cb202a Compare January 16, 2025 09:17
OmarHawk added a commit to OmarHawk/jhipster.github.io that referenced this pull request Jan 16, 2025
@OmarHawk
Copy link
Contributor Author

Now React works (had some issue with field validation when the inserted value contained seconds), but seems like h2 & MariaDB do have issues with LocalTime fields if seconds are not defined (meaning they are set to 0 in LocalTime field.) I'll try to reproduce again locally and see if there is something I can do about it.

@OmarHawk
Copy link
Contributor Author

Now React works (had some issue with field validation when the inserted value contained seconds), but seems like h2 & MariaDB do have issues with LocalTime fields if seconds are not defined (meaning they are set to 0 in LocalTime field.) I'll try to reproduce again locally and see if there is something I can do about it.

Seemed to be related only to liquibase inserting fake data at these two databases. When passing :00 as seconds for the fake data, it works for React (as it then sets the input value properly when editing entity elements making the form be submittable without validation errors) and also for the insertion of fake data by liquibase.

@OmarHawk OmarHawk force-pushed the feature/plain-time-fields branch from c59d929 to bdd352a Compare January 16, 2025 17:07
@OmarHawk OmarHawk marked this pull request as ready for review January 16, 2025 17:42
@OmarHawk
Copy link
Contributor Author

Can be reviewed now.

mshima
mshima previously approved these changes Jan 18, 2025
Copy link
Member

@mshima mshima left a comment

Choose a reason for hiding this comment

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

small format adjusts

@OmarHawk OmarHawk force-pushed the feature/plain-time-fields branch from bdd352a to d0b5a96 Compare January 20, 2025 07:41
@OmarHawk
Copy link
Contributor Author

Adjusted the requested formatting changes. :-)

@OmarHawk OmarHawk requested a review from mshima January 20, 2025 10:11
@mshima mshima requested review from mraible and DanielFran January 21, 2025 13:12
@DanielFran DanielFran merged commit 31ff1c5 into jhipster:main Jan 24, 2025
68 checks passed
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.

Support for plain Time fields
3 participants