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
748 stars 344 forks source link

Custom class is registered as simple type only if there is writing converter #1194

Open Infeligo opened 2 years ago

Infeligo commented 2 years ago

I am using Spring Data JDBC repository to query data for report (hence, readonly). In one place I needed to convert from JSONB to a DTO. I registered a customer converter marked as @ReadingConverter for that purpose, but for some reason it was never called. Debugging the code, I noticed that my DTO is considered as an entity, although it should have been registered as a simple type. Debugging more I found out that only the source type of a @WritingConverter is registered as a simple type. Adding a dummy writing converter fixed the issue for me. However, this seems unexpected and the fix adds redundant code.

My code:

@Data
public class ReportDTO {

  private MyData myData;

}

@Configuration
public class JdbcConfig extends AbstractJdbcConfiguration

  private final ObjectMapper objectMapper = new ObjectMapper();

  @Bean
  @NonNull
  @Override
  public JdbcCustomConversions jdbcCustomConversions() {
    return new JdbcCustomConversions(
        List.of(
            new JsonbToMyData(objectMapper),
        )
    );
  }

  @ReadingConverter
  @RequiredArgsConstructor
  static class JsonbToMyData implements Converter<PGobject, MyData> {

    private final ObjectMapper objectMapper;

    @Override
    public MyData convert(PGobject source) {
      try {
        return objectMapper.readValue(source.getValue(), MyData.class);
      } catch (JsonProcessingException e) {
        throw new RuntimeException(e);
      }
    }

  }
schauder commented 2 years ago

The behaviour you describe is a bug and should get fixed. The type should NOT be considered a simple type, but custom conversions should be taken into consideration before treating it as an entity.

loolzaaa commented 1 year ago

The type should NOT be considered a simple type

But all source types of writing converters always add to custom simple types. Moreover, all custom types in BasicRelationalConverter are treated as simple:

if (converterRegistration.isWriting()) {

    writingPairs.add(pair);
    customSimpleTypes.add(pair.getSourceType());    // <<<<<<<<<<<<<<<<<<<<

    if (logger.isWarnEnabled() && !converterRegistration.isSimpleTargetType()) {
        logger.warn(String.format(WRITE_CONVERTER_NOT_SIMPLE, pair.getSourceType(), pair.getTargetType()));
    }
}

Custom simple types is Set<Class<?>>.

Why not just add this type when registering a converter for reading? The simple types set will not contain repetitions.

schauder commented 2 months ago

The code you pointed to resides in spring-data-commons and read converters where explicitly excluded. See https://github.com/spring-projects/spring-data-mongodb/issues/1203

So we can't simply undo that change.

mp911de commented 2 months ago

There are three aspects:

  1. PGobject should be considered a simple type as per the dialect.
  2. Reading converters should never control whether the source type is a simple one as the type can some from any source and what we want to achieve is allowing to transform the type without considering it a store-native one because there is no way to write the type. Only if we figure that a type can be written, we may consider it a simple one.
  3. Like @schauder said, that's a bug on our side. I think we're using isEntity too liberal without considering CustomConversions.
schauder commented 2 months ago

PGobject should be considered a simple type as per the dialect.

Related: https://github.com/spring-projects/spring-data-relational/issues/1778 but really, the problem here is with MyData which is considered an entity and therefore causes a join to be created with a non-existing table.

schauder commented 2 months ago

We had an internal discussion and these are our findings:

Unfortunately these changes are complex and we probably won't be able to tackle them in the near future. For the time being, please use the workaround of adding a writing converter as well.