Config

[jonenst]

Other information:
Needs discussion: maven cli tests now behave differently from IDE tests...

Please check if the PR fulfills these requirements (please use '[x]' to check the checkboxes, or submit the PR and then click the checkboxes)

• The commit message follows our guidelines
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

Does this PR already have an issue describing the problem ? If so, link to this issue using '#XXX' and skip the rest
NO

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
feature

What is the current behavior? (You can also link to an open issue here)
The PlatformConfig from defaultConfig() has the hardcoded behavior of looking into powsybl.config.dirs (or ~/.itools if not defined) and looking into the environnement

What is the new behavior (if this is a feature change)?
Allow to choose a PlatformConfigProvider using system variables (-Dpowsybl.config.provider). It should implement a getConfig method that is called (once, it is cached) when using defaultConfig(). By default it keeps the old behavior.

Does this PR introduce a breaking change or deprecate an API? If yes, check the following:

• The Breaking Change or Deprecated label has been added
• The migration guide has been updated in the github wiki (What changes might users need to make in their application due to this PR?)
  NO

[coveralls]

Coverage increased (+0.04%) to 83.381% when pulling 72cd678 on config into 1f56228 on master.

[geofjamg]

For me, using a system variable for choosing the PlatformConfigProvider is a bad solution. You should rely only on service loader and classpath configuration. PlatformConfigProvider implementation should be discovered at runtime using service loader and without any system variable:

• if none is found use default platform config provider (normal behaviour)
• if one is found, use it (for instance to run unit tests)
• if more than one is found, throws an exception

So, in all powsybl modules that needs to run unit tests accessing platform config (almost all...), we only have to add a dependency on a Maven artifact that contains the in EmptyPlatformConfigProvider.

    <dependency>
        <groupid>com.powsybl</groupid>
        <artifactid>powsybl-platform-config-empty<artifactid>
        <version>...</version>
        <scope>test</scope>
    </dependency>

Also, instead of an EmptyPlatformConfig we could have a ClassLoaderPlatformConfig, that would allow developpers to add an src/test/resources/config.json to setup platform config using a json file for unit tests. That would be really convenient because in actual code we need to define platform config data programmatically in unit tests.

[jonenst]

For me, using a system variable for choosing the PlatformConfigProvider is a bad solution. You should rely only on service loader and classpath configuration. PlatformConfigProvider implementation should be discovered at runtime using service loader and without any system variable:
if none is found use default platform config provider (normal behaviour)
if one is found, use it (for instance to run unit tests)
if more than one is found, throws an exception

We had a discussion about these ideas and concluded against using them. We can still discuss. The argument against it were:

• The classpath is easy to get wrong. A jvm system property is a lot easier to get right. For example, lookt at the very well known error using slf4j: "Multiple bindings were found on the class path."

However, if we choose to implement your solution, maybe we should use slf4j behavior's of not throwing an exception for multiple implementations ? "Even when multiple bindings are present, SLF4J will pick one logging framework/implementation and bind with it. The way SLF4J picks a binding is determined by the JVM and for all practical purposes should be considered random. As of version 1.6.6, SLF4J will name the framework/implementation class it is actually bound to."

Or at least explicitly find a good reason not to do like slf4j.

• The classpath is harder to change than a jvm system property.

I'm not sure about this one anymore. For the classpath, because of transitive dependencies, you either have to fix projects far away from the failing tests (hard or impossible), or use exclusions (ugly). However, the system property is maybe even worse because IDEs do not pick the maven system property configuration.. This leads many projects to warn their developpers "the tests run in maven CLI are not the same as the tests run in your IDE". If we can avoid that, we should.

• The classpath is harder to debug than a jvm property. Both in IDEs and in Maven, it's hard to inspect the whole class path and dependency graph..

What do you think ?

Also, instead of an EmptyPlatformConfig we could have a ClassLoaderPlatformConfig, that would allow developpers to add an src/test/resources/config.json to setup platform config using a json file for unit tests. That would be really convenient because in actual code we need to define platform config data programmatically in unit tests.

This means that all the tests in one maven project have the same config. I'm not sure if it's enough to solve the problem of having to define data programmatically in the tests.

If it's enough, an alternative would be to configure the target/ folder as a powsybl.config.dirs of the StandardPlatformConfigLoader but I agree that the classpath approach is simpler and superior.

[sylvlecl]

I think enforcing to have only one config provider in the classpath will not be very convenient :

• you will not be able to define a provider based on another implementation (since it will need a dependency to the other provider, and then you will have 2 providers in classpath)
• You will not be able either to use 2 differents implementations in different unit tests of one module.
• you will have to include the "standard" dependency (or your custom one) in any packaging modules (itools or application), and only in them. It will not be obvious for the application builders.

I think we need to have some priority system, to at least authorize having the default implementation and another one in the classpath. In that case the other one will be used.

But it's probably not enough, because of the first reason mentioned above.

[geofjamg]

I think enforcing to have only one config provider in the classpath will not be very convenient :

* you will not be able to define a provider based on another implementation (since it will need a dependency to the other provider, and then you will have 2 providers in classpath)

* You will not be able either to use 2 differents implementations in different unit tests of one module.

* you will **have** to include the "standard" dependency (or your custom one) in any packaging modules (itools or application), and **only** in them. It will not be obvious for the application builders.

I think we need to have some priority system, to at least authorize having the default implementation and another one in the classpath. In that case the other one will be used.

But it's probably not enough, because of the first reason mentioned above.

But what could be the use case having 2 differents providers in your unit test?
I do not see any issue including the dependency in any modules. You do it with mockito, slf4j-simple, jimfs etc.

[geofjamg]

For me, using a system variable for choosing the PlatformConfigProvider is a bad solution. You should rely only on service loader and classpath configuration. PlatformConfigProvider implementation should be discovered at runtime using service loader and without any system variable:
if none is found use default platform config provider (normal behaviour)
if one is found, use it (for instance to run unit tests)
if more than one is found, throws an exception

We had a discussion about these ideas and concluded against using them. We can still discuss. The argument against it were:

* The classpath is easy to get wrong. A jvm system property is a lot easier to get right. For example, lookt at the very well known error using slf4j: "Multiple bindings were found on the class path."

However, if we choose to implement your solution, maybe we should use slf4j behavior's of not throwing an exception for multiple implementations ? "Even when multiple bindings are present, SLF4J will pick one logging framework/implementation and bind with it. The way SLF4J picks a binding is determined by the JVM and for all practical purposes should be considered random. As of version 1.6.6, SLF4J will name the framework/implementation class it is actually bound to."

Or at least explicitly find a good reason not to do like slf4j.

* The classpath is harder to change than a jvm system property.

I'm not sure about this one anymore. For the classpath, because of transitive dependencies, you either have to fix projects far away from the failing tests (hard or impossible), or use exclusions (ugly). However, the system property is maybe even worse because IDEs do not pick the maven system property configuration.. This leads many projects to warn their developpers "the tests run in maven CLI are not the same as the tests run in your IDE". If we can avoid that, we should.

* The classpath is harder to debug than a jvm property. Both in IDEs and in Maven, it's hard to inspect the whole class path and dependency graph..

What do you think ?

Also, instead of an EmptyPlatformConfig we could have a ClassLoaderPlatformConfig, that would allow developpers to add an src/test/resources/config.json to setup platform config using a json file for unit tests. That would be really convenient because in actual code we need to define platform config data programmatically in unit tests.

This means that all the tests in one maven project have the same config. I'm not sure if it's enough to solve the problem of having to define data programmatically in the tests.

If it's enough, an alternative would be to configure the target/ folder as a powsybl.config.dirs of the StandardPlatformConfigLoader but I agree that the classpath approach is simpler and superior.

