spring-data-relational icon indicating copy to clipboard operation
spring-data-relational copied to clipboard

Custom converter not used for Iterable type from 2.4.1

Open loolzaaa opened this issue 3 years ago • 4 comments

I have a custom converter JsonNode <-> PGobject (PGobject represent JSON type for PostgreSQL):

@Configuration
@RequiredArgsConstructor
public class CustomJdbcConfiguration extends AbstractJdbcConfiguration {

    private final ObjectMapper objectMapper;

    @Override
    protected List<?> userConverters() {
        return Arrays.asList(
                new JsonNodeToJsonConverter(),
                new JsonToJsonNodeConverter()
        );
    }

    @ReadingConverter
    class JsonToJsonNodeConverter implements Converter<PGobject, JsonNode> {
        @Override
        public JsonNode convert(PGobject json) {
            try {
                return objectMapper.readTree(json.getValue());
            } catch (JsonProcessingException | NullPointerException e) {
                e.printStackTrace();
            }
            return null;
        }
    }

    @WritingConverter
    class JsonNodeToJsonConverter implements Converter<JsonNode, PGobject> {
        @Override
        public PGobject convert(JsonNode jsonNode) {
            PGobject json = new PGobject();
            json.setType("jsonb");
            try {
                json.setValue(objectMapper.writeValueAsString(jsonNode));
            } catch (SQLException | JsonProcessingException e) {
                e.printStackTrace();
            }
            return json;
        }
    }
}

Also, i have a simple repo @Query method:

@Repository
public interface UserRepository extends CrudRepository<User, Long> {
    @Modifying
    @Query("UPDATE users SET config = :config::jsonb WHERE login = :login")
    void updateConfigByLogin(@Param("config") JsonNode config, @Param("login") String login);
}

Before Spring Data JDBC this converter used normally: old version of code

But, from 2.4.1 version there is a check for Iterable in StringBasedJdbcQuery:

JdbcValue jdbcValue;
if (value instanceof Iterable) {
    // ..........
    Class<?> elementType = resolvableType.getGeneric(0).resolve();
    Assert.notNull(elementType, "@Query Iterable parameter generic type could not be resolved!");
    // ..........
} else {
    jdbcValue = converter.writeJdbcValue(value, type, 
             JdbcUtil.targetSqlTypeFor(JdbcColumnTypes.INSTANCE.resolvePrimitiveType(type)));
}

JsonNode implements Iterable interface, but Class<?> elementType is null, so assert is failed and custom converter never applied. If manually run else block, conversion goes normally.

loolzaaa avatar Sep 29 '22 16:09 loolzaaa

In fact, the implementation of an Iterator does not mean the existence of generics. Moreover, the class generic type may differ from the iterator generic type (iteration over the elements of the inner class, for example), but we can't check this.

In all these cases, the else block must be executed. The object is written with an attempt to find a custom converter:

jdbcValue = converter.writeJdbcValue(value, type, 
             JdbcUtil.targetSqlTypeFor(JdbcColumnTypes.INSTANCE.resolvePrimitiveType(type)));

As a solution, i can propose additional check method:

private boolean isGenericIterable(ResolvableType resolvableType, Object value) {
    return resolvableType.hasGenerics() && value instanceof Iterable;
}

So the code will look like this:

private void convertAndAddParameter(MapSqlParameterSource parameters, Parameter p, Object value) {
    // ..............
    ResolvableType resolvableType = parameter.getResolvableType();
    // ..............
    JdbcValue jdbcValue;
    if (isGenericIterable(resolvableType, value)) {
        // ..............
    } else {
        // ..............
    }
}

loolzaaa avatar Sep 29 '22 19:09 loolzaaa

Hi @loolzaaa, Your suggestion makes sense for me, since its a simple change I have submitted a PR. But I will wait to hear from maintainers of this repo before i sent in an updated PR with the unit tests. Thanks

hariohmprasath avatar Oct 03 '22 06:10 hariohmprasath

#1323 has issues with the same change, so I think this change would not be everything - it needs to have a generic, but the generic also needs to be a primitive type, I think.

MikeN123 avatar Oct 04 '22 12:10 MikeN123

Actually, BasicJdbcConverter#writeJdbcValue has the following checks for an already converted value:

  • null or not array convertedValue == null || !convertedValue.getClass().isArray()
  • array NOT of byte/Byte class componentType != byte.class && componentType != Byte.class
  • array of byte/Byte class (for binary data)

If we exclude the last check that the data is binary, then the final representation (see next) in sql query of the JdbcValue will depend only on whether the converted value is an array or not. And if the generic is not an array, then in the presence of custom converters, not only primitive types, but also custom simple types will be valid.

When Spring create prepared statement, it substitute named parameters. If the parameter itself is iterable, then each of its elements will be wrapped in brackets VALUES ((?, ?), (?, ?)) if it is an array.

So, i think iterable condition above must satisfy the following conditions:

  • value instanceof Iterable
  • resolvableType.hasGenerics()
  • !resolvableType.getGeneric(0).resolve().isArray()

As a solution, i can propose updated additional check method:

private boolean isNonArrayGenericIterable(ResolvableType resolvableType, Object value) {
    boolean typeHasGenerics = resolvableType.hasGenerics();
    if (typeHasGenerics) {
        Class<?> elementType = resolvableType.getGeneric(0).resolve();
        return value instanceof Iterable && !elementType.isArray();
    }
    return false;
}

loolzaaa avatar Oct 05 '22 11:10 loolzaaa