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

Bindings do not work with POJOs with no setters but a defined @ConstructorProperties constructor #18344

Closed
ddcruver opened this issue Sep 24, 2019 · 8 comments
Labels
status: invalid An issue that we don't feel is valid

Comments

@ddcruver
Copy link

ddcruver commented Sep 24, 2019

I have been trying to use an effectively immutable class in a @ConfigurationProperties class but the binder is failing with "No setter found for property" message. #8762 would lead me to believe this is working but it is not.

Description:

Failed to bind properties under 'com.example.constructor-properties-app.properties' to com.example.constructorpropertiesnotworking.ConfigurationProvidingPojo:

    Property: com.example.constructor-properties-app.properties.x
    Value: X
    Origin: class path resource [application.yaml]:4:8
    Reason: No setter found for property: x

Action:

Update your application's configuration

It is effectively immutable because while the fields are not final but can only be set externally through constructor. See ConfigurationProvidingPojo in attached project.

constructor-properties-not-working-example.zip
not-working.log

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Sep 24, 2019
@snicoll
Copy link
Member

snicoll commented Sep 24, 2019

Thanks for the sample but that zip file seems to be corrupted. Can you please check?

@snicoll snicoll added the status: waiting-for-feedback We need additional information before we can continue label Sep 24, 2019
@ddcruver
Copy link
Author

ddcruver commented Sep 24, 2019

That's weird I download it and it seemed to open just fine on my machine. I tried to upload a 7zip formatted version but the format is not supported by GitHub.

Maybe the problem is the compression method used, for some reason my 7zip was using Bzip2 for zip files. I will switch it to Deflate.

constructor-properties-not-working-example.zip

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Sep 24, 2019
@snicoll
Copy link
Member

snicoll commented Sep 24, 2019

Thanks it's working fine now.

This is working as advertized. There are two constructors in your class and immutable binding works with a single constructor match.

@snicoll snicoll closed this as completed Sep 24, 2019
@snicoll snicoll added status: invalid An issue that we don't feel is valid and removed status: feedback-provided Feedback has been provided status: waiting-for-triage An issue we've not yet triaged labels Sep 24, 2019
@ddcruver
Copy link
Author

ddcruver commented Sep 24, 2019

I would like to reuse generated classes in our @ConfigurationProperties but this looks like it will be troublesome. I was hoping to be able to use the @ConstructorProperties to tell Spring Boot which constructor it should us.

Our Use Case:
Right now we require the no-args constructor to be generated because it is required for serialization and our generated inner class builders.

Additionally In the future I wanted our generated classes to have an additional constructor that only has the required properties of an object. This would not be used by serialization/deserialization (Jackson) but for programmatic construction of objects.

Feature Request:

I suppose this is now a feature request but would it be possibility to allow for a single @ConstructorProperties constructor to be used by the Binder.

Additionally allow for the constructor with the most parameters AND @ConstructorProperties to be the one that the binder uses. This later being an edge case that maybe too specific to my own proposed use case.

I can create a separate issues if required.

@snicoll snicoll added the for: team-attention An issue we'd like other members of the team to review label Sep 24, 2019
@ddcruver
Copy link
Author

Thank you for taking the time to look over this request.

I believe the example project I submitted might be using a POJO that might be too simple to illustrate our use case so I will provide a better example of our generated objects below.

package com.example.phones;

import java.beans.ConstructorProperties;
import java.io.Serializable;
import javax.validation.constraints.DecimalMin;
import javax.validation.constraints.NotNull;
import com.fasterxml.jackson.annotation.JsonInclude;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.annotation.JsonPropertyOrder;
import com.fasterxml.jackson.annotation.JsonTypeInfo;


/**
 * 
 * 
 */
@JsonTypeInfo(use = JsonTypeInfo.Id.CLASS, include = JsonTypeInfo.As.PROPERTY, property = "@class")
@JsonInclude(JsonInclude.Include.USE_DEFAULTS)
@JsonPropertyOrder({
    "width",
    "height"
})
public class PixelResolution implements Serializable
{

    /**
     * 
     * (Required)
     * 
     */
    @JsonProperty("width")
    @DecimalMin("0")
    @NotNull
    private Integer width;
    /**
     * 
     * (Required)
     * 
     */
    @JsonProperty("height")
    @DecimalMin("0")
    @NotNull
    private Integer height;
    private final static long serialVersionUID = -1754877909526366602L;

