Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

pjjonesnz
Copy link

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();
}

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Apr 3, 2024
@pjjonesnz
Copy link
Author

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 schauder self-assigned this Apr 3, 2024
@schauder schauder self-requested a review April 3, 2024 13:36
@schauder
Copy link
Contributor

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
Copy link
Author

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 mp911de mentioned this pull request Jun 10, 2024
Closed
@mp911de mp911de assigned mp911de and unassigned schauder Jun 10, 2024
@mp911de mp911de added type: task A general task and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 10, 2024
@mp911de
Copy link
Member

mp911de commented Jun 10, 2024

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

@mp911de mp911de closed this in cf26d7e Jun 10, 2024
mp911de pushed a commit that referenced this pull request Jun 10, 2024
@mp911de
Add unit tests to verify DTO projection with AggregationReference.
b76bd37 
Closes #1759
mp911de pushed a commit that referenced this pull request Jun 10, 2024
@mp911de
Add unit tests to verify DTO projection with AggregationReference.
4d4d9b5 
Closes #1759
@mp911de mp911de added this to the 3.2.7 (2023.1.7) milestone Jun 10, 2024
@mp911de
Copy link
Member

mp911de commented Jun 10, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@schauder schauder Awaiting requested review from schauder

Assignees

@mp911de mp911de

Labels
type: task A general task
Projects
None yet
Milestone
3.2.7 (2023.1.7)
Development

Successfully merging this pull request may close these issues.
4 participants
@pjjonesnz @schauder @mp911de @spring-projects-issues