Skip to content

3.0.0-M2 Improved @ConstructorBinding Detection #166

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

Closed
Tracked by #163
fabapp2 opened this issue Jun 13, 2022 · 6 comments
Closed
Tracked by #163

3.0.0-M2 Improved @ConstructorBinding Detection #166

fabapp2 opened this issue Jun 13, 2022 · 6 comments
Assignees

Comments

@fabapp2
Copy link
Contributor

fabapp2 commented Jun 13, 2022

What needs to be done

Implement the required migration

https://github.com/spring-projects/spring-boot/wiki/Spring-Boot-3.0.0-M2-Release-Notes#improved-constructorbinding-detection

Example code (to discuss)

Ideal translation

TestCase 1

Given: the Spring Boot 2 has Configuration property file annotated with @ConstructorBinding and has only single constructor
Expected: the @ConstructorBinding annotation is removed for SpringBoot 3.0.X (it is not necessary anymore)

Acceptance criterias

Report
Given: class annotated with ...
When: recipe is applied..
Then: ....

Recipe
Given: class annotated with ...
When: recipe is applied..
Then: ....

Initial code:

@ConfigurationProperties(prefix = "mail")
@ConstructorBinding
public class ConfigProperties {
    private String hostName;
    private int port;
    private String from;

    public ConfigProperties(String hostName, int port, String from) {
        this.hostName = hostName;
        this.port = port;
        this.from = from;
    }


    public String getHostName() {
        return hostName;
    }

    public int getPort() {
        return port;
    }

    public String getFrom() {
        return from;
    }
}

Transformed code for SpringBoot 3.0.X:

@ConfigurationProperties(prefix = "mail")
public class ConfigProperties {
    private String hostName;
    private int port;
    private String from;

    public ConfigProperties(String hostName, int port, String from) {
        this.hostName = hostName;
        this.port = port;
        this.from = from;
    }


    public String getHostName() {
        return hostName;
    }

    public int getPort() {
        return port;
    }

    public String getFrom() {
        return from;
    }
}

case 2

@ashakirin
Copy link
Contributor

ashakirin commented Jun 21, 2022

TestCase 1:
Given: the Spring Boot 2 has Configuration property file annotated with @ConstructorBinding and has only single constructor
Expected: the @ConstructorBinding annotation is removed for SpringBoot 3.0.X (it is not necessary anymore)

Initial code:

@ConfigurationProperties(prefix = "mail")
@ConstructorBinding
public class ConfigProperties {
    private String hostName;
    private int port;
    private String from;

    public ConfigProperties(String hostName, int port, String from) {
        this.hostName = hostName;
        this.port = port;
        this.from = from;
    }


    public String getHostName() {
        return hostName;
    }

    public int getPort() {
        return port;
    }

    public String getFrom() {
        return from;
    }
}

Transformed code for SpringBoot 3.0.X:

@ConfigurationProperties(prefix = "mail")
public class ConfigProperties {
    private String hostName;
    private int port;
    private String from;

    public ConfigProperties(String hostName, int port, String from) {
        this.hostName = hostName;
        this.port = port;
        this.from = from;
    }


    public String getHostName() {
        return hostName;
    }

    public int getPort() {
        return port;
    }

    public String getFrom() {
        return from;
    }
}

@sanagaraj-pivotal
Copy link
Contributor

sanagaraj-pivotal commented Jun 22, 2022

Acceptance criteria for constructor binding

Following is an overview of different test cases for constructor binding:
Screenshot 2022-06-22 at 11 43 25

Following is the list of all tests cases for Constructor Binding

@ConstructorBinding on Class level and only one constructor

Input:

package com.example.config.demo;

import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.context.properties.ConfigurationProperties;
import org.springframework.boot.context.properties.ConstructorBinding;

@ConfigurationProperties(prefix = "mail")
@ConstructorBinding
public class ConfigProperties {
    private String hostName;
    private int port;
    private String from;

    public ConfigProperties(String hostName, int port, String from) {
        this.hostName = hostName;
        this.port = port;
        this.from = from;
    }

    public String getHostName() {
        return hostName;
    }

