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

Refactor of DBIFactory (Issue #2056) #2067

Merged
merged 3 commits into from Jun 8, 2017

Conversation

Projects
None yet
5 participants
@isaki
Contributor

isaki commented Jun 1, 2017

Refactor of DBIFactory (Issue #2056)

  • Split DBI construction and configuration into dedicated protected methods.
  • Added unit tests to verify default configuration.

Please also merge this into master.

Thank you!

isaki added some commits Jun 1, 2017

Refactor of DBIFactory (Issue #2056)
- Split DBI construction and configuration into dedicated protected
  methods.
- Added unit tests to verify default configuration.
@coveralls

This comment has been minimized.

coveralls commented Jun 1, 2017

Coverage Status

Coverage increased (+0.008%) to 84.77% when pulling 6ee105a on isaki-x:dw-2056 into 07a1087 on dropwizard:release/1.1.x.

@jplock jplock added this to the 1.1.2 milestone Jun 1, 2017

@jplock jplock added the improvement label Jun 1, 2017

* @param dbi
* @param configuration
*/
protected void configure(final DBI dbi, final PooledDataSourceFactory configuration) {

This comment has been minimized.

@nickbabcock

nickbabcock Jun 2, 2017

Contributor

Should the argument be String driverClazz instead of PooledDataSourceFactory? It doesn't look like any other properties from the configuration are useful for JDBI. And it would make it easier to extract the creation and configuring of a DBI instance that isn't tied to a DBIFactory

This comment has been minimized.

@nickbabcock

nickbabcock Jun 2, 2017

Contributor

Just realized databaseTimeZone needs to be accounted for too, so might not be as straightforward as I initially thought 🤔

This comment has been minimized.

@isaki

isaki Jun 2, 2017

Contributor

I originally had configure taking a second parameter as a String (called driverClazz), however the configuration class also happens to be the factory. Should anyone extend PooledDataSourceFactory to have extra options in their YAML config, they may wish to have that extra data available to their own configure method. That was my thinking anyway. I can very easily swap this back to using just a plain String.

As for databaseTimeZone, that is taken care of by a method so configure doesn't access it directly.

In retrospect, it might be ideal to take that as a parameter to configure instead of the method override approach; however I don't want to break any existing code in the field that might be using an override of the database time zone method.

This comment has been minimized.

@nickbabcock

nickbabcock Jun 2, 2017

Contributor

Good point, I probably should stop trying to kill two birds with one stone, and continue to do what I am currently doing. I maintain a JdbiUtils in my code, so that I can do the following in my tests:

final DBI jdbi = JdbiUtils.configureDbi(new DBI("url"), "", Optional.empty());

I don't think this should hold up this PR. 👍

This comment has been minimized.

@isaki

isaki Jun 2, 2017

Contributor

There could be another approach. We could create a minimally defined interface that is parent of PooledDataSourceFactory that has only the method(s) we need for setting up the default argument settings. Then, the configure method could take this interface as a second argument. If this interface has only one method, say getDriverClass(), then for easy testing you could do something like this:

configure(new DBI("url"), () -> "io.dropwizard.driver.Driver");

This comment has been minimized.

@nickbabcock

nickbabcock Jun 2, 2017

Contributor

That would be interesting to see 😄

But if you think that would detract from the PR, we can continue in another issue/pr.

This comment has been minimized.

@isaki

isaki Jun 2, 2017

Contributor

I'll look into it; I want to get the API right now so when I subclass this, I don't have to change it later if I don't have to. It might be worth moving to another issue, but it doesn't hurt to think about for a bit.

This comment has been minimized.

@isaki

isaki Jun 5, 2017

Contributor

After looking into this, I feel it is probably worth pushing to another issue/PR and I'll update my downstream code when the time comes.

I have pushed one last change to remove some unused imports from the unit test.

This comment has been minimized.

@nickbabcock

nickbabcock Jun 5, 2017

Contributor

Ok sounds good to me 👍

@isaki

This comment has been minimized.

Contributor

isaki commented Jun 5, 2017

For some reason the Travis CI changes completed successfully but the pull request shows them as pending.

@isaki

This comment has been minimized.

Contributor

isaki commented Jun 5, 2017

Github support has asked me to close and re-open this to try and fix the hung Travis status.

@isaki isaki closed this Jun 5, 2017

@isaki

This comment has been minimized.

Contributor

isaki commented Jun 5, 2017

Re-opening as per GitHub support.

@isaki isaki reopened this Jun 5, 2017

@coveralls

This comment has been minimized.

coveralls commented Jun 5, 2017

Coverage Status

Coverage increased (+0.008%) to 84.77% when pulling ce5a5e0 on isaki-x:dw-2056 into 07a1087 on dropwizard:release/1.1.x.

@arteam arteam merged commit f0aafde into dropwizard:release/1.1.x Jun 8, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.008%) to 84.77%
Details
@arteam

This comment has been minimized.

Member

arteam commented Jun 8, 2017

Thank you very much for the contribution and a very thorough unit test!

@isaki isaki deleted the isaki:dw-2056 branch Jun 8, 2017

arteam added a commit that referenced this pull request Jun 8, 2017

Refactor of DBIFactory (Issue #2056) (#2067)
* Refactor of DBIFactory (Issue #2056)

- Split DBI construction and configuration into dedicated protected
  methods.
- Added unit tests to verify default configuration.

* Removed unnecessary whitespace left by Eclipse

* Fixed warnings in unit test for DBIFactory.
@arteam

This comment has been minimized.

Member

arteam commented Jun 8, 2017

Applied to master as b6097f2

@isaki

This comment has been minimized.

Contributor

isaki commented Jun 8, 2017

Thank you!

sankate pushed a commit to sankate/dropwizard that referenced this pull request Nov 21, 2017

Refactor of DBIFactory (Issue dropwizard#2056) (dropwizard#2067)
* Refactor of DBIFactory (Issue dropwizard#2056)

- Split DBI construction and configuration into dedicated protected
  methods.
- Added unit tests to verify default configuration.

* Removed unnecessary whitespace left by Eclipse

* Fixed warnings in unit test for DBIFactory.

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

Refactor of DBIFactory (Issue dropwizard#2056) (dropwizard#2067)
* Refactor of DBIFactory (Issue dropwizard#2056)

- Split DBI construction and configuration into dedicated protected
  methods.
- Added unit tests to verify default configuration.

* Removed unnecessary whitespace left by Eclipse

* Fixed warnings in unit test for DBIFactory.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment