PLAT-3546: Validating Config-files #797
Conversation
| m.preBind(); | ||
| } | ||
|
|
||
| mergedModule = Modules.override(mergedModule) |
There was a problem hiding this comment.
checked if the test injector uses this feature to override modules?
There was a problem hiding this comment.
TestInjector should indeed override the modules. I made a special case for it.
| @Override | ||
| public Integer getPrio() { | ||
| // We want this module to override the default binding of ConfigFactory | ||
| return 110; |
There was a problem hiding this comment.
Is there any way of creating a test to validate that this actually works, and more importantly continues to work in the future as well?
There was a problem hiding this comment.
I added a test that compares priorities between this module and the config module.
| @@ -28,12 +28,12 @@ void shouldBindImplementationsToTheirInterfaces() { | |||
| } | |||
|
|
|||
| @Test | |||
| void shouldUseBootStrapBindingOverDefault() { | |||
There was a problem hiding this comment.
This seems to be a very conscious choice, did you find any hints on why it was this way? It could help prevent reintroducing bugs that this might have resolved.
There was a problem hiding this comment.
No, I don't have any clue why it's done this way. The bootstrap module binds only two things:
bind(String[].class).annotatedWith(Names.named("args")).toInstance(args);
bind(ConfigFactory.class).toInstance(configFactory);So it's either the default ConfigFactory class or whatever ConfigFactory compatible class defined in the override modules. I could not find hints why the default factory should always override.
There was a problem hiding this comment.
Maybe this order of overrides was made just for a single purpose: to promote the TestInjector in DbProxyTest. When this case is fixed, both tests and applications work fine.
… default config factory because of the bootstrap binding. Validation of the configs did not work. That is fixed by changing the order of the modules binding in relation to the bootstrap binding.
nolexa
force-pushed
the
PLAT-3464-validation-of-config-classes
branch
from
f6b0eaa
to
d0e916d
Compare
| * <p> | ||
| * As logging can be part of the configuration file and the configuration file | ||
| * could be missing, we have a side effect of setting up and initializing a LoggingFactory | ||
| * that doesn't depend on ConfigFactory, if the configuration file is missing. |
There was a problem hiding this comment.
It is not clear if this justification is still vald.
|
Validation of the config files seems to be broken for a long time.
ValidatingConfigFactorydid not have effect because its binding was always overridden by the default config factory from the bootstrap binding.For some reason bootstrap binding was giving the highest priority, thus overriding the bindings of all the auto-bind modules. I could not find any justification for this behaviour. The only class that is bound in the bootstrap module is
ConfigFactory, and we want it to be overridden byValidatingConfigFactory. It looks like it was a bug.The modules precedence is fixed so that the bootstrap is at the lowest place, overridden by the rest of the modules.