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.

Sign up for GitHub

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

Jump to bottom

handle optional in Jackson #10781

Merged
merged 1 commit into from
Sep 16, 2024
Merged

handle optional in Jackson #10781

merged 1 commit into from
Sep 16, 2024

Conversation

fabienpuissant
Copy link
Collaborator

create bean Jdk8Module and ObjectMapper

Fix #10751

@fabienpuissant
Copy link
Collaborator Author

@DamnClin
Jackson was not available in the spring boot core module.
In a first version I've just imported jdk8Module from jackson-datatype-jdk8 but in the end, I preferred use the existing starter web

I don't know what do you think about it

@fabienpuissant fabienpuissant force-pushed the feature/handle-optional-in-jackson branch from 1d56351 to 40d3759 Compare September 8, 2024 01:25
@fabienpuissant
Copy link
Collaborator Author

@DamnClin Jackson was not available in the spring boot core module. In a first version I've just imported jdk8Module from jackson-datatype-jdk8 but in the end, I preferred use the existing starter web

I don't know what do you think about it

Importing the starter-web leads to side effects (especially with Spring cloud), so I just import jackson-datatype-jdk8

@fabienpuissant fabienpuissant force-pushed the feature/handle-optional-in-jackson branch 2 times, most recently from 7ace8bb to b49362a Compare September 8, 2024 02:11
@DamnClin
Copy link
Collaborator

DamnClin commented Sep 8, 2024

@DamnClin Jackson was not available in the spring boot core module. In a first version I've just imported jdk8Module from jackson-datatype-jdk8 but in the end, I preferred use the existing starter web

I don't know what do you think about it

This is probably the wrong module to add it so :). There should be a module with this dependency already on and his must be done here.

We can't add the web starter in this module if it wasn't there since SpringBoot can be use for other needs (like batch), my bad on identifying the sweet spot

murdos
murdos reviewed Sep 8, 2024
View reviewed changes
src/main/resources/generator/dependencies/pom.xml Outdated
@@ -28,6 +28,7 @@
<liquibase.version>4.29.2</liquibase.version>
<reflections.version>0.10.2</reflections.version>
<json-web-token.version>0.12.6</json-web-token.version>
<jackson-datatype-jdk8.version>2.17.2</jackson-datatype-jdk8.version>
Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove the changes from this file, they're not used anymore

@fabienpuissant fabienpuissant force-pushed the feature/handle-optional-in-jackson branch from b49362a to 3ef9973 Compare September 9, 2024 11:30
murdos
murdos requested changes Sep 14, 2024
View reviewed changes
...h/jhipster/lite/generator/server/springboot/mvc/web/domain/SpringBootMvcsModulesFactory.java Show resolved Hide resolved
...gboot/jackson/test/wire/jackson/infrastructure/config/JacksonConfigurationTest.java.mustache Outdated
Optional<String> optional = Optional.of("test");
ObjectMapper objectMapper = new ObjectMapper();
objectMapper.registerModule(jacksonConfiguration.jdk8Module());
assertThat(objectMapper.writeValueAsString(optional)).isEqualTo("\"test\"");
Copy link
Contributor

Choose a reason for hiding this comment

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

Here you're just testing the Jdk8Module which is an external dependency, so there's no real reason to test it again.
What could be more interesting is an integration test where you inject the ObjectMapper provided by Spring Boot, and assert that it indeed correctly map Optional.

@fabienpuissant fabienpuissant force-pushed the feature/handle-optional-in-jackson branch from 3ef9973 to d253191 Compare September 14, 2024 18:00
murdos
murdos previously approved these changes Sep 14, 2024
View reviewed changes
@fabienpuissant fabienpuissant force-pushed the feature/handle-optional-in-jackson branch from d253191 to b7f47ca Compare September 14, 2024 21:01
@fabienpuissant fabienpuissant force-pushed the feature/handle-optional-in-jackson branch 6 times, most recently from c6e1383 to 5a5003e Compare September 15, 2024 03:22
@fabienpuissant
Copy link
Collaborator Author

