Skip to content

Custom converter not used for Iterable type from 2.4.1 #1343

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

Closed
loolzaaa opened this issue Sep 29, 2022 · 4 comments
Closed

Custom converter not used for Iterable type from 2.4.1 #1343

loolzaaa opened this issue Sep 29, 2022 · 4 comments
Assignees
Labels
type: regression A regression from a previous release

Comments

@loolzaaa
Copy link

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.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Sep 29, 2022
@loolzaaa
Copy link
Author

loolzaaa commented Sep 29, 2022

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 {
        // ..............
    }
}

hariohmprasath added a commit to hariohmprasath/spring-data-relational that referenced this issue Oct 3, 2022
Fix to check for both iterable and hasGenerics before proceeding
hariohmprasath added a commit to hariohmprasath/spring-data-relational that referenced this issue Oct 3, 2022
Fix to check for both iterable and hasGenerics before proceeding
@hariohmprasath
Copy link
Contributor

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

loolzaaa added a commit to PNPPK/sso-authentication-server that referenced this issue Oct 4, 2022
@MikeN123
Copy link

MikeN123 commented Oct 4, 2022

#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.

@christophstrobl christophstrobl added type: regression A regression from a previous release and removed status: waiting-for-triage An issue we've not yet triaged labels Oct 4, 2022
@loolzaaa
Copy link
Author

loolzaaa commented Oct 5, 2022

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;
}

@schauder schauder linked a pull request Oct 6, 2022 that will close this issue
4 tasks
schauder added a commit that referenced this issue Oct 11, 2022
Simplify test.

See #1343
schauder added a commit that referenced this issue Oct 11, 2022
Contents of Iterables that aren't collections will not be converted individually.

Closes #1343
schauder added a commit that referenced this issue Oct 11, 2022
Contents of Iterables that aren't collections will not be converted individually.

Closes #1343
schauder added a commit that referenced this issue Feb 16, 2023
Simplify test.

See #1343
schauder added a commit that referenced this issue Feb 16, 2023
Contents of Iterables that aren't collections will not be converted individually.

Closes #1343
mp911de pushed a commit that referenced this issue Mar 15, 2023
Simplify test.

Original pull request: #1356
See #1343
mp911de pushed a commit that referenced this issue Mar 15, 2023
Contents of Iterables that aren't collections will not be converted individually.

Closes #1343
Original pull request: #1356
mp911de added a commit that referenced this issue Mar 15, 2023
Simplify TypeInformation creation from a MethodParameter.

Original pull request: #1356
See #1343
mp911de pushed a commit that referenced this issue Mar 15, 2023
Simplify test.

Original pull request: #1356
See #1343
mp911de added a commit that referenced this issue Mar 15, 2023
Simplify TypeInformation creation from a MethodParameter.

Original pull request: #1356
See #1343
@mp911de mp911de added this to the 3.0.4 (2022.0.4) milestone Mar 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: regression A regression from a previous release
Projects
None yet
7 participants