Do you agree that having a different behaviour in Maven, and IDE is a showstopper? It would be so a mess.
I totally disagree in your first statement about classpath eaysier to get wrong than system variable. Where does it come from?
Slf4j is a very sucessfull libray and its design is well knwon to be very good. What is the problem with the warning about multiple impl? If you have this warn, you just have to fix it and change your classpath to have only one impl. Pretty simple, clear and well design.
A general comment, are you able to find a well knwon libray that rely on system var to configure unit testing? Personnaly I cannot. More generally widely used design are often best...

[sylvlecl]

I have an additional issue to bring to the discussion :)
But first, I just want to make clear that I am not defending the system property approach.

The ServiceLoader approach does not integrate very well with dependency injection frameworks. It's hard to inject some "external behaviour" inside services instantiated this way. Typically, if you use powsybl and spring-boot to build an application, you will not be able to inject application components (say datasource) inside the config provider. The example use-case, here, is to have a database-backed platform config.

Do we want to support such a feature ?

[geofjamg]

I have an additional issue to bring to the discussion :)
But first, I just want to make clear that I am not defending the system property approach.

The ServiceLoader approach does not integrate very well with dependency injection frameworks. It's hard to inject some "external behaviour" inside services instantiated this way. Typically, if you use powsybl and spring-boot to build an application, you will not be able to inject application components (say datasource) inside the config provider. The example use-case, here, is to have a database-backed platform config.

Do we want to support such a feature ?

For sure database platform config is a valid use case, but why do you need multiple PlatformConfigProvider for that?
I agree with your concern about dependency injection and service loader but it is a more general topic that only platform config and I really doubt this PR could solve it. I also doubt we could solve it at short/middle term. I would require a deep redesign of the code.

What I really like in this PR is that with a simple developement it could solve several current issues.

• platform config for unit testing. Today it is impossible to test code like MyConfig.load(). With this PR modified like described in my first comment (add a test dependency in each module) it would solve it in a clean way.
• Your use case about database instead file could be done easily to.

Platform configuration is a complex topic and I think that we must be pragmatic and not try to solve all issues / use cases in this PR, otherwise we will just never make any progress on it.

[jonenst]

Do you agree that having a different behaviour in Maven, and IDE is a showstopper? It would be so a mess.

I agree that we should try really hard to avoid this, that's why I have mentioned the problem.

I totally disagree in your first statement about classpath eaysier to get wrong than system variable. Where does it come from?

