Fix R2dbcEntityTemplate.getRowsFetchSpec(…) to use the correct Converter for result type conversion

[oktupol]

This change fixes a bug introduced with version 3.2.2 of spring-data-r2dbc.

Scenario:

An entity ExampleEntity exists, along with a ReactiveCrudRepository ExampleRepository.

ExampleEntity has following fields:

public class ExampleEntity {
  @Id private UUID id;
  private String someField;
}

ExampleRepository has following method: {

public interface ExampleRepository extends ReactiveCrudRepository<ExampleEntity, UUID> {
  @Query("SELECT COUNT(*) FROM EXAMPLE_ENTITY e WHERE e.SOME_FIELD = 'test'")
  Mono<Long> countBySomeFieldEqualsTest();
}

Additionally, an explicit converter exists for ExampleEntity:

@ReadingConverter
private static class RowToExampleEntityConverter implements Converter<Row, ExampleEntity> {
  @Override
  public ExampleEntity convert(Row source) {
    // constructing an ExampleEntity here...
  }
}

Up until Version 3.2.1, invoking ExampleRepository.countBySomeFieldEqualsTest worked.
Upon invocation, at some point, the method changed in this Pull Request getRowsFetchSpec checks which, if any, converter shall be used for converting the row fetched from the database.

The implementation up until 3.2.1 was:

	// entityType is ExampleEntity.class, resultType is Long.class
	public <T> RowsFetchSpec<T> getRowsFetchSpec(DatabaseClient.GenericExecuteSpec executeSpec, Class<?> entityType,
			Class<T> resultType) {

                // In the case of countBySomeFieldEqualsTest, resultType is Long, therefore simpleType == true
		boolean simpleType = getConverter().isSimpleType(resultType);

		BiFunction<Row, RowMetadata, T> rowMapper;

		if (simpleType) {
			rowMapper = dataAccessStrategy.getRowMapper(resultType);
		} else {
			// ... irrelevant in our case
		}

		// avoid top-level null values if the read type is a simple one (e.g. SELECT MAX(age) via Integer.class)
		if (simpleType) {
			return new UnwrapOptionalFetchSpecAdapter<>(
					executeSpec.map((row, metadata) -> Optional.ofNullable(rowMapper.apply(row, metadata))));
		}

		return executeSpec.map(rowMapper);
	}

however, the new implementation breaks things:

	// entityType is ExampleEntity.class, resultType is Long.class
	public <T> RowsFetchSpec<T> getRowsFetchSpec(DatabaseClient.GenericExecuteSpec executeSpec, Class<?> entityType,
			Class<T> resultType) {
                // again, simpleType == true
		boolean simpleType = getConverter().isSimpleType(resultType);

		BiFunction<Row, RowMetadata, T> rowMapper;

		// The next if-block executes, as the existence of RowToExampleEntityConverter fulfils the condition
		if (converter instanceof AbstractRelationalConverter relationalConverter
				&& relationalConverter.getConversions().hasCustomReadTarget(Row.class, entityType)) {

			ConversionService conversionService = relationalConverter.getConversionService();
			// rowMapper is now a Lambda that converts to ExampleEntity, not Long
			rowMapper = (row, rowMetadata) -> (T) conversionService.convert(row, entityType);
		} else if (simpleType) {
			// This branch is no longer executed
			rowMapper = dataAccessStrategy.getRowMapper(resultType);
		} else {
			// ... irrelevant in our case
		}

		// avoid top-level null values if the read type is a simple one (e.g. SELECT MAX(age) via Integer.class)
		if (simpleType) {
			return new UnwrapOptionalFetchSpecAdapter<>(
					executeSpec.map((row, metadata) -> Optional.ofNullable(rowMapper.apply(row, metadata))));
		}

		return executeSpec.map(rowMapper);
	}

Becuase of this, the result of the "SELECT COUNT(*)..." query above is fed into RowToExampleEntityConverter.convert, which expects a different input format.

By switching around these two if-blocks, the implementation works again in the shown scenario, which is why I would like to propose this change.

• You have read the Spring Data contribution guidelines.
• 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]

Good catch. I'd argue that the converter lookup must check for resultType instead of entityType. Have you tried switching the target type and running your tests?

It would be also great to have a test along with your changes.

[oktupol]

You're right, Switching these two variables also solves this issue.

I added a test case, but I'm not sure whether my testing approach is ideal.

[mp911de]

Thanks a lot. The changes look decent and the fix is much more efficient.

[mp911de]

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