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

Don't use play.modules.disabled #245

Closed
jroper opened this issue May 5, 2015 · 2 comments

Comments

Projects
None yet
2 participants
@jroper
Copy link
Member

commented May 5, 2015

play.modules.disabled is intended for end users to be able to override what modules are enabled by default. If libraries provide config for it, this undermines what the user can do - for example, if the user for whatever reason has play-slick on the classpath, but they want it disabled and Play's built in DBModule enabled, then they can't do that, because slick has already provided default configuration that disables Play's built in DBModule.

Additionally, since Play by default provides an empty list for the disabled modules, this means the current configuration is susceptible to classpath ordering issues, if Play's jar is loaded after play-slick, then play-slicks disabled modules config will be overridden by the empty list that Play provides.

@jroper

This comment has been minimized.

Copy link
Member Author

commented May 5, 2015

A few possible solutions here:

  • Require users to explicitly disable Play's built in DbApi module and enable slicks.
  • Separate Play's DbApi and the evolutions plugin out from the implementation, that way, only one of slick and play-jdbc will be on the classpath, and both of them can provide bindings by default, with the user having to disable one if they include both.
  • Rather than having a separate implementation of DbApi and separate configuration namespace, reuse Play's, but implement Play's play.api.db.ConnectionPool trait using Slick - thus Slick implements and manages the connection pool. Users will have to manually specific the slick connection pool for each datasource.

@dotta dotta self-assigned this May 5, 2015

@dotta dotta added the type:defect label May 5, 2015

@dotta dotta added this to the 1.0.0 milestone May 5, 2015

dotta added a commit to dotta/play-slick that referenced this issue May 5, 2015

Don't use play.modules.disabled. Fix playframework#245
The problem with using play.modules.disabled is well explained in the related
ticket. The proposed fix in this commit is to require the user to include
`slick-evolutions.conf` to disable the conflicting `DBApi` implementation wired
by the jdbc module, so that Play Slick can support database evolutions.
@dotta

This comment has been minimized.

Copy link
Contributor

commented May 5, 2015

Require users to explicitly disable Play's built in DbApi module and enable slicks.

While I don't particularly like this solution, this was easy to implement, and here is a link to the commit.

Separate Play's DbApi and the evolutions plugin out from the implementation, that way, only one of slick and play-jdbc will be on the classpath, and both of them can provide bindings by default, with the user having to disable one if they include both.

I like this solution. I thought it was too late for doing such a change in Play 2.4. Looks like it's not, so I'll have a look at this right away.

EDIT: Mmmm, many play db traits have a companion object, and that's a problem as I can't split traits and companion object in different projects. Hence, I'm no longer convinced this a viable solution.

EDIT: I found a nice solution for the trait + companion object problem. So, this works nicely:

dotta added a commit to dotta/playframework that referenced this issue May 5, 2015

Splitted play-jdbc into three different modules
The reason for splitting play-jdbc into three modules is that I need to provide
a different implementation of `DBApi` in Play Slick to support Play database
evolutions. This solution was somewhat hinted by @jroper in this comment
playframework/play-slick#245 (comment)
(the second bullet).

I still need to clean up this commit and move tests in the proper module.
However, before investing more time I'd like to know if this has a chance to get
merged in time for Play 2.4 final release.

dotta added a commit to dotta/play-slick that referenced this issue May 5, 2015

Updated code after splitting of play-jdbc. Fix playframework#245
By splitting play-jdbc in different modules, the Play Slick module can depend
on play-jdbc-api and play-jdbc-evolutions to support Play database evolutions in
Slick.

This PR depends on playframework/playframework#4380

dotta added a commit to dotta/play-slick that referenced this issue May 6, 2015

Better implementation for supporting Play database evolutions. Fix pl…
…ayframework#245

By splitting play-jdbc in different modules, the Play Slick module can depend
on play-jdbc-api and play-jdbc-evolutions to support Play database evolutions in
Slick.

This commit depends on playframework#4380

dotta added a commit to dotta/play-slick that referenced this issue May 6, 2015

Better implementation for supporting Play database evolutions. Fix pl…
…ayframework#245

By splitting play-jdbc in different modules, the Play Slick module can depend
on play-jdbc-api and play-jdbc-evolutions to support Play database evolutions in
Slick.

This commit depends on playframework/playframework#4380

dotta added a commit to dotta/play-slick that referenced this issue May 10, 2015

Improved database evolutions support. Fix playframework#245
After having split play-jdbc in different modules, the Play Slick module can depend
on play-jdbc-api alone, allowing it to bind `DBApi` to a different implementation than
the one provided by `play-jdbc`. The clear benefit of this change is that we no longer
need to disable modules in the reference.conf, because both the `DBModule` and `HikariCPModule` are no longer in the classpath.

Furthermore, evolutions support was moved out of the Play Slick core, so that it
is now an opt-in feature.

This commit depends on playframework/playframework#4380

@dotta dotta closed this in 9313f8a May 12, 2015

richdougherty added a commit that referenced this issue May 12, 2015

Merge pull request #247 from dotta/wip/without-dependency-on-play-jdbc
Better implementation for supporting Play database evolutions. Fix #245
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.