DATACMNS-1762 - Support JDK Optional as return type in projections

[wimdeblauwe]

This commit adds 3 unit tests for using a JDK Optional as return
type in a projection. 2 of them are working with a minimal change
to ProjectingMethodInterceptor although I wonder if the change is
ok like that, or if the Optional creation should be delegated to
the conversionService.

1 unit test is failing: if the Optional is typed to another projection,
the Optional does not contain the projection, leading to a ClassCastException.

Could somebody give a tip where to best fix that Optional-with-projection fail?

• You have read the Spring Data contribution guidelines.
• There is a ticket in the bug tracker for the project in our JIRA.
• You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
• You submit test cases (unit or integration tests) that back your changes.
• You added yourself as author in the headers of the classes you touched. Amend the date range in the Apache license header if needed. For new types, add the license header (copy from another file and set the current year only).

[mp911de]

Thanks, Wim. We have similar functionality in the repository abstraction with QueryExecutionConverters.

We would need to move all functionality but ALLOWED_PAGEABLE_TYPES and isSingleValue into a new type in (e.g. WrapperConverters) to org.springframework.data.util to avoid package cycles. We also should preserve backward compatibility by changing public method calls in QueryExecutionConverters to invoke their corresponding new method in WrapperConverters. The same goes for NullableWrapper, where we could deprecate the one in data.repository.util and introduce a replacement in data.util of which the deprecated variant is a subclass.

Let me know if that makes sense to you.

[wimdeblauwe]

Not really unfortunately.

The last sentence I understood I think:

1. Copy org.springframework.data.repository.util.NullableWrapper to a new class org.springframework.data.util.NullableWrapper
2. Have org.springframework.data.repository.util.NullableWrapper extend org.springframework.data.util.NullableWrapper and mark it deprecated.
3. Have org.springframework.data.repository.util.QueryExecutionConverters use the new class and not the deprecated class.

But the first part is a bit over my head I am afraid.

This is my first look at the Spring Data code ever. I was already glad I could write some unit tests that show what I would like to have supported :-)

[mp911de]

That’s exactly how it was meant to be. Feel free to reach out if you need further guidance, happy to help.

There are a few methods in QueryExecutionConverters that report type multiplicity and paging-capabilities (I.e.whether a wrapper can represent a List). My suggestion there is to leave those methods untouched for now. We can sort out these during the merge.

[wimdeblauwe]

I have added a new commit on the branch for the NullableWrapper refactor, but I don't understand how that will help or how I should continue now.

[wimdeblauwe]

Re-reading your initial comment and I think I get it now:

1. Copy org.springframework.data.repository.util.QueryExecutionConverters into a new class org.springframework.data.util.WrapperConverters
2. Remove the isSingleValue(Class<?> type) method from WrapperConverters
3. Remove the getAllowedPageableTypes() method and the ALLOWED_PAGEABLE_TYPES from WrapperConverters
4. Have the methods on QueryExecutionConverters delegate to the corresponding methods on WrapperConverters

Is that correct?

If so, how would I then use WrapperConverters in ProjectingMethodInterceptor ?

[mp911de]

Yes, that sounds about right.

ProjectingMethodInterceptor needs to check if the return type of a method is a supported wrapper (WrapperConverters.supports(…)). If so, then the value needs to wrapped in NullableWrapper and converted into the target type (see

spring-data-commons/src/main/java/org/springframework/data/repository/core/support/QueryExecutionResultHandler.java

Lines 129 to 146 in eed7658

	if (QueryExecutionConverters.supports(expectedReturnType)) {
	// For a wrapper type, try nested resolution first
	result = postProcessInvocationResult(result, nestingLevel + 1, descriptor);
	if (conversionRequired(WRAPPER_TYPE, returnTypeDescriptor)) {
	return conversionService.convert(new NullableWrapper(result), returnTypeDescriptor);
	}
	if (result != null) {
	TypeDescriptor source = TypeDescriptor.valueOf(result.getClass());
	if (conversionRequired(source, returnTypeDescriptor)) {
	return conversionService.convert(result, returnTypeDescriptor);
	}
	}
	}

for the Repository implementation).

Since ProjectingMethodInterceptor is also responsible for light-weight type conversion (i.e. the backing object contains int but the projection method returns long or the like), it's required to determine the actual return type (the component type) of a wrapper so that the actual value gets converted to its target type first.

[wimdeblauwe]

I had another look, but it seems to difficult for me to fully understand what needs to happen, sorry for that.

[mp911de]

No worries. Let me have a look tomorrow at the PR to see how we can continue here.

[mp911de]

I took a look at the current state. It seems to be easier to start from scratch as this pull request contains a good start but it requires a bit more work to bring it into a state where it should be. What do you think about closing this PR and following DATACMNS-1762 to review the changes afterwards? Being unfamiliar with code isn't something to worry about.

[wimdeblauwe]

Ok for me. I think the unit tests I have written might be useful, so feel free to use them in your own PR if you think the same.

[mp911de]

I created a new pull request #459, specifically this commit shows the integration with ProjectingMethodInterceptor. Besides, the PR contains a few changes that we would have done in the course of merging this PR.