Graylog2 / Graylog2/graylog2-server
Guice bindings need refactoring
- Dominant language
- Java
- Stars
- 8.1k
- Forks
- 1.1k
- Avg merge
- 1d 20h
- Merged PRs (30d)
- 217
Description
Graylog's guice binding setup is pretty crufty and parts of the code is among the oldest surviving code in the project.
Specifically there are multiple problems with the current state that make writing new code, CLI tools and different implementations of subsystems (e.g. for unit/integration tests) more difficult than it should be.
Among them are in no particular order:
* Code and binding structure still reflects some of the Graylog radio/server split, which simply doesn't exist anymore ("Shared" bindings and packages)
* Super-modules bind a lot of different and otherwise unrelated subsystems at the same time.
* [`ServerBindings`](https://github.com/Graylog2/graylog2-server/blob/master/graylog2-server/src/main/java/org/graylog2/bindings/ServerBindings.java) and others are structured by "binding type" not subsystem, which makes it really difficult to create only what you need.
* There are providers that are plainly superfluous ([`BundleImporter and friends`](https://github.com/Graylog2/graylog2-server/blob/master/graylog2-server/src/main/java/org/graylog2/bindings/providers/BundleExporterProvider.java#L34)) or do work manually that should really be done via direct bindings ([Periodicals](https://github.com/Graylog2/graylog2-server/blob/master/graylog2-server/src/main/java/org/graylog2/shared/bindings/SchedulerBindings.java#L57))
* We can't seem to decide how to inject configuration, either by name or by config object. This sometimes leads to injecting all kinds of stuff that we actually don't need. Due to the ill-defined boundaries, that sometimes means we have to bind and inject whole other subsystems, too, that are actually unrelated.
* Multibinders and specific elements of them are mixed, which makes it difficult to prevent certains things to start inadvertently ([Periodicals](https://github.com/Graylog2/graylog2-server/blob/master/graylog2-server/src/main/java/org/graylog2/bindings/PeriodicalBindings.java#L48) and more). The Multibinders should be bound together with their usage sites and the specific elements in them should be bound in the subsystem they belong to (e.g. AlertScanner should be bound as part of the alerting subsystem, not the `PeriodicalsBinding`).
* There should be no I/O in constructors ever. (`NodeId` is bad, but network connections are a no-no as well).
* We should clean up binding annotations vs manual scoping in modules in favor of only doing it in the modules. That makes finding and understanding those declarations much easier (this more or less applies to `@Singleton` and `@ImplementedBy`). The more explicit it is, the easier it is to debug and follow.
* Modules should be small and targeted. There will be domain-specific dependencies (e.g. the core components of the server require a lot of individual services to function) but if we have dedicated modules for them, it's easier to fulfill and possibly override them if you want a different implementation (e.g. in tests or CLI tools). With large encompassing modules this is next to impossible to customize. In doubt make smaller modules and combine them with other modules (by calling `binder.install(otherModule)`.
* Ideally modules are next to the subsystem that they bind to avoid having to hunt for the correct module. This might require moving packages, which can be problematic because of external identifiers in databases and metrics, so tread carefully here.
* Only inject direct dependencies and not transient ones, which means that if you only inject something for the reason to manually construct another instance of something, consider making a provider for the class you require.
* Don't use `@Named` annotations to distinguish two types of objects, but create actual qualifier binding annotations for them, because the strings are so easy to mistype and so difficult to search for (e.g. our [`schedulers`](https://github.com/Graylog2/graylog2-server/blob/master/graylog2-server/src/main/java/org/graylog2/shared/bindings/SchedulerBindings.java#L56) follow the wrong pattern)
* Guice can automatically inject `Provider` instances for bindings, don't create providers that don't do anything special.
* Avoid ad-hoc bindings in favor of explicit bindings (we should at some point think about requiring explicit bindings to avoid accidentally creating objects implicitly).
Contributor guide
Assessment
This issue has not been assessed yet.