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

Add 'dropwizard-jdbi3' module #2243

Merged
merged 15 commits into from Jan 18, 2018

Conversation

Projects
None yet
4 participants
@FredDeschenes
Contributor

FredDeschenes commented Jan 16, 2018

Problem:

jDBI3 as been released for a couple of months and with the recent update to Metrics 4.X Dropwizard can now use it without losing any of the features from jDBI2. This should of course not be a forced upgrade and allow users to decide which jDBI version they wish to use in their project. jDBI now also supports most of the custom mappers that were in dropwizard-jdbi out of the box or in a separate module, removing some code from Dropwizard.

Solution:

Create a dropwizard-jdbi3 module alongside the existing dropwizard-jdbi module.

Result:

Added a basic implementation of dropwizard-jdbi3, without any of the previous mappers or documentation.

TODO:

  • Add documentation (how should this be approached? a new page for the module? have documentation for both modules in the current page?)
  • Check if any existing mappers need to be implemented for jDBI3 (jDBI has a module for Guava which seems to cover every mapper Dropwizard had, Joda time is effectively deprecated, we might need the OptionalInt/Long/Double/etc)
  • Add unit tests for JdbiFactory
@isaki

This comment has been minimized.

Contributor

isaki commented Jan 17, 2018

For JdbiFactory, can you please follow the configure paradigm in io.dropwizard.jdbi.DBIFactory? I had implemented that to inject custom configuration in my own subclass of DBIFactory. Thanks.

@FredDeschenes

This comment has been minimized.

Contributor

FredDeschenes commented Jan 17, 2018

@isaki-x : Will do.

@FredDeschenes

This comment has been minimized.

Contributor

FredDeschenes commented Jan 17, 2018

I validated that all Dropwizard-specific mappers are now not required as they are either supported out of the box by jDBI or one of its plugins (Joda or Guava). Should we include those plugins by default or just document that they exist and should be added if required by the project?

@arteam

This comment has been minimized.

Member

arteam commented Jan 17, 2018

I think it's better to leave them out. For users who uses Guava and JodaTime, the modules easy to include to their projects.

@arteam arteam self-assigned this Jan 17, 2018

@arteam arteam added the feature label Jan 17, 2018

@arteam arteam added this to the 1.3.0 milestone Jan 17, 2018

@arteam arteam removed their assignment Jan 17, 2018

@arteam arteam self-requested a review Jan 17, 2018

@joschi

This comment has been minimized.

Member

joschi commented Jan 17, 2018

@FredDeschenes I'd vote for including these plugins by default because Dropwizard usually comes with "batteries included" for the libraries it's using itself (i. e. JodaTime and Guava).

@arteam

Looks good! I've left minor comments.