To avoid errors, use default spring boot jackson configuration and enrich with jdk8Module
Source : https://github.com/spring-projects/spring-boot/blob/main/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/jackson/JacksonAutoConfiguration.java

murdos
murdos reviewed Sep 16, 2024
View reviewed changes
...pringboot/jackson/main/wire/jackson/infrastructure/config/JacksonConfiguration.java.mustache Outdated
class JacksonConfiguration {

@Bean
public ObjectMapper objectMapper(Jackson2ObjectMapperBuilder builder) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I've just tested it, the following code is sufficient and more extensible (since it doesn't overwrite SpringBoot configuration):

@Bean
  Jdk8Module jdk8Module() {
    return new Jdk8Module();
  }

Copy link
Contributor

Choose a reason for hiding this comment

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

It will be injected in the list of modules here: https://github.com/spring-projects/spring-boot/blob/74ddea416b8a0d2c12e289116e75aace3c70acf2/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/jackson/JacksonAutoConfiguration.java#L179

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

true xD
Kinda dark magic

murdos
murdos reviewed Sep 16, 2024
View reviewed changes
...h/jhipster/lite/generator/server/springboot/mvc/web/domain/SpringBootMvcsModulesFactory.java Outdated
@@ -23,6 +23,10 @@ public class SpringBootMvcsModulesFactory {
private static final JHipsterSource MAIN_SOURCE = SOURCE.append("main");
private static final JHipsterSource TEST_SOURCE = SOURCE.append("test");

private static final JHipsterSource JACKSON_MAIN_SOURCE = from("server/springboot/jackson/main");
private static final JHipsterSource JACKSON_TEST_SOURCE = from("server/springboot/jackson/test");
private static final String WIRE_JACKSON_CONFIG = "wire/jackson/infrastructure/config";
Copy link
Contributor

@murdos murdos Sep 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Subfolders of infrastructure folder are either primary or secondary:

Suggested change
private static final String WIRE_JACKSON_CONFIG = "wire/jackson/infrastructure/config";
private static final String WIRE_JACKSON_CONFIG = "wire/jackson/infrastructure/primary";

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

image

Copy link
Contributor

Choose a reason for hiding this comment

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

You need the update the JacksonConfiguration.java content too:
package {{packageName}}.wire.jackson.infrastructure.config; -> package {{packageName}}.wire.jackson.infrastructure.primary;

murdos
murdos reviewed Sep 16, 2024
View reviewed changes
...ipster/lite/generator/server/springboot/mvc/web/domain/SpringBootMvcsModulesFactoryTest.java Outdated
@@ -145,6 +148,11 @@ private JHipsterModuleAsserter assertMvcModule(JHipsterModule module) {
</dependency>
"""
)
.and()
.hasFile("src/main/java/tech/jhipster/jhlitest/wire/jackson/infrastructure/config/JacksonConfiguration.java")
.containing("public ObjectMapper objectMapper(Jackson2ObjectMapperBuilder builder)")
Copy link
Contributor

Choose a reason for hiding this comment

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

Usually we test content of a file when it's a template and we want to check the dynamic content.
Here it's not needed since this is a static content handled in the template.

@fabienpuissant fabienpuissant force-pushed the feature/handle-optional-in-jackson branch from 5a5003e to 513701c Compare September 16, 2024 20:28
@fabienpuissant fabienpuissant force-pushed the feature/handle-optional-in-jackson branch 2 times, most recently from e8562aa to c303f06 Compare September 16, 2024 20:48
@fabienpuissant
handle optional in Jackson
c21b940 
create bean Jdk8Module and ObjectMapper

Fix jhipster#10751
@fabienpuissant fabienpuissant force-pushed the feature/handle-optional-in-jackson branch from c303f06 to c21b940 Compare September 16, 2024 21:33
@murdos murdos enabled auto-merge September 16, 2024 21:37
@murdos murdos merged commit fdf6ed8 into jhipster:main Sep 16, 2024
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@murdos murdos murdos approved these changes

Assignees
No one assigned
Labels
area: enhancement 🔧 server: spring boot
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Handle Optional in Jackson
4 participants
@fabienpuissant @DamnClin @murdos @pascalgrimaud