spring-projects / spring-data-relational

Spring Data Relational. Home of Spring Data JDBC and Spring Data R2DBC.
https://spring.io/projects/spring-data-jdbc
Apache License 2.0
766 stars 346 forks source link

PersistentEntity not created when top-level converter [DATAJDBC-295] #520

Open spring-projects-issues opened 5 years ago

spring-projects-issues commented 5 years ago

Greg Potter opened DATAJDBC-295 and commented

When registering a customer Converter that uses Object or some other top-level class (perhaps in the form of a ConditionalConverter), Spring Data JDBC fails to think a PersistentEntity is required for the top-level object and creating the repository fails.

This is actually mentioned in the javadoc for AbstractMappingContext.shouldCreatePersistentEntityFor:

By default this will reject all types considered simple and non-supported Kotlin classes, but it might be necessary to tweak that in case you have registered custom converters for top level types (which renders them to be considered simple) but still need meta-information about them.

However, resolving this does not seem to be trivial since it's not easy to extend that method (simpleTypeHolder is private). I'm thinking we actually might want to try to convert it first (or at least go through the "match" process for a ConditionalConverter) before assuming that all converter sources are not entities


Issue Links:

spring-projects-issues commented 5 years ago

Jens Schauder commented

Both issues touch on the same underlying issue: converting an object doesn't just change the value but also change the way we need to persist it.

spring-projects-issues commented 5 years ago

Greg Potter commented

What is the long-term plan here? I think I was trying to do something similar to your attached card. I wanted to allow a custom converter to look at annotations on the field to decide if it was going to be relevant or not. I noticed that some of the GenericConverters just declare themselves as new ConvertiblePair(Object.class, Object.class) but having a @Type annotation would be easier for me anyways.

I'm happy to look into fixing this but I'd be curious what your plans are

spring-projects-issues commented 5 years ago

Jens Schauder commented

Well, right now I'm trying to find out what a good plan would be.

Right now, custom conversions in Spring Data JDBC turn the converted type into a simple type, with the assumption that the database can handle the converted type. With this mindset, it doesn't really make sense to convert a top-level type. So the question arises: What is the specific goal of registering a conversion for a top-level type?

spring-projects-issues commented 5 years ago

Greg Potter commented

I have a field that I'm interested in mapping to JSON instead of just using a separate table. I was hoping to annotate the field and use a custom conditional converter to do the magic. I've gotten around it right now by simply using a marker interface and registering that as the converter type

spring-projects-issues commented 5 years ago

Jens Schauder commented

And what is the reason for not using the actual type of the field as the source type for the converter?

spring-projects-issues commented 5 years ago

Greg Potter commented

I guess I can but I was trying to build something more generic that I could use across multiple columns throughout my project

spring-projects-issues commented 5 years ago

Jens Schauder commented

How would that generic solution look like? Could you sketch out an Entity or two and a converter?

I don't think to convert an entity first and then storing it still as an entity doesn't make much sense. So we probably won't support that.

But we still might be able to make the logic for identifying simple types smarter, but for that I'd need a more specific example

spring-projects-issues commented 5 years ago

Greg Potter commented

I was planning on doing something like this:

public class StringToJsonConverter implements ConditionalConverter, ConverterFactory<String, Object> {

  private final ObjectMapper objectMapper;

  public StringToJsonConverter(ObjectMapper objectMapper) {     this.objectMapper = objectMapper;     }

  @Override   public <T> Converter<String, T> getConverter(Class<T> targetType) {     return source -> {       try {         return objectMapper.readValue(source, targetType);       } catch (IOException e) {         throw new RuntimeException(e);              }     };     }

  @Override   public boolean matches(TypeDescriptor sourceType, TypeDescriptor targetType) {     return targetType.hasAnnotation(JsonMappedType.class);     } }

And then annotating my field with @JsonMappedType. That way I could designate some fields as being converted to/from Json and then falling back on the native functionality when the annotation is absent.

 

spring-projects-issues commented 5 years ago

Jens Schauder commented

Ok, that makes a lot of sense. So the problem is that the logic for determining if a type is a simple type doesn't correctly considers ConditionalConverters

That should be fixable