String name) {
// Create the instance
final Jdbi jdbi = this.newInstance(dataSource);

This comment has been minimized.

@arteam

arteam Jan 17, 2018

Member

this seems to be redundant, because there's no superclass for JdbiFactory which has a method with the same name.

This comment has been minimized.

@FredDeschenes

FredDeschenes Jan 17, 2018

Contributor

Removed 'this'

final ExtensionMethod extensionMethod = ctx.getExtensionMethod();
final String originalResult = originalEngine.render(template, ctx);
if ((extensionMethod != null)

This comment has been minimized.

@arteam

arteam Jan 17, 2018

Member

I think it would be more clear to make a check that the extension method doesn't exist and exit early.

if (extensionMethod == null || extensionMethod.getType() == null || extensionMethod.getMethod() == null) {
            return originalResult;
}

This comment has been minimized.

@FredDeschenes

FredDeschenes Jan 17, 2018

Contributor

Turns out ExtensionMethod has requireNonNull for type and method (which couldn't really be null anyway) so I'll remove those unnecessary checks as well.

* Jdbi {@link TemplateEngine} that prepends the SQLObject's type and method name in front of every
* statement sent to the database.
*/
public class NamePrependingTemplateEngine implements TemplateEngine {

This comment has been minimized.

@arteam

arteam Jan 17, 2018

Member

It would be nice to have a simple unit test for NamePrependingTemplateEngine to verify that it actually inserts comments to the query.

This comment has been minimized.

@FredDeschenes

FredDeschenes Jan 17, 2018

Contributor

Test added

new JdbiExceptionsBundle().run(environment);
verify(jerseyEnvironment, times(1)).register(isA(LoggingSQLExceptionMapper.class));

This comment has been minimized.

@arteam

arteam Jan 17, 2018

Member

times(1) seems to be redundant, because verify by default checks that the call has happened only once.

Handle handle = mock(Handle.class);
when(jdbi.open()).thenReturn(handle);
when(jdbi.withHandle(Mockito.any())).thenCallRealMethod();
Mockito.doAnswer(invocation -> {

This comment has been minimized.

@arteam

arteam Jan 17, 2018

Member

The handle.execute method returns int instead of void, so we can use the when().doAnswer form.

when(jdbi.withHandle(Mockito.any())).thenCallRealMethod();
Mockito.doAnswer(invocation -> {
try {
TimeUnit.SECONDS.sleep(10);

This comment has been minimized.

@arteam

arteam Jan 17, 2018

Member

The answer method can throw exceptions, so we can avoid catching the exception.

validationQuery);
HealthCheck.Result result = jdbiHealthCheck.check();
executorService.shutdown();
assertThat("is unhealthy", false, is(result.isHealthy()));

This comment has been minimized.

@arteam

arteam Jan 17, 2018

Member

Could you please use an AssertJ assertions for consistency with rest of the codebase?

This comment has been minimized.

@FredDeschenes

FredDeschenes Jan 17, 2018

Contributor

Yeah this was copied from the DBIHealthcheck in the jdbi2 module, I'll convert them.

* Iterates through a {@link JdbiException}'s cause if it's a {@link SQLException} otherwise log as normal.
*/
@Provider
public class LoggingJdbiExceptionMapper extends LoggingExceptionMapper<JdbiException> {

This comment has been minimized.

@arteam

arteam Jan 17, 2018

Member

I guess the file also should be named LoggingJdbiExceptionMapper instead LoggingJDBIExceptionMapper

This comment has been minimized.

@FredDeschenes

FredDeschenes Jan 17, 2018

Contributor

Yeah the joys of file paths on Windows with Git... I'll rename those properly.

@arteam

This comment has been minimized.

Member

arteam commented Jan 17, 2018

I'd vote for including these plugins by default because Dropwizard usually comes with "batteries included" for the libraries it's using itself (i. e. JodaTime and Guava).

On second thought, I think it makes sense. The modules are small and it will make transition from dropwizard-jdbi2 a whole lot simpler.

@FredDeschenes

This comment has been minimized.

Contributor

FredDeschenes commented Jan 17, 2018

I added the JodaTime and Guava plugins and also added the jDBI3-bom to Dropwizard's bom, but I'm not super used to Maven, just make sure I haven't messed up the dependencies to everything else ;)

@FredDeschenes

This comment has been minimized.

Contributor

FredDeschenes commented Jan 17, 2018

Concerning the documentation, I thought about having a new 'JDBI3' page with updated documentation content (although it's going to be pretty similar) and flagging the 'jdbi2' page to tell new projects that they should use the new 'jdbi3' module but existing projects can update at their discretion. Sounds good?

EDIT: I'll also update the paths in the JDBI2 docs as some of them are now broken.

@arteam

arteam approved these changes Jan 17, 2018

Nice! I've left a pair of comments.

when(jdbi.open()).thenReturn(handle);
when(jdbi.withHandle(Mockito.any())).thenCallRealMethod();
when(handle.execute(validationQuery)).thenAnswer((Answer<Integer>) invocation -> null);

This comment has been minimized.

@arteam

arteam Jan 17, 2018

Member

Because the method returns an integer, I think we could just use thenReturn(0)

@Test
public void testNoTimeoutReturnsHealthy() throws Exception {
String validationQuery = "select 1";

This comment has been minimized.

@arteam

arteam Jan 17, 2018

Member

It seems that we could extract preparing of executorService, validationQuery, jdbi and handle to a setup method and share it between the tests.

jdbi,
validationQuery);
HealthCheck.Result result = jdbiHealthCheck.check();
executorService.shutdown();

This comment has been minimized.

@arteam

arteam Jan 17, 2018

Member

I think it would be nice to shutdown the executor in a tearDown method, so even if the test fails, we correctly close resources.

ExecutorService executorService = Executors.newSingleThreadExecutor();
JdbiHealthCheck jdbiHealthCheck = new JdbiHealthCheck(executorService,
Duration.milliseconds(5),

This comment has been minimized.

@arteam

arteam Jan 17, 2018

Member

I would increase timeout from 5ms to 100ms. 5ms is too little to get a result from a new thread. The test may fail with a timeout while the new thread is being scheduled to run.

ExecutorService executorService = Executors.newSingleThreadExecutor();
JdbiHealthCheck jdbiHealthCheck = new JdbiHealthCheck(executorService,
Duration.milliseconds(5),

This comment has been minimized.

@arteam

arteam Jan 17, 2018

Member

I would increase the timeout, because a test with 5ms timeout can fail sporadically, not because we actually wait for a result from the database.

final TemplateEngine original = mock(TemplateEngine.class);
final StatementContext ctx = mock(StatementContext.class);
final String template = UUID.randomUUID().toString();
final String originalRendered = UUID.randomUUID().toString();

This comment has been minimized.

@arteam

arteam Jan 17, 2018

Member

I think we could extract preparing of the variables to a setup method and share it between the tests.

@Override
protected Result check() throws Exception {
return timeBoundHealthCheck.check(() ->
jdbi.withHandle((handle) -> {

This comment has been minimized.

@arteam

arteam Jan 17, 2018

Member

On second thought, I think it would be better to use try (Handle handle = jdbi.open()) instead of withHandle. The biggest advantages of lambda-expressions is that we could use a short-form for one-line calls or even reference a method. Here we use two commands, so basically the both expressions require the same amount of code. At the same time the former version is easier to test.

@arteam

This comment has been minimized.

Member

arteam commented Jan 17, 2018

Concerning the documentation, I thought about having a new 'JDBI3' page with updated documentation content (although it's going to be pretty similar) and flagging the 'jdbi2' page to tell new projects that they should use the new 'jdbi3' module but existing projects can update at their discretion. Sounds good?

Sounds good to me!

@arteam

This comment has been minimized.

Member

arteam commented Jan 17, 2018

I think we also need to add an integration test test to verify that Dropwizard and Jdbi3 together and you can write database access code with JdbiFactory. For example, for Jdbi2 we've got JDBITest. In my personal project, I wrote a couple tests which showcase Jdbi features, you can get an idea:

@isaki

This comment has been minimized.

Contributor

isaki commented Jan 18, 2018

One note on the configure implementation, you can drop the PooledDataSourceFactory argument; the was something I hadn't wanted there in the first place but had no other simple way of getting some of the things required to configure the legacy mappers/translators. There is no harm in leaving it, it just was something added as a shortcut and due to design requirements. Thanks!

'Jdbi3' -> 'JDBI3'
For consistency

@FredDeschenes FredDeschenes changed the title from WIP : Add 'dropwizard-jdbi3' module to Add 'dropwizard-jdbi3' module Jan 18, 2018

FredDeschenes added some commits Jan 18, 2018

@arteam arteam merged commit 898118b into dropwizard:master Jan 18, 2018

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@arteam

This comment has been minimized.

Member

arteam commented Jan 18, 2018

@FredDeschenes Thank you very much for the contribution, great work! I'm going to make a RC shortly.

@FredDeschenes FredDeschenes deleted the FredDeschenes:jdbi3 branch Jan 19, 2018

@isaki

This comment has been minimized.

Contributor

isaki commented Jan 19, 2018

This is super exciting, I look forward to migrating when this is ready. Thanks for the work @FredDeschenes !

aaanders added a commit to aaanders/dropwizard that referenced this pull request Sep 20, 2018

Add 'dropwizard-jdbi3' module (dropwizard#2243)
* Start adding 'dropwizard-jdbi3' module

* Re-added 'configure' method

* 'DBI' -> 'JDBI' in doc and class names

* Add JdbiFactoryTest

* Fixed code review issues

* Add jDBI3 Guava and JodaTime plugins

* More code review fixes

* Add Jdbi3 'SqlObject' plugin be default

* Add Jdbi3 integration test

* Remove unused parameter from JdbiFactory::configure

* NullAway doesn't like Mockito apparently

* Add JDBI3 documentation

* 'Jdbi3' -> 'JDBI3'

For consistency

* Fixed 'Plugins' header

Oups...

* Fixed documentation issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment