Introduce configuration parameter to control the generation of the evolution SQL files #433

ghost · 2024-01-10T16:31:16Z

Closes #366

mkurz

Thanks for you time to provide this pull request.
Some things need to be changed, please refer my comments. Specially we need to keep binary compatibility so I can release that as new minor or patch release without breaking user code.
Thanks!

mkurz · 2024-01-11T09:10:17Z

play-ebean/src/main/java/play/db/ebean/DefaultEbeanConfig.java

@@ -24,15 +24,24 @@
 /** Ebean server configuration. */
 @Singleton
 public class DefaultEbeanConfig implements EbeanConfig {
+  private final Boolean ddlGenerate;


Let's rename this variable (and the config key, etc.) to

Suggested change

private final Boolean ddlGenerate;

private final Boolean generateEvolutionsScripts;

mkurz · 2024-01-11T09:13:40Z

play-ebean/src/main/java/play/db/ebean/DefaultEbeanConfig.java

+  public DefaultEbeanConfig(
+      Boolean ddlGenerate, String defaultServer, Map<String, DatabaseConfig> serverConfigs) {


I prefer to put the new parameter last:

Suggested change

public DefaultEbeanConfig(

Boolean ddlGenerate, String defaultServer, Map<String, DatabaseConfig> serverConfigs) {

public DefaultEbeanConfig(

String defaultServer, Map<String, DatabaseConfig> serverConfigs, Boolean generateEvolutionsScripts) {

Also, you can see the binary compatibility checks are failing, because you bascially removed a constructor. So you we have to keep that constructor to not break any user code. Please re-add it like:

public DefaultEbeanConfig(String defaultServer, Map<String, DatabaseConfig> serverConfigs) { this(defaultServer, serverConfigs, true); }

This will just call the new constructor, keeping generating the evolutions files.

Initially I didn't quite understood what this was all about.

mkurz · 2024-01-11T09:13:57Z

play-ebean/src/main/java/play/db/ebean/DefaultEbeanConfig.java

+  @Override
+  public Boolean ddlGenerate() {
+    return ddlGenerate;
+  }


Method now needs to be renamed as well.

mkurz · 2024-01-11T09:14:17Z

play-ebean/src/main/java/play/db/ebean/EbeanConfig.java

@@ -8,6 +8,7 @@
 import java.util.Map;

 public interface EbeanConfig {
+  Boolean ddlGenerate();


Please rename as well.

mkurz · 2024-01-11T09:18:20Z

play-ebean/src/main/java/play/db/ebean/EbeanDynamicEvolutions.java

@@ -64,6 +64,9 @@ public void create() {
    if (environment.isProd()) {
      return;
    }
+    if (config.ddlGenerate().equals(Boolean.FALSE)) {


This is bad practice, you should call equals on Boolean.FALSE because it will never be null. Right now you risk running into a NullPointerException in case config.ddlGenerate() would return null. See:

jshell> Boolean foo = null; foo ==> null jshell> Boolean.FALSE.equals(foo); // good practice $2 ==> false jshell> foo.equals(Boolean.FALSE); // bad practice | Exception java.lang.NullPointerException | at (#3:1)

So your code should end up like:

Suggested change

if (config.ddlGenerate().equals(Boolean.FALSE)) {

if (Boolean.FALSE.equals(config.generateEvolutionsScripts())) {

mkurz · 2024-01-11T09:19:30Z

play-ebean/src/main/java/play/db/ebean/EbeanParsedConfig.java

-  public EbeanParsedConfig(String defaultDatasource, Map<String, List<String>> datasourceModels) {
+  public EbeanParsedConfig(
+      Boolean ddlGenerate, String defaultDatasource, Map<String, List<String>> datasourceModels) {
+    this.ddlGenerate = ddlGenerate;


This constructor is the same story I described before at the other constructor, please follow what I described there also, thanks!

mkurz · 2024-01-11T09:19:43Z

play-ebean/src/main/java/play/db/ebean/EbeanParsedConfig.java

    this.defaultDatasource = defaultDatasource;
    this.datasourceModels = datasourceModels;
  }

+  public Boolean getDdlGenerate() {


Please rename.

mkurz · 2024-01-11T09:21:34Z

play-ebean/src/main/java/play/db/ebean/EbeanParsedConfig.java

    String ebeanConfigKey = playEbeanConfig.getString("config");
+    Boolean ebeanDdlGenerateKey = playEbeanConfig.getBoolean("ddlGenerate");
+    String ebeanDefaultDatasourceKey = playEbeanConfig.getString("defaultDatasource");


Please do not add a ...Key suffix to the variable names here. I see you did that because of how ebeanConfigKey is named, but this is different, because ebeanConfigKey actually really contains a config key, but the other variables do not, they contain actual values.
So please for the two variables, remove the ...Key suffix (and rename the new var accordingly).

mkurz · 2024-01-11T09:21:57Z

play-ebean/src/main/resources/reference.conf

@@ -7,6 +7,9 @@ play {
    # The key for ebean config
    config = "ebean"

+    # The key to control the generation of the evolution SQL files
+    ddlGenerate = true


Please rename.

ghost · 2024-01-11T22:16:41Z

I see the pipeline failing with this error: abstract method generateEvolutionsScripts()java.lang.Boolean in interface play.db.ebean.EbeanConfig is present only in current, but the EbeanConfig needs to have that new value, right?

…olution SQL files

…dingly

mkurz · 2024-01-16T13:13:43Z

I see the pipeline failing with this error: abstract method generateEvolutionsScripts()java.lang.Boolean in interface play.db.ebean.EbeanConfig is present only in current, but the EbeanConfig needs to have that new value, right?

Yeah that's because EbeanConfig is an interface and MiMa complains that now all subclasses need to define that method. We can't really work around that.

I pushed a commit to use a primitive boolean instead of the wrapper. Will release 8.1.0 in a minute.
Thanks!

mkurz · 2024-01-16T13:14:05Z

@Mergifyio backport 7.0.x

mergify · 2024-01-16T13:14:33Z

backport 7.0.x

✅ Backports have been created

#436 [7.0.x] Introduce configuration parameter to control the generation of the evolution SQL files (backport #433) by @vadimvera has been created for branch 7.0.x

[7.0.x] Introduce configuration parameter to control the generation of the evolution SQL files (backport #433) by @vadimvera

ihostage · 2024-01-16T13:22:07Z

Yeah that's because EbeanConfig is an interface and MiMa complains that now all subclasses need to define that method. We can't really work around that.

Does default method not help?
It doesn't matter for this, I think. But It's just interesting what behaviour MiMa with adding default method in interface. 🤔

mkurz requested changes Jan 11, 2024

View reviewed changes

Vadim Vera and others added 4 commits January 16, 2024 13:19

Introduce configuration parameter to control the generation of the ev…

d2910e3

…olution SQL files

Rename field to generateEvolutionsScripts and adjust references accor…

6ecdb54

…dingly

Lets use boolean instead of Boolean wrapper

c54a39b

Add MiMa filters

f5a5e22

mkurz merged commit 2a13870 into playframework:main Jan 16, 2024
20 checks passed

mergify bot mentioned this pull request Jan 16, 2024

[7.0.x] Introduce configuration parameter to control the generation of the evolution SQL files (backport #433) by @vadimvera #436

Merged

mkurz added a commit that referenced this pull request Jan 16, 2024

Merge pull request #436 from playframework/mergify/bp/7.0.x/pr-433

cdcf182

[7.0.x] Introduce configuration parameter to control the generation of the evolution SQL files (backport #433) by @vadimvera

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce configuration parameter to control the generation of the evolution SQL files #433

Introduce configuration parameter to control the generation of the evolution SQL files #433

ghost commented Jan 10, 2024

mkurz left a comment •

edited

Loading

mkurz Jan 11, 2024

mkurz Jan 11, 2024

ghost Jan 11, 2024

mkurz Jan 11, 2024

mkurz Jan 11, 2024

mkurz Jan 11, 2024

mkurz Jan 11, 2024

mkurz Jan 11, 2024

mkurz Jan 11, 2024

mkurz Jan 11, 2024

ghost commented Jan 11, 2024

mkurz commented Jan 16, 2024

mkurz commented Jan 16, 2024

mergify bot commented Jan 16, 2024 •

edited

Loading

ihostage commented Jan 16, 2024

	private final Boolean ddlGenerate;
	private final Boolean generateEvolutionsScripts;

		public DefaultEbeanConfig(
		Boolean ddlGenerate, String defaultServer, Map<String, DatabaseConfig> serverConfigs) {

	if (config.ddlGenerate().equals(Boolean.FALSE)) {
	if (Boolean.FALSE.equals(config.generateEvolutionsScripts())) {

Introduce configuration parameter to control the generation of the evolution SQL files #433

Introduce configuration parameter to control the generation of the evolution SQL files #433

Conversation

ghost commented Jan 10, 2024

mkurz left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jan 11, 2024

mkurz commented Jan 16, 2024

mkurz commented Jan 16, 2024

mergify bot commented Jan 16, 2024 • edited Loading

✅ Backports have been created

ihostage commented Jan 16, 2024

mkurz left a comment •

edited

Loading

mergify bot commented Jan 16, 2024 •

edited

Loading