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

Fix PlayEbean configuration loading for sub projects (#209) #210

Merged
merged 1 commit into from May 25, 2021

Conversation

LeComptoirDesPharmacies
Copy link
Contributor

The goal of this PR is to modify the way Configuration was moved to Config in order to keep the capability to build sub projects with PlayEbean inside.

See #209 for more details.

@lightbend-cla-validator
Copy link

Hi @LeComptoirDesPharmacies,

Thank you for your contribution! We really value the time you've taken to put this together.

Before we proceed with reviewing this pull request, please sign the Lightbend Contributors License Agreement:

https://www.lightbend.com/contribute/cla

@felipebonezi
Copy link
Contributor

@LeComptoirDesPharmacies hi, this PR is very important to our community.

Did you Test with multiple sbt projects?
BTW, can you explain why just change to Scala Configuration class fix it?

@LeComptoirDesPharmacies
Copy link
Contributor Author

Hi @felipebonezi,

On our side we built a fork of 6.0.0 with content of this PR and we are running it on our production server but we would like to use the official build to benefit from future enhancements.

We created this minimal reproducer that you can clone/run to see the error/behavior. (Just go to localhost:9000)
https://github.com/LeComptoirDesPharmacies/IssuePlayMultiProject

Here is a branch of the same project using old "play-ebean 4.1.4" which was working.
https://github.com/LeComptoirDesPharmacies/IssuePlayMultiProject/tree/workingIn2.6

We did not test this on other SBT projects but do not hesitate to tell us If you have any project in mind where we could test it.

Explanation of the problem:
In versions < Play 2.7, "Configuration" class was reworked with a "Config" interface.
This PR (https://github.com/playframework/play-ebean/pull/157/files) updated the ModelsConfigLoader.java to reflect this new naming.
image

As you can see, it does not only rename "Configuration -> Config" but it also use env.classLoader() instead of env.
env.classLoader() is the exact same meaning as using the method input parameter classLoader.
In this context, the following lines of code and comment are circumvented, creating error when conf file does not exists.
// Using TEST mode is the only way to load configuration without failing if application.conf doesn't exist Environment env = new Environment(new File("."), classLoader, Mode.TEST);

I think that the author does that because "ConfigFactory" does not have any method with signature capable to "load" "play.Environment" class.

Solution of this PR :
The tips here was to move from Java Environment to Scala one in order to get the good "Configuration.load" signature available.
Then, when calling the "EbeanParsedConfig.parseFromConfig" method, we are using config.underlying to retrieve the Java instance from Scala type as this method require it.

Yours faithfully,
LCDP

@jmswenski jmswenski self-requested a review May 25, 2021 19:46
@jmswenski
Copy link
Contributor

Tested changes out with our app. LGTM.

@jmswenski jmswenski merged commit 36c6f65 into playframework:master May 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants