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

Enhancement proposal: give jdbcclient access to conversion services for converting custom object types from db. #33467

Open
alexanderankin opened this issue Aug 25, 2024 · 4 comments
Labels
in: data Issues in data modules (jdbc, orm, oxm, tx) status: waiting-for-triage An issue we've not yet triaged or decided on

Comments

@alexanderankin
Copy link

alexanderankin commented Aug 25, 2024

Enhancement Description: allow JdbcClient to convert custom object types in records. for example, in postgres you may have a jsonb and you'd want to be able to convert that into something so that you can parse it onto your record class while fetching:

record CustomRecord(JsonNode jsonbColumn) {}
jdbcClient.sql("select jsonb_column from some_table").query(CustomRecord.class).list();

code references:

possible solution:

  • refactor DefaultJdbcClient constructors
from their current form
class DefaultJdbcClient {

    private final JdbcOperations classicOps;

    private final NamedParameterJdbcOperations namedParamOps;

    private final Map<Class<?>, RowMapper<?>> rowMapperCache = new ConcurrentHashMap<>();


    public DefaultJdbcClient(DataSource dataSource) {
        this.classicOps = new JdbcTemplate(dataSource);
        this.namedParamOps = new NamedParameterJdbcTemplate(this.classicOps);
    }

    public DefaultJdbcClient(JdbcOperations jdbcTemplate) {
        Assert.notNull(jdbcTemplate, "JdbcTemplate must not be null");
        this.classicOps = jdbcTemplate;
        this.namedParamOps = new NamedParameterJdbcTemplate(jdbcTemplate);
    }

    public DefaultJdbcClient(NamedParameterJdbcOperations jdbcTemplate) {
        Assert.notNull(jdbcTemplate, "JdbcTemplate must not be null");
        this.classicOps = jdbcTemplate.getJdbcOperations();
        this.namedParamOps = jdbcTemplate;
    }
}
to this pattern:
class DefaultJdbcClient {
    private final JdbcOperations classicOps;
    private final NamedParameterJdbcOperations namedParamOps;
    private final ConversionService conversionService;
    private final Map<Class<?>, RowMapper<?>> rowMapperCache = new ConcurrentHashMap<>();

    public DefaultJdbcClient(DataSource dataSource) {
        this(new JdbcTemplate(dataSource));
    }

    public DefaultJdbcClient(JdbcOperations jdbcTemplate) {
        this(new NamedParameterJdbcTemplate(jdbcTemplate));
    }

    public DefaultJdbcClient(NamedParameterJdbcOperations jdbcTemplate) {
        this(jdbcTemplate.getJdbcOperations(), jdbcTemplate, DefaultConversionService.getSharedInstance());
    }

    public DefaultJdbcClient(
            JdbcOperations jdbcOperations,
            NamedParameterJdbcOperations namedParameterJdbcOperations,
            ConversionService conversionService
    ) {
        Assert.notNull(jdbcOperations, "jdbcOperations must not be null");
        Assert.notNull(namedParameterJdbcOperations, "namedParameterJdbcOperations must not be null");
        Assert.notNull(conversionService, "conversionService must not be null");
        this.classicOps = jdbcOperations;
        this.namedParamOps = namedParameterJdbcOperations;
        this.conversionService = conversionService;
    }
}
  • add a JdbcClient.create method that is just going to be kept in sync with the bottom constructor (just allow user to pass all components)
  • pass this conversion service where appropriate to simple property row mapper constructors et al
  • change the AutoConfiguration class from
	@Bean
	JdbcClient jdbcClient(NamedParameterJdbcTemplate jdbcTemplate) {
		return JdbcClient.create(jdbcTemplate);
	}

to

	@Bean
	JdbcClient jdbcClient(NamedParameterJdbcTemplate jdbcTemplate, /*kosher?*/ @Nullable ConversionService conversionService) {
		return JdbcClient.create(jdbcTemplate, java.util.Objects.requireNonNullElseGet(conversionService, DefaultConversionService::getSharedInstance));
	}

benefits

you can now register converters as beans and they will be able to be used for jdbcClient queries.

considerations

  • not sure how ok it is to rely on the conversion service bean as I understand its intent was for converting http requests/responses, hence why i made it an optional dependency and defaulted it to: java.util.Objects.requireNonNullElseGet(conversionService, DefaultConversionService::getSharedInstance).
  • alternative to consider is just to write boilerplate, eg. by creating your own row mappers - https://stackoverflow.com/a/78655612
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Aug 25, 2024
@alexanderankin
Copy link
Author

for anyone else struggling with this the solution was to add the jdbc driver for your database to the compilation scope (e.g. from runtimeClasspath to implementation in gradle terms) and simply refer the types in the jdbc driver in your record/classes for fetching. a conversion service for the jdbcclient would still be an improvement though, im not sure using types from a jdbc driver is a great choice.

@quaff
Copy link
Contributor

quaff commented Aug 26, 2024

It requires changes both in Spring Framework (construct DefaultJdbcClient by accepting ConversionService) and Spring Boot (autoconfigure DefaultJdbcClient by injecting ConversionService), it's not appropriate to add ConversionService as property of DefaultJdbcClient because conversion is not the responsibility of it, why not passing rowMapper instead of mappedClass ?

jdbcClient.sql("select jsonb_column from some_table").query(new SimplePropertyRowMapper<>(CustomRecord.class, conversionService)).list();

@philwebb philwebb added the status: waiting-for-feedback We need additional information before we can continue label Aug 27, 2024
@alexanderankin
Copy link
Author

If this issue is waiting for feedback from me, I would just say that it appears part of the plumbing is already there, so I am just trying to improve the wiring. clearly the responsibility of the client is to return pojos, which includes conversion. why not support conversion from the spring framework? this preserves the typical responsibility of framework to do, and boot to configure.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Aug 28, 2024
@bclozel
Copy link
Member

bclozel commented Sep 2, 2024

We have discussed it as a team and here is our assessment:

From a Spring Framework perspective, this would indeed mean adding constructor variants for DefaultJdbcClient accepting a custom ConversionService (to be used in place of the default one, if present). This would then be used for rowmappers calls internally.

From a Spring Boot perspective, we don't think that we should change the auto-configuration there, as there is a default conversion service used for binding properties, another one for the web parts of the application, and possibly more... Instead, we think that the users can create a custom JdbcClient bean in their configuration. The JdbcClientAutoConfiguration is really straightforward so the effort would be minimal and the opinion always right.

We are declining this enhancement from a Spring Boot perspective, and transferring this issue to the Framework team so they can consider this request.

Thanks!

@bclozel bclozel removed the status: feedback-provided Feedback has been provided label Sep 2, 2024
@bclozel bclozel transferred this issue from spring-projects/spring-boot Sep 2, 2024
@bclozel bclozel added the in: data Issues in data modules (jdbc, orm, oxm, tx) label Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: data Issues in data modules (jdbc, orm, oxm, tx) status: waiting-for-triage An issue we've not yet triaged or decided on
Projects
None yet
Development

No branches or pull requests

5 participants