RFC: Redundant code in getPropertyValue

[ArtDu]

cartridge-springdata/src/main/java/org/springframework/data/tarantool/core/convert/MappingTarantoolReadConverter.java

Lines 198 to 231 in a7c438e

	public <R> R getPropertyValue(TarantoolPersistentProperty property) {
	if (source == null) {
	return null;
	}
	TypeInformation<?> propType = property.getTypeInformation();
	Class<?> propClass = propType.getType();
	Optional<Class<?>> customTargetClass = conversions.getCustomWriteTarget(propClass);
	String fieldName = property.getFieldName();
	if (source instanceof TarantoolTuple) {
	Object value;
	if (propType.isCollectionLike()) {
	value = ((TarantoolTuple) source).getList(fieldName);
	} else if (propType.isMap()) {
	value = ((TarantoolTuple) source).getMap(fieldName);
	} else if (customTargetClass.isPresent() &&
	conversions.hasCustomReadTarget(customTargetClass.get(), propClass) &&
	((TarantoolTuple) source).canGetObject(fieldName, customTargetClass.get())) {
	value = ((TarantoolTuple) source).getObject(fieldName, customTargetClass.get()).orElse(null);
	} else if (((TarantoolTuple) source).canGetObject(fieldName, Map.class)) {
	value = convertCustomType((Map<String, Object>) ((TarantoolTuple) source).getMap(fieldName), propType);
	} else if (((TarantoolTuple) source).canGetObject(fieldName, propClass)) {
	value = ((TarantoolTuple) source).getObject(fieldName, propClass).orElse(null);
	} else
	value = ((TarantoolTuple) source).getObject(fieldName).orElse(null);
	return readValue(value, propType);
	} else if (source instanceof Map) {
	return readValue(((Map<String, Object>) source).get(fieldName), propType);
	} else {
	throw new MappingException("Cannot read properties from a source of type " + source.getClass());
	}
	}

We don't need this branch condition:

cartridge-springdata/src/main/java/org/springframework/data/tarantool/core/convert/MappingTarantoolReadConverter.java

Lines 219 to 220 in a7c438e

	} else if (((TarantoolTuple) source).canGetObject(fieldName, Map.class)) {
	value = convertCustomType((Map<String, Object>) ((TarantoolTuple) source).getMap(fieldName), propType);

Now I'll try to explain why:

• This is redundant code because it completely overlaps functionality below by this code
  

  cartridge-springdata/src/main/java/org/springframework/data/tarantool/core/convert/MappingTarantoolReadConverter.java

  Lines 223 to 225 in a7c438e

  	} else
  	value = ((TarantoolTuple) source).getObject(fieldName).orElse(null);
  	return readValue(value, propType);

  • getMap vs getObject
    in fact, the difference between these methods is that one of them has a targetClass, while the other does not have this knowledge. And the key converter search call for these two functions looks like this:

    For getMap:

    public <V extends Value, O> O fromValue(V v, Class<O> 𝐭𝐚𝐫𝐠𝐞𝐭𝐂𝐥𝐚𝐬𝐬) {
        Function<String, Optional<ValueConverter<V, O>>> getter =
                typeName -> valueConverters.getOrDefault(typeName, Collections.emptyList()).stream()
                        .map(c -> (ValueConverter<V, O>) c)
                        .filter(c -> c.canConvertValue(v))
                        .𝐟𝐢𝐥𝐭𝐞𝐫(𝐜 -> 𝐜𝐡𝐞𝐜𝐤𝐂𝐨𝐧𝐯𝐞𝐫𝐭𝐞𝐫𝐁𝐲𝐓𝐚𝐫𝐠𝐞𝐭𝐓𝐲𝐩𝐞(𝐜, 𝐭𝐚𝐫𝐠𝐞𝐭𝐂𝐥𝐚𝐬𝐬))
                        .findFirst();
        return fromValue(v, getter);
    }

    And for getObject:

    public <V extends Value, O> O fromValue(V v) {
        Function<String, Optional<ValueConverter<V, O>>> getter =
                typeName -> valueConverters.getOrDefault(typeName, Collections.emptyList()).stream()
                        .map(c -> (ValueConverter<V, O>) c)
                        .filter(c -> c.canConvertValue(v))
                        .findFirst();
        return fromValue(v, getter);
    }

    We have checkConverterByTargetType, but what does that mean for converting. We filter only those converters that convert the value to Map.class. Such a converter by default is one DefaultMapValueConverter and it is also the only converter in valueConverters that converts exactly those objects whose class is inherited from MapValue.

    public class DefaultMapValueConverter implements ValueConverter<MapValue, Map<?, ?>> {

    Those only one converter will be found for class objects inherited from MapValue and we also check this only converter if it is really converted to Map.class. Therefore, this filtering is not needed here and we can use getObject without targetClass.

  • convertCustomType overlaps in readValue
    After calling getMap and after calling getObject we have in HashMap which passed in all cases to convertCustomType.
    In case without canGetObject(fieldName, Map.class) condition branch we passed this Map in readValue -> convertIfNeeded -> convertCustomType
    

    cartridge-springdata/src/main/java/org/springframework/data/tarantool/core/convert/MappingTarantoolReadConverter.java

    Lines 233 to 250 in a7c438e

    	private <R> R readValue(@Nullable Object source, TypeInformation<?> propertyType) {
    	Assert.notNull(propertyType, "Target type must not be null!");
    	if (source == null) {
    	return null;
    	}
    	Class<?> targetClass = propertyType.getType();
    	if (conversions.hasCustomReadTarget(source.getClass(), targetClass) ||
    	conversions.isSimpleType(targetClass) && conversionService.canConvert(source.getClass(), targetClass)) {
    	return (R) conversionService.convert(source, targetClass);
    	} else if (propertyType.isCollectionLike()) {
    	return convertCollection(asCollection(source), propertyType);
    	} else if (propertyType.isMap()) {
    	return (R) convertMap((Map<String, Object>) source, propertyType);
    	}
    	return (R) convertIfNeeded(source, propertyType);
    	}

    
    

    cartridge-springdata/src/main/java/org/springframework/data/tarantool/core/convert/MappingTarantoolReadConverter.java

    Lines 327 to 336 in a7c438e

    	private Object convertIfNeeded(Object value, TypeInformation<?> propertyType) {
    	Class<?> targetClass = propertyType.getType();
    	if (Enum.class.isAssignableFrom(targetClass)) {
    	return Enum.valueOf((Class<Enum>) targetClass, value.toString());
    	} else if (value instanceof Map && !propertyType.isMap()) {
    	return convertCustomType((Map<String, Object>) value, propertyType);
    	} else {
    	return value;
    	}
    	}

Please advise me if I made a mistake somewhere and it is impossible to remove it.