spring-projects / spring-data-relational

Spring Data Relational. Home of Spring Data JDBC and Spring Data R2DBC.
https://spring.io/projects/spring-data-jdbc
Apache License 2.0
740 stars 342 forks source link

Support AggregateReference in a Dto that contains only a single no-args constructor #1759

Closed pjjonesnz closed 1 month ago

pjjonesnz commented 3 months ago

This is a simple fix to populate AggregateReference columns for Class based Dtos which only have a No-args constructor. At present all other columns are correctly injected, it is just the AggregateReferences that are not transferred to the Dto.

I've added tests to my PR to prove the problem and the fix. In the following example, the 'productType' column for the ProductDTO will be null using the current main branch, while all other columns will be correctly populated.

Hope this is helpful. Please let me know your thoughts. Thanks!

Model

@NoArgsConstructor
@Getter
@Setter
@Table
public class Product {
    @Id
    private Long id;
    private String name;
    private AggregateReference<ProductType,Long> productType;
}

DTO

@NoArgsConstructor
@Getter
@Setter
public class ProductDTO {
        private Long id;
        private String name;
        private AggregateReference<ProductType,Long> productType;
}

My Repository

public interface ProductRepository extends ListCrudRepository<Product, Long> {
    List<Product> findAll();
    List<ProductDTO> findAllBy();
}
pjjonesnz commented 3 months ago

PS. I should mention for clarity, that the JdbcDtoInstantiatingConverter is a duplicate of the DtoInstantiatingConverter from Spring Relational with lines 92 - 102 added to support AggregateReference columns.

schauder commented 3 months ago

Thanks for looking into this. I don't think though that we can accept a fix that duplicates code like this. And that shouldn't be necessary, since in general the conversion seems to work fine, so there shouldn't be special handling necessary for Spring Data JDBC.

Instead there seems to be some inconsistency in commons, namely the meaning of (persistent)property is not clear.

BasicPersistentEntity has three relevant fields: List<P> properties List<P> persistentPropertiesCache Set<Association<P>> associations

Note that the first two both contain PersistentProperty elements, despite their name difference. What really differentiates the two is the inclusion of associations in the first (and third) but not in the second.

The two variants of doWithProperties both use persistentPropertiesCache and thus ignore associations. But iterator() is based on properties.

Instantiators start with the arguments of the constructor which the map to properties. This resolution seems to happen against properties thus including associations.

I therefore think we should do one of the following:

Either make doWithProperties work on properties instead. Or basically make your addition to the DtoInstantiatingConverter and possibly other cases where we have a similar issue.

I can't tell which would be the right thing to do. Therefore pinging @mp911de

At the very least we should spend some time to explain the three properties of BasicPersistentEntity in its documentation a little better.

pjjonesnz commented 3 months ago

Thanks so much for your feedback.

It definitely had me scratching my head when I was trying to work out the difference in purpose between the three properties.

I have looked at the DtoInstantiatingConverter in spring-data-commons, making the following change, and it does indeed solve the issue. It does still pass all the tests in spring-data-commons and spring-data-relational, although I see that it is used in most spring-data projects so it may have other consequences.

// targetEntity.doWithProperties((SimplePropertyHandler) property -> {

targetEntity.iterator().forEachRemaining(property -> {

Thanks for all your work on this project - it is inspiring.

mp911de commented 1 month ago

This has been fixed in spring-projects/spring-data-commons#3104. From this pull request we intend to keep the test.

mp911de commented 1 month ago

Thank you for your contribution. That's merged and backported now.