    /**
     * No args constructor for use in serialization
     * 
     */
    public PixelResolution() {
    }

    /**
     * 
     * @param width
     * @param height
     */
    @ConstructorProperties({
        "width",
        "height"
    })
    public PixelResolution(Integer width, Integer height) {
        super();
        this.width = width;
        this.height = height;
    }

    /**
     * 
     * (Required)
     * 
     */
    @JsonProperty("width")
    public Integer getWidth() {
        return width;
    }

    /**
     * 
     * (Required)
     * 
     */
    @JsonProperty("height")
    public Integer getHeight() {
        return height;
    }

    @Override
    public String toString() {
        StringBuilder sb = new StringBuilder();
        sb.append(PixelResolution.class.getName()).append('@').append(Integer.toHexString(System.identityHashCode(this))).append('[');
        sb.append("width");
        sb.append('=');
        sb.append(((this.width == null)?"<null>":this.width));
        sb.append(',');
        sb.append("height");
        sb.append('=');
        sb.append(((this.height == null)?"<null>":this.height));
        sb.append(',');
        if (sb.charAt((sb.length()- 1)) == ',') {
            sb.setCharAt((sb.length()- 1), ']');
        } else {
            sb.append(']');
        }
        return sb.toString();
    }

    @Override
    public int hashCode() {
        int result = 1;
        result = ((result* 31)+((this.width == null)? 0 :this.width.hashCode()));
        result = ((result* 31)+((this.height == null)? 0 :this.height.hashCode()));
        return result;
    }

    @Override
    public boolean equals(Object other) {
        if (other == this) {
            return true;
        }
        if ((other instanceof PixelResolution) == false) {
            return false;
        }
        PixelResolution rhs = ((PixelResolution) other);
        return (((this.width == rhs.width)||((this.width!= null)&&this.width.equals(rhs.width)))&&((this.height == rhs.height)||((this.height!= null)&&this.height.equals(rhs.height))));
    }

    public static class PixelResolutionBuilder<T extends PixelResolution >{

        protected T instance;

        @SuppressWarnings("unchecked")
        public PixelResolutionBuilder() {
            // Skip initialization when called from subclass
            if (this.getClass().equals(PixelResolution.PixelResolutionBuilder.class)) {
                this.instance = ((T) new PixelResolution());
            }
        }

        @SuppressWarnings("unchecked")
        public PixelResolutionBuilder(Integer width, Integer height) {
            // Skip initialization when called from subclass
            if (this.getClass().equals(PixelResolution.PixelResolutionBuilder.class)) {
                this.instance = ((T) new PixelResolution(width, height));
            }
        }

        public T build() {
            T result;
            result = this.instance;
            this.instance = null;
            return result;
        }

        public PixelResolution.PixelResolutionBuilder withWidth(Integer width) {
            ((PixelResolution) this.instance).width = width;
            return this;
        }

        public PixelResolution.PixelResolutionBuilder withHeight(Integer height) {
            ((PixelResolution) this.instance).height = height;
            return this;
        }

    }

}

@wilkinsona
Copy link
Member

That looks quite complex for a configuration properties class. Have you considered binding the configuration properties into one type and using another for Java and JSON serialisation?

@ddcruver
Copy link
Author

ddcruver commented Sep 30, 2019

Ironically that class is meant to be a simple POJO. The builder arguably makes it look much more complex then it really is. If you updated the JSON schema (which is what generates it) then it would update that POJO but then the configuration properties class would be outdated. That is why I would like to use the generated POJO as a configuration class.

Additionally the generated class would include the Java Validation annotations that the binder uses as well. If I change the schema simply to add validation I would have to also remember to update the configuration properties instance of the class as well. Since this is a multi-developer project it is even more likely that things will be forgotten.

@philwebb
Copy link
Member

philwebb commented Oct 2, 2019

#18469 will add a @ConstructorBinding annotation which you can use to indicate which constructor to use.

@philwebb philwebb removed the for: team-attention An issue we'd like other members of the team to review label Oct 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: invalid An issue that we don't feel is valid
Projects
None yet
Development

No branches or pull requests

5 participants