Skip to content
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

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

Open
Infeligo opened this issue Mar 8, 2022 · 6 comments
Assignees
Labels
in: jdbc Spring Data JDBC type: bug A general bug

Comments

@Infeligo
Copy link

Infeligo commented Mar 8, 2022

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

  }
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 8, 2022
@schauder schauder self-assigned this Mar 10, 2022
@schauder schauder added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Mar 10, 2022
@schauder
Copy link
Contributor

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.

@schauder schauder added the in: jdbc Spring Data JDBC label Jul 8, 2022
@loolzaaa
Copy link

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
Copy link
Contributor

The code you pointed to resides in spring-data-commons and read converters where explicitly excluded. See spring-projects/spring-data-mongodb#1203

So we can't simply undo that change.

@mp911de
Copy link
Member

mp911de commented Jun 20, 2024

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
Copy link
Contributor

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

Related: #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
Copy link
Contributor

We had an internal discussion and these are our findings:

  • Spring Data JDBC currently uses isEntity with the interpretation of mapsToASeparateTable
    eventually we want to make that distinction explicit so it can take conversions properly into account.
  • There is a challenge here since the mapping package (what object/field maps to which table/column) should not depend on the conversion package, but it does need the information from it. This is different from other modules where the mapping is largely independent of conversions.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: jdbc Spring Data JDBC type: bug A general bug
Projects
None yet
Development

No branches or pull requests

5 participants