PLAT-3546: Validating Config-files #797
Conversation
… 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.
| 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?
| @@ -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.
… it's a TestInjector.
… the bootstrap override sequence. The latter is needed for TestInjector take effect. There still are some doubt about the initial justification of having ConfigFactory in the main method. There was an explanation about LoggingFactory. Not clear if it's still valid.
|
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.