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

DtoQuery may match the wrong constructor #3062

Closed
spinachomes opened this issue May 19, 2023 · 6 comments · Fixed by #3075
Closed

DtoQuery may match the wrong constructor #3062

spinachomes opened this issue May 19, 2023 · 6 comments · Fixed by #3075
Assignees
Labels
Milestone

Comments

@spinachomes
Copy link
Contributor

spinachomes commented May 19, 2023

when dto have some diffrent constructors,only match constructor with constructor's args length can't find the right constructor

  public DtoQueryPlan match(DtoMappingRequest request) {
    DtoColumn[] cols = request.getColumnMeta();
    int colLen = cols.length;
    // get the wrong constructor
    DtoMetaConstructor constructor = constructorMap.get(colLen);
    if (constructor != null) {
      return new DtoQueryPlanConstructor(request, constructor);
    }
    if (maxArgConstructor != null && colLen > maxArgConstructor.getArgCount()) {
      // maxArgConst + setters
      return matchMaxArgPlusSetters(request);
    }
    if (defaultConstructor != null) {
      return matchSetters(request);
    }
    String msg = "Unable to map the resultSet columns " + Arrays.toString(cols)
      + " to the bean type ["+dtoType+"] as the number of columns in the resultSet is less than the constructor"
      + " (and that there is no default constructor) ?";
    throw new IllegalStateException(msg);
  }

demo dto

@Data
@NoArgsConstructor
public class NameStatisticsResult {

    /**
     * 名称
     */
    private String name;

    /**
     * 部门名称,过渡字段
     */
    @JsonIgnore
    private String deptName;

    /**
     * 数量
     */
    private Integer value;

    /**
     * 详情
     */
    @JsonInclude(JsonInclude.Include.NON_EMPTY)
    private List<NameStatisticsResult> details;

    public NameStatisticsResult(String name, Integer value) {
        this.name = name;
        this.value = value;
    }

    public NameStatisticsResult(String name, Integer value, List<NameStatisticsResult> details) {
        this.name = name;
        this.value = value;
        this.details = details;
    }

    public NameStatisticsResult(String name, String deptName, Integer value) {
        this.name = name;
        this.deptName = deptName;
        this.value = value;
    }

}
@rbygrave
Copy link
Member

Well yes in this example there is are two constructors that both take 3 parameters.

Ebean is expecting us to remove one of those constructors so that there is only 1 constructor that takes 3 arguments.

I wonder if what you mean here is that Ebean should automatically ignore the constructor that takes the List<NameStatisticsResult> details ? ... but no, Ebean is simply going on the number of arguments.

So there is no "bug" here, we just can't have 2 or more constructors that have the same number of arguments.

@spinachomes
Copy link
Contributor Author

spinachomes commented May 19, 2023

yes,maybe think about if find one more constructor,return DtoQueryPlanConSetter or DtoQueryPlanConPlus?

@rbygrave
Copy link
Member

if find one more constructor,return DtoQueryPlanConSetter or DtoQueryPlanConPlus?

Do you mean, in this case Ebean finds 2 constructors that take 3 arguments ... so it should then ignore both constructors [because it will not be able to resolve which one to use], and hence the only constructors it should use is the NameStatisticsResult(String name, Integer value) one and the default constructor [which we can't see but is added by Lombok @NoArgsConstructor]?

rob-bygrave added a commit that referenced this issue May 25, 2023
When 2 or more Dto constructors clash by argument count
then we need to ignore those constructors.
@rob-bygrave rob-bygrave linked a pull request May 25, 2023 that will close this issue
rob-bygrave added a commit that referenced this issue May 25, 2023
#3062 - DtoQuery may match the wrong constructor
@rob-bygrave
Copy link
Contributor

it should then ignore both constructors [because it will not be able to resolve which one to use],

Yes, this is effectively the fix for this.

@rob-bygrave rob-bygrave self-assigned this May 25, 2023
@rob-bygrave rob-bygrave added this to the 13.18.0 milestone May 25, 2023
@jnehlmeier
Copy link

@rob-bygrave

I wonder if what you mean here is that Ebean should automatically ignore the constructor that takes the List details ? ... but no, Ebean is simply going on the number of arguments.

Maybe the documentation needs an update to be more precise? Currently it says

"Firstly we look for a constructor with the same number of arguments as columns in the resultSet. If we have such a constructor we use it for mapping (assuming the correct types)"

A reader could very well understand that if Ebean finds two or more constructors of the same length, Ebean would then choose the one with the correct order of parameter types.

That is kind of what I would expect too. If the result set contains String, String, int then search for a ctor with 3 (or less parameters if setters exist) in the order String, String, int. Given that many people (I assume) use default build tool settings class files also almost always have debug symbols included including parameter names. So these could be used most of the time as well. I assume that if Ebean decides to look for setters as well these need to be done by name anyways, not just by type, right?

@rob-bygrave
Copy link
Contributor

If the result set contains String, String, int then search for a ctor with 3 (or less parameters if setters exist) in the order String, String, int.

We have many more Java types than database types plus for the case of number types and Date/Time types there are multiple matching possibilities so no DtoQuery constructor matching is just simple and based on argument count.

decides to look for setters as well these need to be done by name anyways, not just by type, right?

Setters are matched by name yes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants