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

DTO joins support issues #2761

Draft
wants to merge 10 commits into
base: 4.10.x
Choose a base branch
from
Draft

DTO joins support issues #2761

wants to merge 10 commits into from

Conversation

radovanradic
Copy link
Contributor

@dstepanov I started looking at DTO association support as there are few reported issues. Seems like support for ManyToOne is not added.

@@ -172,4 +173,6 @@ protected Book newBook(Author author, String title, int pages) {
abstract List<Book> findByTitleInAndTotalPagesGreaterThan(List<String> titles, int totalPages);

abstract Long countByTitleInAndTotalPagesGreaterThan(List<String> titles, int totalPages);

abstract Optional<BookDtoWithAuthorDto> queryByTitleContains(String title);
Copy link
Contributor Author

@radovanradic radovanradic Jan 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having AuthorDto as BookDto field should work but compiler gives this error

error: Unable to implement Repository method: PostgresBookRepository.queryByTitleContains(String title). Property [author] of type [io.micronaut.data.tck.entities.AuthorDTO] is not compatible with equivalent property of type [io.micronaut.data.tck.entities.Author] declared in entity: io.micronaut.data.tck.entities.Book

So we might need to check nested types if compatible. I will commit potential fix for this, but still SELECT does not use this joined entity and mapper also does not consider it when reading data.

if (!TypeUtils.areTypesCompatible(dtoPropertyType, pp.getType())) {
throw new MatchFailedException("Property [" + propertyName + "] of type [" + dtoPropertyType.getName() + "] is not compatible with equivalent property of type [" + pp.getType().getName() + "] declared in entity: " + entity.getName());
boolean compatibleTypes = TypeUtils.areTypesCompatible(dtoPropertyType, pp.getType());
if (!compatibleTypes) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be the fix for initial issue. If field types are not simple field or same classes, then check if entity matches with DTO.
After this, new error is when reading data

io.micronaut.data.exceptions.DataAccessException: Error reading object for name [author] from result set: Column "author" not found [42122-224]

because mapper tries to read it into the field vs into joined object. Also, should method specify join type for the DTO or automatically assume join is requested?

@CLAassistant
Copy link

CLAassistant commented Feb 7, 2024

CLA assistant check
All committers have signed the CLA.

@radovanradic
Copy link
Contributor Author

@dstepanov I have committed changes in this PR that should fix DTO join issue.
Turns out that initial implementation for DTO join support assumed it will join persistent entities on DTOs while users are trying to join another DTO that is representation of other entity that is joined on the main persistent entity. To support that, we need to change logic how we are checking type compatibility like done here. If that's not what we should support (and support only DTO->PersistentEntity join/loading) then this PR is not needed. So, in first tests for DTO support if in AuthorDtoWithBooks we replace Book with BookDto then tests would be failing.
This change in code fixes the issue it seems, however for Hibernate things look a bit more complicated to support because of the way Hibernate loads data back into the objects.

// DTO might not have ID mapped, and in this case to maintain relation
// we set random UUID as id to be able to read and make relation
if (isDto) {
return UUID.randomUUID();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure this will work?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, there is a test that covers that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the other hand, don't know if could break some user case, all tests we have are passing.

Copy link

sonarcloud bot commented Feb 14, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
57.1% Coverage on New Code (required ≥ 70%)
2 New Bugs (required ≤ 0)

See analysis details on SonarCloud

idea Catch issues before they fail your Quality Gate with our IDE extension SonarLint SonarLint

@radovanradic radovanradic changed the base branch from 4.7.x to 4.10.x September 12, 2024 10:50
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

Successfully merging this pull request may close these issues.

3 participants