    public int getPort() {
        return port;
    }

    public String getFrom() {
        return from;
    }
}

output

package com.example.config.demo;

import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.context.properties.ConfigurationProperties;

@ConfigurationProperties(prefix = "mail")
public class ConfigProperties {
    private String hostName;
    private int port;
    private String from;

    public ConfigProperties(String hostName, int port, String from) {
        this.hostName = hostName;
        this.port = port;
        this.from = from;
    }

    public String getHostName() {
        return hostName;
    }

    public int getPort() {
        return port;
    }

    public String getFrom() {
        return from;
    }
}

@ConstructorBinding on Constructor level and only one constructor

input:

package com.example.config.demo;

import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.context.properties.ConfigurationProperties;
import org.springframework.boot.context.properties.ConstructorBinding;

@ConfigurationProperties(prefix = "mail")
public class ConfigProperties {
    private String hostName;
    private int port;
    private String from;

    @ConstructorBinding
    public ConfigProperties(String hostName, int port, String from) {
        this.hostName = hostName;
        this.port = port;
        this.from = from;
    }

    public String getHostName() {
        return hostName;
    }

    public int getPort() {
        return port;
    }

    public String getFrom() {
        return from;
    }
}

output:

package com.example.config.demo;

import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.context.properties.ConfigurationProperties;

@ConfigurationProperties(prefix = "mail")
public class ConfigProperties {
    private String hostName;
    private int port;
    private String from;

    public ConfigProperties(String hostName, int port, String from) {
        this.hostName = hostName;
        this.port = port;
        this.from = from;
    }

    public String getHostName() {
        return hostName;
    }

    public int getPort() {
        return port;
    }

    public String getFrom() {
        return from;
    }
}

@ConstructorBinding on Constructor level and multiple constructors

input:

package com.example.config.demo;

import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.context.properties.ConfigurationProperties;
import org.springframework.boot.context.properties.ConstructorBinding;

@ConfigurationProperties(prefix = "mail")
public class ConfigProperties {
    private String hostName;
    private int port;
    private String from;

    @ConstructorBinding
    public ConfigProperties(String hostName, int port, String from) {
        this.hostName = hostName;
        this.port = port;
        this.from = from;
    }
    
    public ConfigProperties(String hello) {
    }

    public String getHostName() {
        return hostName;
    }

    public int getPort() {
        return port;
    }

    public String getFrom() {
        return from;
    }
}

output: Do nothing.

@ConstructorBinding on Class level and multiple constructors

input:

package com.example.config.demo;

import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.context.properties.ConfigurationProperties;
import org.springframework.boot.context.properties.ConstructorBinding;

@ConfigurationProperties(prefix = "mail")
@ConstructorBinding
public class ConfigProperties {
    private String hostName;
    private int port;
    private String from;
    
    public ConfigProperties(String hostName, int port, String from) {
        this.hostName = hostName;
        this.port = port;
        this.from = from;
    }
    
    public ConfigProperties(String hello) {
    }

    public String getHostName() {
        return hostName;
    }

    public int getPort() {
        return port;
    }

    public String getFrom() {
        return from;
    }
}

output:

package com.example.config.demo;

import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.context.properties.ConfigurationProperties;
import org.springframework.boot.context.properties.ConstructorBinding;

@ConfigurationProperties(prefix = "mail")
/*
 * TODO:
 * You need to remove ConstructorBinding on class level and move it to appropriate
 * constructor.
 * 
 * For more info visit:
 * https://bit.ly/3n5Uflt
 * */
@ConstructorBinding
public class ConfigProperties {
    private String hostName;
    private int port;
    private String from;
    
    public ConfigProperties(String hostName, int port, String from) {
        this.hostName = hostName;
        this.port = port;
        this.from = from;
    }
    
    public ConfigProperties(String hello) {
    }

    public String getHostName() {
        return hostName;
    }

    public int getPort() {
        return port;
    }

    public String getFrom() {
        return from;
    }
}

@ConstructorBinding on Class level but no constructors

package com.example.config.demo;