the classpath is the result of a pretty complex computation (maven dependency mediation) on a pretty big graph (hundreds of deps). The system variable is a manually written string. From my experience, it happens more frequently that people get something bad in their classpath (causing things like slf4j's "Multiple bindings were found on the class path." than that they write the wrong value in their system property. Forcing a system variable is often possible in all kinds of setups (maven cli, IDEs, ...). Forcing the classpath is harder. However, what will be extremely common though is that they will forget to set the system property (like when running the test in the IDE) and we had not taken this into account when we said that it's easier to get wrong, so because of this case I take back what I said: it's easy to forget to set a system variable.

Slf4j is a very sucessfull libray and its design is well knwon to be very good. What is the problem with the warning about multiple impl? If you have this warn, you just have to fix it and change your classpath to have only one impl. Pretty simple, clear and well design.

So should we do that when there are multiple implementations, we just log a warning and use a random one like SLF4J instead of throwing like you proposed initially ?

A general comment, are you able to find a well knwon libray that rely on system var to configure unit testing? Personnaly I cannot.

Doesn't seem crazy to me... setting the xmx for tests, configuring some loggers for tests.. For example: https://github.com/spring-projects/spring-framework/blob/master/import-into-eclipse.md "If attempting to run all JUnit tests from within the IDE, you will likely need to set the following VM options to avoid out of memory errors: -XX:MaxPermSize=2048m -Xmx2048m -XX:MaxHeapSize=2048m"

Log4j2 uses this mechanism: https://logging.apache.org/log4j/2.x/manual/extending.html -DLoggerContextFactory=... and -Dlog4j.configurationFactory

More generally widely used design are often best...

Yes I try as much as I can to do this too.

Using a system property to choose one of the implementations in the class path instead of expecting the classpath to contain only one implementation has the additional benefit to allow to rerun a program with a different PlatformConfigProvider. It could be useful if users report bugs and you want to disable their config, you can tell them to start the program with -Dpowsybl.config.provider=empty ?

In the end, I think there are 2 different aspects to this problem:

• do we allow only one implementation in the classpath ? I think this is too rigid. One good example is that this prevents inheritance between PlatformConfigProvider (@sylvlecl 's argument). We don't have this restrictions for all the other classes that we load through serviceloader.
• If we allow more than one, how do we choose the one we want ? For all other things we load through service loader, we use PlatformConfig (e.g. componentDefaultConfig: LoadFlowFactory: com.rte_france.powsybl.hades2.Hades2Factory). but like "who watches the watcher?" we have the problem of "who configures the config". A system variable is easy to force once you know about it but also easy to forget. A classpath element is not forgotten but if it has the wrong value it's less flexible to change.

What about looking for a system property first, and only if missing look for a hardcoded file in the classpath containing the same thing as the system property ? (Not sure, I can't tell if this is crazy or not)

[geofjamg]

Do you agree that having a different behaviour in Maven, and IDE is a showstopper? It would be so a mess.

I agree that we should try really hard to avoid this, that's why I have mentioned the problem.

We should not "try hard to", we should do it. Most of the developpers are IDE users. I personnally click "run test" on my IDE hundred times a day but on the contrary I very rarely run tests through maven (just one time before commit). In IntelliJ you can run unit test at different level: method, class but also package and each time you run at a new level, a new test runner is created and you have to add variables again. And everybody will most of the time forget to add theses variables.

I totally disagree in your first statement about classpath eaysier to get wrong than system variable. Where does it come from?

the classpath is the result of a pretty complex computation (maven dependency mediation) on a pretty big graph (hundreds of deps). The system variable is a manually written string. From my experience, it happens more frequently that people get something bad in their classpath (causing things like slf4j's "Multiple bindings were found on the class path." than that they write the wrong value in their system property. Forcing a system variable is often possible in all kinds of setups (maven cli, IDEs, ...). Forcing the classpath is harder. However, what will be extremely common though is that they will forget to set the system property (like when running the test in the IDE) and we had not taken this into account when we said that it's easier to get wrong, so because of this case I take back what I said: it's easy to forget to set a system variable.

Classpath could be complex, to manage yes but most of the time it is not. In the case of Slf4j it is not tricky at all because impl are always added in final modules. I never had any trouble to use it or debug bad configuration.

Slf4j is a very sucessfull libray and its design is well knwon to be very good. What is the problem with the warning about multiple impl? If you have this warn, you just have to fix it and change your classpath to have only one impl. Pretty simple, clear and well design.

So should we do that when there are multiple implementations, we just log a warning and use a random one like SLF4J instead of throwing like you proposed initially ?

A general comment, are you able to find a well knwon libray that rely on system var to configure unit testing? Personnaly I cannot.

Doesn't seem crazy to me... setting the xmx for tests, configuring some loggers for tests.. For example: https://github.com/spring-projects/spring-framework/blob/master/import-into-eclipse.md "If attempting to run all JUnit tests from within the IDE, you will likely need to set the following VM options to avoid out of memory errors: -XX:MaxPermSize=2048m -Xmx2048m -XX:MaxHeapSize=2048m"

JVM parameters is a totally different thing, it is not applicative.

Log4j2 uses this mechanism: https://logging.apache.org/log4j/2.x/manual/extending.html -DLoggerContextFactory=... and -Dlog4j.configurationFactory

More generally widely used design are often best...

Yes I try as much as I can to do this too.

Using a system property to choose one of the implementations in the class path instead of expecting the classpath to contain only one implementation has the additional benefit to allow to rerun a program with a different PlatformConfigProvider. It could be useful if users report bugs and you want to disable their config, you can tell them to start the program with -Dpowsybl.config.provider=empty ?

In the end, I think there are 2 different aspects to this problem:

* do we allow only one implementation in the classpath ? I think this is too rigid. One good example is that this prevents inheritance between PlatformConfigProvider (@sylvlecl  's argument). We don't have this restrictions for all the other classes that we load through serviceloader.

Inheritance is a bad example, because in that case you can just create an abstract class not annotated for service loader and inherit from it so that you won't have 2 PlatformConfigProvider in your classpath. I really don't understand why it is too rigid. We don't have to compare it with other service loader usage, it depends of the design you want to have. I you want a singleton (and a singleton is fine for many use cases), there is no problem to enforce a single impl in the classpath.

* If we allow more than one, how do we choose the one we want ? For all other things we load through service loader, we use PlatformConfig (e.g. componentDefaultConfig:  LoadFlowFactory: com.rte_france.powsybl.hades2.Hades2Factory). but like "who watches the watcher?" we have the problem of "who configures the config". A system variable is easy to force once you know about it but also easy to forget. A classpath element is not forgotten but if it has the wrong value it's less flexible to change.

What about looking for a system property first, and only if missing look for a hardcoded file in the classpath containing the same thing as the system property ? (Not sure, I can't tell if this is crazy or not)

Not clear to me how this proposition could solve Maven/IDE issue which is the major issue of the actual design. There is no perfect solution. Even if I have a clear preference on classpath based design because I think there are more modern and widely used, I totally respect other other way to see and solve this kind of problem but one thing is sure for me: we must not have to add manual config to IDE to run a unit test and have the same result that with Maven. It is not acceptable and I personnaly never approved a change that will break this rule. I would just lead me to waste time in my day to day work (and make errors).

[jonenst]

What about looking for a system property first, and only if missing look for a hardcoded file in the classpath containing the same thing as the system property ?

Not clear to me how this proposition could solve Maven/IDE issue

This proposition was that we don't set the system property at all for the tests (the system property is only used by users running programs, if they want. Or the system property could be removed entirely if we don't need the feature). Without a system property, all the info comes from the classpath so tests run the same in maven CLI and the IDE. But instead of coming from the META-INF/services/PlatformConfigProvider file, it comes from another one (we could use META-INF/powsybl-platformconfigprovider or something)

This is basically saying that a module running tests can't easily control what META-INF/services/PlatformConfigProvider files they have in the classpath, but they can more easily control this other file.

I've never seen this idea implemented so it fails the "More generally widely used design are often best..." rule though :)

Additionally, This is a good example of how the classpath can be hard to control:
https://www.slf4j.org/faq.html#excludingJCL

Another example: (from https://www.slf4j.org/codes.html ):

cassandra-all version 0.8.1 declares both log4j and slf4j-log4j12 as compile-time dependencies. Thus, when you include cassandra-all as a dependency in your project, the cassandra-all declaration will cause both slf4j-log4j12.jar and log4j.jar to be pulled in as dependencies. In case you do not wish to use log4j as the the SLF4J backend, you can instruct Maven to exclude these two artifacts

Side note: It's really sad that IDEs correctly import the classpath from maven but incorrectly import system properties.. What a nuisance..

[sylvlecl]

What about keeping the service-based approach, but with an ordering system as proposed by @mathbagu on monday ?

I think it would solve most problems : the problem of enforcing the use of one implementation for tests, and the problem of not being able to inherit from one implementation for another one.

It's simple to implement with a getOrder or getPriority method which could have a default implementation. Value will be low for default implementation, higher for the default test implementation.

It's similar to the solution adopted in spring to order collections of beans with @Order annotation.

However, it would not solve the issue I mentioned above with external application components interoperability. I guess for this we will still need some static variable mechanism.

[sylvlecl]

PS:
We may have common static methods to look up those kind of ordered or named services.
For that purpose, those services could implement general purpose interfaces OrderedService orNamedService.

[jonenst]

What about keeping the service-based approach, but with an ordering system as proposed by @mathbagu on monday ?

What I don't like about this approach is that it can get abused and people start using order=1000 and then order=10000 etc..

[sylvlecl]

It sounds to me like a very theoretical downside to be compared with a very tangible added value :)

What exactly is wrong in your example ? What will be the actual cost of having arbitrary values such as 10000 ?

Note: it would also be possible to provide abstract implementations to let the user decide of the actual order.

[jonenst]

Let's go forward with the classpath solution ?

So, in all powsybl modules that needs to run unit tests accessing platform config (almost all...), we only have to add a dependency on a Maven artifact that contains the in EmptyPlatformConfigProvider.

Is this OK? or can we avoid to duplicate this dependency everywhere ?

If we put the dependency in the top pom (with scope=test), it's included everywhere but the project hosting the class can't use the top pom as its parent or else it would be self referencing in the dependencies.

[jonenst]

Side Note: in a somewhat shocking plot twist, it turns out that intellij by default correctly imports maven's systempropertyvariables and passes it to it's junit runner ( cf https://www.jetbrains.com/help/idea/running-tests.html and I tested manually using idea-IC-192.6262.58 ), so importing a project with a systempropertyvariable defined for tests works out of the box even when clicking on the little green arrow inside intellij..

In eclipse, the testng runner has this feature too ( https://testng.org/doc/eclipse.html ), but not the junit runner apparently :(

Does this change anything for you ? I assume it doesn't.

[jonenst]

@geofjamg Did you have a solution in mind for the classpath based config ? Currently the config API uses Path objects and hence need a proper filesystem in 2 places:

• PlatformConfig.getConfigDir() returns a Path object where modules can look for their files (e.g. import-post-processor.js )
• ModuleConfig.getPathProperty() and its variants where the content of the config file is parsed into a java Path object using the passed in FileSystem object.

I think the long term plan should be to remove filesystems entirely from the config API (breaking change). Did you want to do this now or in another PR ?

An alternative would be to make the classpath based config only usable during tests and in this case we can use jimfs to implement everything.

An even simpler alternative is just to use EmptyPlatformConfigProvider and make the tests work with an empty config: most tests should run with the empty config and can call defaultConfig() but tests requiring specific config must pass it explicitly and can't use defaultConfig()

[jonenst]

Note: Can paths be replaced with (using Files.InputStream newInputStream(Path path, OpenOption... o):

• directly an input stream ? But this opens the file right away.
• a Supplier ? But this uses default OpenOptions.
• a Function<List, inputStream> ?

Or maybe all of them so that the API is more flexible ?

[jonenst]

So I pushed a new implementation called TestPlatformConfigProvider. It uses google reflections https://github.com/ronmamo/reflections and jimfs to provide a platformconfig initiliazed with the values of a folder in src/test/resources/

For now I have not implemented a new system to use this in all the modules so it's with the system properties. (Also there's a hack to use it everywhere automatically that requires jimfs and reflections to be in scope=provided in powsybl-commons but this will be be removed eventually)

[jonenst]

Just pushed a version implementings @geofjamg initial proposal:

• powsybl-commons has the interface PlatformConfigProvider
• powsybl-config-standard has the StandardPlatformConfigProvider
• powsybl-config-test has the TestPlatformConfigProvider

PlatformConfig.defaultConfig() throws if the classpath doesn't contain exactly one serviceloader implementation of PlatformConfigProvider. This has 2 consequences:

• exceptions for users upgrading to this version of powsybl
• impossible to test PlatformConfig.getDefault() in powsybl-commons

To discuss..

[jonenst]

Google reflections doesn't work on java9+ ronmamo/reflections#202

[mathbagu]

I'm not sure the name of this module is ok, mainly the standard word. Maybe we should use simple, classic, impl...?

[mathbagu]

Same remark

[mathbagu]

Dependencies should be sorted alphabetically (groupId+artefactId)

[mathbagu]

Same remark here

[mathbagu]

Fix the groupId (see previous remarks)

[miovd]

It has been renamed as config-classic