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

ResultProcessor should back off for objects of the target type #2347

Closed
ada-waffles opened this issue Mar 4, 2021 · 2 comments
Closed

ResultProcessor should back off for objects of the target type #2347

ada-waffles opened this issue Mar 4, 2021 · 2 comments
Labels
type: enhancement A general enhancement

Comments

@ada-waffles
Copy link

When using a class as a DTO for a @Query-based projection, it appears the DTO conversion is performed twice. This causes issues with DTOs that use/require custom converters, as the second conversion seems to ignore them.

I've uploaded a reproduction project here.

When run, it produces the following output:

Initializing...
Checking inserted entity...
Getting automatic DTO projection...
AutoDto constructor called
AutoDto constructor called
Getting custom DTO projection...
CustomDtoReadConverter called
WARNING: An illegal reflective access operation has occurred
//SNIP: Illegal reflection warning output
2021-03-04 14:56:36.757 ERROR 20369 --- [tor-tcp-epoll-1] r.n.channel.ChannelOperationsHandler     : [id: 0xeb237d5e, L:/0:0:0:0:0:0:0:1%0:55338 - R:localhost/0:0:0:0:0:0:0:1:5432] Error was received while reading the incoming data. The connection will be closed.

java.lang.InstantiationError: com.example.demo.CustomDto
	at com.example.demo.CustomDto_Instantiator_ddeq8t.newInstance(Unknown Source) ~[main/:na]
	at org.springframework.data.mapping.model.ClassGeneratingEntityInstantiator$EntityInstantiatorAdapter.createInstance(ClassGeneratingEntityInstantiator.java:238) ~[spring-data-commons-2.4.5.jar:2.4.5]
	at org.springframework.data.mapping.model.ClassGeneratingEntityInstantiator.createInstance(ClassGeneratingEntityInstantiator.java:87) ~[spring-data-commons-2.4.5.jar:2.4.5]
	at org.springframework.data.relational.repository.query.DtoInstantiatingConverter.convert(DtoInstantiatingConverter.java:82) ~[spring-data-relational-2.1.5.jar:2.1.5]
	at org.springframework.data.repository.query.ResultProcessor$ChainingConverter.convert(ResultProcessor.java:236) ~[spring-data-commons-2.4.5.jar:2.4.5]
	at org.springframework.data.repository.query.ResultProcessor$ChainingConverter.lambda$and$0(ResultProcessor.java:222) ~[spring-data-commons-2.4.5.jar:2.4.5]
	at org.springframework.data.repository.query.ResultProcessor$ChainingConverter.convert(ResultProcessor.java:236) ~[spring-data-commons-2.4.5.jar:2.4.5]
//SNIP: Mostly Project Reactor stack frames

It appears that the DTO class is directly read from the result set, as expected, but the resulting object is then sent through the codepath used for entity-based projections, which does not short-circuit on the value already being the desired type. If the type can be instantiated via reflection, it simply converts the object into another instance of the same type (thus the two constructor calls in the output). If the type cannot be properly instantiated that way (for example, if it's an abstract class), it fails.

I'm not sure where would be best to do it, but I'm guessing the fix for this is as simple as short-circuiting this process when the desired type has been read directly.

These few lines in Data Commons jump out to me:

if (source instanceof Stream && method.isStreamQuery()) {
return (T) ((Stream<Object>) source).map(t -> type.isInstance(t) ? t : converter.convert(t));
}
if (ReactiveWrapperConverters.supports(source.getClass())) {
return (T) ReactiveWrapperConverters.map(source, converter::convert);
}

The Stream-based codepath is guarding the converter on a typecheck, but the Reactive codepath is not. There might be a better place to fix this, but that jumped out to me.

Thanks so much for all the hard work on R2DBC!

@mp911de
Copy link
Member

mp911de commented Mar 5, 2021

Related to #2306.

It appears that the DTO class is directly read from the result set

That's exactly what's happening here. While we don't create partially materialized entity objects, we lose column customizations and potential converters that are applied on the entity level before creating a projection on top of the entity.

The ResultProcessor however should not attempt to convert the DTO again which seems a bug.

@mp911de mp911de changed the title @Query projection DTOs are converted twice ResultProcessor should back off for objects of the target type Apr 6, 2021
@mp911de mp911de transferred this issue from spring-projects/spring-data-r2dbc Apr 6, 2021
@mp911de mp911de added the type: enhancement A general enhancement label Apr 6, 2021
@mp911de mp911de added this to the 2.4.8 (2020.0.8) milestone Apr 6, 2021
@mp911de
Copy link
Member

mp911de commented Apr 6, 2021

I moved this ticket to Spring Data Commons. The issue arises from the fact that the declared return type is abstract so Spring Data cannot instantiate that type. However, we should back off if the result object is already an instance of the target type.

mp911de added a commit that referenced this issue Apr 6, 2021
…e target type.

We now do not attempt to convert the object if it is an instance of the declared target type.

Closes #2347.
@mp911de mp911de closed this as completed in d27ac79 Apr 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

2 participants