import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.context.properties.ConfigurationProperties;
import org.springframework.boot.context.properties.ConstructorBinding;

@ConfigurationProperties(prefix = "mail")
@ConstructorBinding
public class ConfigProperties {
    private String hostName;
    private int port;
    private String from;
    
    public String getHostName() {
        return hostName;
    }

    public int getPort() {
        return port;
    }

    public String getFrom() {
        return from;
    }
}

output:

package com.example.config.demo;

import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.context.properties.ConfigurationProperties;

@ConfigurationProperties(prefix = "mail")
public class ConfigProperties {
    private String hostName;
    private int port;
    private String from;
    
    public String getHostName() {
        return hostName;
    }

    public int getPort() {
        return port;
    }

    public String getFrom() {
        return from;
    }
}

@BoykoAlex
Copy link
Collaborator

BoykoAlex commented Jun 22, 2022

I looked at this recipe already and contributed to rewrite-spring openrewrite/rewrite-spring@5a37e8e

I was looking at annotations over constructors as well but at the end decided to follow exactly what release note state without doing any further work:
@ConstructorBinding is no longer needed at the type level on @ConfigurationProperties classes and should be removed. When a class or record has multiple constructors, it may still be used on a constructor to indicate which one should be used for property binding.

Just removed the type annotation from the type annotated with @ConfigurationProperties. If @ConstructorBinding is present over type with multiple constructors - remove the annotation, likely the code wasn't working properly anyway. Also, single constructor annotated i was removing it at first but the release notes don't state that we should remove anything from constructors hence i left it in. However, if you feel we should make it better for the constructors as well - I don't mind

Since I've contributed it to rewrite-spring already perhaps further corrections (if needed) we can make in rewrite-spring directly?

(As a side note - I can reanimate the code i had to handle constructors so feel free to assign to me if you like)

@ashakirin
Copy link
Contributor

ashakirin commented Jun 22, 2022

@ConstructorBinding on Class level but no constructors

@sanagaraj-pivotal: In this case we have to remove annotation as well

@ashakirin
Copy link
Contributor

ashakirin commented Jun 22, 2022

I looked at this recipe already and contributed to rewrite-spring openrewrite/rewrite-spring@5a37e8e

I was looking at annotations over constructors as well but at the end decided to follow exactly what release note state without doing any further work: @ConstructorBinding is no longer needed at the type level on @ConfigurationProperties classes and should be removed. When a class or record has multiple constructors, it may still be used on a constructor to indicate which one should be used for property binding.

Just removed the type annotation from the type annotated with @ConfigurationProperties. If @ConstructorBinding is present over type with multiple constructors - remove the annotation, likely the code wasn't working properly anyway. Also, single constructor annotated i was removing it at first but the release notes don't state that we should remove anything from constructors hence i left it in. However, if you feel we should make it better for the constructors as well - I don't mind

Since I've contributed it to rewrite-spring already perhaps further corrections (if needed) we can make in rewrite-spring directly?

(As a side note - I can reanimate the code i had to handle constructors so feel free to assign to me if you like)

@BoykoAlex:

  • for the case @ConstructorBinding on class level and multiple constructors we decided to leave annotation and generate TODO for the user. Compilation error will be easy to find as runtime one
  • in case of single annotated constructor release notes explicitly says that it is not really necessary:
Improved @ConstructorBinding Detection
When using constructor bound @ConfigurationProperties the @ConstructorBinding annotation is no longer required if the class has a single parameterized constructor. If you have more than one constructor, you’ll still need to use @ConstructorBinding to tell Spring Boot which one to use.

For most users, this updated logic will allow for simpler @ConfigurationProperties classes. If, however, you have a @ConfigurationProperties and you want to inject beans into the constructor rather than binding it, you’ll now need to add an @Autowired annotation.

yes, we can update rewrite-spring recipe directly or add this functionality into our spring-boot-update-30 module and synchronise all changes at the end, as we agreed

@fabapp2
Copy link
Contributor Author

fabapp2 commented Jul 12, 2022

Provided to OpenRewrite: openrewrite/rewrite-spring#204

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

No branches or pull requests

4 participants