PR #382: Added spring-boot autoconfiguration support. #526

vadeg · 2017-10-28T14:41:02Z

[PR #382] : Spring boot autoconfiguration support.

Spring boot autoconfiguration module added.
Spring boot starter module added.
Added some integration tests for checking Dozer autoconfiguration.
Test logging set to "WARN" when tests runs using maven-surefire-plugin. It increases logs readability.

Issue link

    [PR: #382] Provide spring-boot support

Purpose

Provide spring-boot autoconfiguration support for Dozer.

Approach

Simplifies Dozer configuration when using spring-boot framework.

Open Questions and Pre-Merge TODOs

Issue created
Unit tests pass
Documentation updated
Travis build passed

Fixes PR#382 - Spring boot autoconfiguration module added. - Spring boot starter module added. - Added some integration tests for checking Dozer autoconfiguration. - Test logging set to "WARN" when tests runs using 'maven-surefire-plugin'. It increases logs readability.

garethahealy · 2017-10-29T17:54:20Z

dozer-spring-boot-auto-configuration/dozer-spring-boot-autoconfigure/pom.xml

+
+    <name>Spring Boot AutoConfiguration :: Dozer AutoConfiguration</name>
+
+    <properties>


These properties shouldn't be needed, as provided by dozer-parent

garethahealy · 2017-10-29T17:54:31Z

dozer-spring-boot-auto-configuration/dozer-spring-boot-autoconfigure/pom.xml

+        <dependency>
+            <groupId>org.springframework.boot</groupId>
+            <artifactId>spring-boot-configuration-processor</artifactId>
+            <optional>true</optional>


why is this optional?

Spring boot documentation recommends to do like this. I think this is just for exclude transitive dependency from spring-boot-configuration-processor

garethahealy · 2017-10-29T17:55:30Z

dozer-spring-boot-auto-configuration/dozer-spring-boot-autoconfigure/pom.xml

+            <groupId>org.springframework.boot</groupId>
+            <artifactId>spring-boot-starter-logging</artifactId>
+        </dependency>
+        <dependency>


junit is added by dozer-parent, so can be removed here.

garethahealy · 2017-10-29T17:55:46Z

dozer-spring-boot-auto-configuration/dozer-spring-boot-autoconfigure/pom.xml

+        </dependency>
+    </dependencies>
+
+    <build>


same as others, dozer-parent adds surefire.

garethahealy · 2017-10-29T17:56:42Z

...autoconfigure/src/main/java/com/github/dozermapper/autoconfigure/DozerAutoConfiguration.java

+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package com.github.dozermapper.autoconfigure;


can you update package to include springboot in name. i.e.: com.github.dozermapper.springboot.autoconfigure

garethahealy · 2017-10-29T18:01:52Z

dozer-spring-boot-auto-configuration/dozer-spring-boot-starter/pom.xml

+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+    xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+    <parent>
+        <artifactId>dozer-spring-boot-auto-configuration</artifactId>


can you just switch the order, i.e.: groupid and then artifact

garethahealy · 2017-10-29T18:02:01Z

dozer-spring-boot-auto-configuration/dozer-spring-boot-autoconfigure/pom.xml

+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+    xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+    <parent>
+        <artifactId>dozer-spring-boot-auto-configuration</artifactId>


can you just switch the order, i.e.: groupid and then artifact

garethahealy · 2017-10-29T18:02:47Z

dozer-spring-boot-auto-configuration/pom.xml

+            <groupId>com.github.dozermapper</groupId>
+            <artifactId>dozer-spring</artifactId>
+            <version>${project.version}</version>
+            <optional>true</optional>


whys this optional?

Because spring boot documentation recommends to do like this just to remove transitive dependencies. It is not an auto-configuration module's responsibility to provide needed libraries. It is up to the project dependencies. If there is no proper dependency autoconfiguration will be just ignored.

Agree that providing dependencies is not a responsibility of autoconfiguration. But it is of the starter, the same documentation states:

The starter module that provides a dependency to the autoconfigure module as well as the library and any additional dependencies that are typically useful. In a nutshell, adding the starter should be enough to start using that library.
...
Its only purpose is to provide the necessary dependencies to work with the library; see it as an opinionated view of what is required to get started.

Thus I would recommend to include autoconfiguration and dozer-spring as compile-time dependencies to the starter module.

And then I would also switch dependencies in autoconfiguration module from optional to provided, because optional has different semantic in Maven, basically meaning "dependencies required by optional feature", while autoconfiguration will not work at all unless Dozer library is provided.

garethahealy · 2017-10-29T18:02:54Z

dozer-spring-boot-auto-configuration/pom.xml

+            <groupId>com.github.dozermapper</groupId>
+            <artifactId>dozer-core</artifactId>
+            <version>${project.version}</version>
+            <optional>true</optional>


whys this optional?

Please see explanation in previous comment.

garethahealy · 2017-10-29T18:03:49Z

pom.xml

@@ -81,6 +81,7 @@
        <module>spring</module>
        <module>proto</module>
        <module>tests</module>
+        <module>dozer-spring-boot-auto-configuration</module>


can this be renamed to: spring-boot

I would call it dozer-spring-boot just to have clear relation between module folder name and module name.

Actually, I would do the same with dozer-spring module. Having dozer-spring module in spring folder a little bit confusing :)

I would also appreciate having folder names the same as module names.
In addition, to me the perfect structure of Spring-related modules would be like this:

dozer |- dozer-spring-support (currently does not exist, aggregate module) | |- dozer-spring (existing module from spring folder) | |- dozer-spring-boot-starter (newly introduced module) | |- dozer-spring-boot-autoconfigure (newly introduced module) |- ...

i'd suggest to change the structure as suggested by @orange-buffalo to another PR, and keep this focused on boot.

garethahealy · 2017-10-29T18:07:50Z

@vadeg ; have added some comments. nothing major.

@orange-buffalo ; have you guys started using springboot? any comments welcome.

orange-buffalo

@garethahealy not yet, but we have some plans to move from OSGi to spring-boot based services.
I have added my comments to the review.

orange-buffalo · 2017-10-31T14:21:12Z

plugins-parent/pom.xml

@@ -289,6 +289,9 @@
                    <excludes>
                        <exclude>**/Test*.java</exclude>
                    </excludes>
+                    <systemPropertyVariables>
+                        <org.slf4j.simpleLogger.defaultLogLevel>WARN</org.slf4j.simpleLogger.defaultLogLevel>


imho, this change is too strong. I would say that running tests with Info level is okay and often useful. If there is a need to disable info output for some particular tests, this should be done on per-test or at least per-module basis.

Agree, but I think log output should be more accurate. Even Travis-CI complaining that log is too long.

orange-buffalo · 2017-10-31T14:54:52Z

pom.xml

@@ -81,6 +81,7 @@
        <module>spring</module>
        <module>proto</module>
        <module>tests</module>
+        <module>dozer-spring-boot-auto-configuration</module>


I would also appreciate having folder names the same as module names.
In addition, to me the perfect structure of Spring-related modules would be like this:

dozer |- dozer-spring-support (currently does not exist, aggregate module) | |- dozer-spring (existing module from spring folder) | |- dozer-spring-boot-starter (newly introduced module) | |- dozer-spring-boot-autoconfigure (newly introduced module) |- ...

orange-buffalo · 2017-10-31T14:59:12Z

...re/src/main/java/com/github/dozermapper/springboot/autoconfigure/DozerAutoConfiguration.java

+public class DozerAutoConfiguration {
+
+    @Autowired
+    private DozerConfigurationProperties configurationProperties;


I would recommend to replace field injection with the constructor injection (actually, this is a recommendation by Spring Team).

orange-buffalo · 2017-10-31T15:08:46Z

dozer-spring-boot-auto-configuration/pom.xml

+            <groupId>com.github.dozermapper</groupId>
+            <artifactId>dozer-spring</artifactId>
+            <version>${project.version}</version>
+            <optional>true</optional>


Agree that providing dependencies is not a responsibility of autoconfiguration. But it is of the starter, the same documentation states:

The starter module that provides a dependency to the autoconfigure module as well as the library and any additional dependencies that are typically useful. In a nutshell, adding the starter should be enough to start using that library.
...
Its only purpose is to provide the necessary dependencies to work with the library; see it as an opinionated view of what is required to get started.

Thus I would recommend to include autoconfiguration and dozer-spring as compile-time dependencies to the starter module.

And then I would also switch dependencies in autoconfiguration module from optional to provided, because optional has different semantic in Maven, basically meaning "dependencies required by optional feature", while autoconfiguration will not work at all unless Dozer library is provided.

orange-buffalo · 2017-10-31T15:22:02Z

.../main/java/com/github/dozermapper/springboot/autoconfigure/DozerConfigurationProperties.java

+@ConfigurationProperties(prefix = "dozer")
+public class DozerConfigurationProperties {
+
+    private Resource[] mappingFiles = new Resource[] {};


It would be nice to have a javadoc on this field (and maybe event skipping javadocs for getter and setter, to avoid unnecessary duplication).

The reason to have a javadoc on the field is to generate a better meta-data file to help IDE to provide richer support for these properties, compare currently generated file

"properties": [ { "sourceType": "com.github.dozermapper.springboot.autoconfigure.DozerConfigurationProperties", "defaultValue": [], "name": "dozer.mapping-files", "type": "org.springframework.core.io.Resource[]" } ]

to the one if field has a javadoc:

"properties": [ { "sourceType": "com.github.dozermapper.springboot.autoconfigure.DozerConfigurationProperties", "defaultValue": [], "name": "dozer.mapping-files", "description": "Mapping files configuration.\n For example <code>classpath:*.dozer.xml<\/code>.", "type": "org.springframework.core.io.Resource[]" } ]

I agree. But to avoid unnecessary duplication maven-checkstyle-plugin configuration should be updated. Are you fine with that?

In general I do not see any problems in tuning tools to suite the project needs. In this particular case I would recommend to use @SuppressWarnings("checkstyle:xxx") in code + enabling SuppressWarningsFilter in CheckStyle config.

orange-buffalo · 2017-10-31T15:29:36Z

dozer-spring-boot-auto-configuration/dozer-spring-boot-autoconfigure/pom.xml

+        </dependency>
+        <dependency>
+            <groupId>org.springframework.boot</groupId>
+            <artifactId>spring-boot-autoconfigure</artifactId>


Shouldn't all Spring dependencies be provided?

Made all of them optional.

orange-buffalo · 2017-10-31T15:36:03Z

dozer-spring-boot-auto-configuration/pom.xml

+            <dependency>
+                <groupId>org.springframework.boot</groupId>
+                <artifactId>spring-boot-dependencies</artifactId>
+                <version>1.5.8.RELEASE</version>


Probably it makes sense to extract this version to a property in bom-dependencies/pom.xml

It did not not work. It will work only in case if bom-dependencies is a direct parent of dozer-spring-boot-auto-configuration.

Sorry for misleading comment, it's not only about the property, but also about the dependency itself.
I.e. if we define in bom-dependencies/pom.xml:

<dependencyManagement> ... <dependency> <groupId>org.springframework.boot</groupId> <artifactId>spring-boot-dependencies</artifactId> <version>${spring-boot.version}</version> <type>pom</type> <scope>import</scope> </dependency>

And then in dozer-spring-boot-auto-configuration/pom.xml define just:

<dependency> <groupId>org.springframework.boot</groupId> <artifactId>spring-boot-dependencies</artifactId> </dependency>

It should work fine.

Yes. That make sense.

vadeg · 2017-11-02T17:31:23Z

@orange-buffalo For somehow reason I can not reply to comment.

And then I would also switch dependencies in autoconfiguration module from optional to provided, because optional has different semantic in Maven, basically meaning "dependencies required by optional feature", while autoconfiguration will not work at all unless Dozer library is provided.

I think optional dependency is semantically more correct rather than provided in this case. provided means provided by some container or etc. but not the project. servlet-api library looks as perfect as a provided dependency because usually it provided by container.

Foe example spring-boot-autoconfigure module marks all dependencies as optional.

So would leave as optional. The same problem with dozer-spring project which has provided dependency from dozer-core what looks confusing.

autoconfigure module will not work in both cases while dozer library will be defined (not provided) by parent project.

orange-buffalo · 2017-11-03T06:40:21Z

@vadeg frankly speaking, I believe neither provided nor optional is a good fit for autoconfigurer because none of their definitions covers the use case. That's why I like Gradle's compileOnly, which to me is semantically neutral and opens wide range of applications.

vadeg · 2017-11-04T11:08:19Z

@orange-buffalo unfortunately, we are not using Gradle (yet). In this case I would stick with optional. We can add requirement like "Change optional/provided to compileOnly" in #328

garethahealy · 2017-11-05T09:40:37Z

...re/src/main/java/com/github/dozermapper/springboot/autoconfigure/DozerAutoConfiguration.java

+        DozerBeanMapperFactoryBean factoryBean = new DozerBeanMapperFactoryBean();
+        factoryBean.setMappingFiles(configurationProperties.getMappingFiles());
+        factoryBean.setApplicationContext(applicationContext);
+        return factoryBean;


Just wondering if the Mapper instance should be returned, instead of the spring factory. Or does spring wire together the Mapper by its "magic" :)

FactoryBean should be returned. @Bean annotation has the same semantics as <bean> xml tag. The following steps are executed by spring itself.

BTW I have removed ApplicationContext from DozerAutoConfiguration because DozerBeanMapperFactoryBean implements ApplicationContextAware. That means it is not necessary to call factoryBean.setApplicationContext() because it will be injected by spring itself into DozerBeanMapperFactoryBean instance.

…tion.

vadeg · 2017-11-09T09:47:27Z

@garethahealy @orange-buffalo any updates on this?

orange-buffalo

@vadeg from my side the only comment left is about the spring-boot version definition - I would really appreciate if it was in dozer-bom-dependencies. The rest looks good to me.

garethahealy · 2017-11-18T17:49:18Z

dozer-spring-boot-auto-configuration/dozer-spring-boot-autoconfigure/pom.xml

+    <artifactId>dozer-spring-boot-autoconfigure</artifactId>
+    <packaging>jar</packaging>
+
+    <name>Spring Boot AutoConfiguration :: Dozer AutoConfiguration</name>


Can you update name to match other modules, i.e.: Dozer :: Module :: Sub Module

https://github.com/DozerMapper/dozer/blob/master/spring/pom.xml#L33

garethahealy · 2017-11-18T17:52:39Z

dozer-spring-boot-auto-configuration/dozer-spring-boot-starter/pom.xml

+    </parent>
+    <modelVersion>4.0.0</modelVersion>
+    <artifactId>dozer-spring-boot-starter</artifactId>
+    <name>Spring Boot AutoConfiguration :: Dozer Starter</name>


Same as other comment. Match other names

garethahealy · 2017-11-18T17:53:50Z

Just added a minor comment. But think everything looks fine.

Any further comments @orange-buffalo ?

orange-buffalo · 2017-11-18T17:55:04Z

Nothing from my side, looks good to me.

Rename modules.

garethahealy requested changes Oct 29, 2017

View reviewed changes

garethahealy added needs-code-review new-feature labels Oct 30, 2017

Some review comments fixes.

94a10fc

orange-buffalo reviewed Oct 31, 2017

View reviewed changes

vadeg mentioned this pull request Nov 1, 2017

module structure #527

Closed

Fix review comments.

ba10036

garethahealy reviewed Nov 5, 2017

View reviewed changes

Removed useless ApplicationContext dependency from DozerAutoConfigura…

ff62844

…tion.

vadeg force-pushed the feature/spring-boot-support branch from b9a6ecf to ff62844 Compare November 5, 2017 11:27

orange-buffalo reviewed Nov 9, 2017

View reviewed changes

garethahealy reviewed Nov 18, 2017

View reviewed changes

Move "spring-boot-dependencies" into dozer "bom-dependencies".

f015d77

Rename modules.

vadeg force-pushed the feature/spring-boot-support branch from d4a215c to f015d77 Compare November 19, 2017 13:36

garethahealy approved these changes Nov 19, 2017

View reviewed changes

garethahealy merged commit d7ecf30 into DozerMapper:master Nov 19, 2017

vadeg deleted the feature/spring-boot-support branch November 21, 2017 20:13


		<name>Spring Boot AutoConfiguration :: Dozer AutoConfiguration</name>

		<properties>

PR #382: Added spring-boot autoconfiguration support. #526

PR #382: Added spring-boot autoconfiguration support. #526

Conversation

vadeg commented Oct 28, 2017 • edited Loading

Issue link

Purpose

Approach

Open Questions and Pre-Merge TODOs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garethahealy commented Oct 29, 2017

orange-buffalo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orange-buffalo Oct 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vadeg commented Nov 2, 2017

orange-buffalo commented Nov 3, 2017

vadeg commented Nov 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vadeg commented Nov 9, 2017

orange-buffalo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garethahealy commented Nov 18, 2017

orange-buffalo commented Nov 18, 2017

vadeg commented Oct 28, 2017 •

edited

Loading

orange-buffalo Oct 31, 2017 •

edited

Loading