Skip to content

Draft: Refactor top level Python without strict dependency injection

Avielle Wolfe requested to merge create-scan-coordinator-without-strict-DI into master

What does this MR do?

This MR is a version of !248 (closed) with some of the dependency injection removed.

I'm not intending to merge this MR. I want to use it as a demonstration to help us decide whether or not to strictly use dependency injection.

Technical details

  • Stop injecting Python modules logging and uuid
  • Stop injecting System
  • In 3014c23e, provide an example of not injecting our own classes, and of only providing low level classes with the configuration options they need

What I learned

  1. It feels wrong to me to inject Python's own modules, for the following reasons:
    a. We're already only injecting them sometimes
    b. They are used at every level of the application, so injecting them from the top level (like we were with logging) adds a lot of boilerplate
    c. We can trust that they will not be modified while the application is running
    d. Even typing them is weird. For example, logging is of type Module. It feels odd (unnecessarily meta) to be passing a builtin Python module through our classes
    e. It could be argued that some, like logging, might be replaced at some point by a custom or third party library. Even if that is true, injecting it feels like pre-optimization. Moreover, we might want to use a different method for implementing a custom logger (like using a singleton).
  2. It feels wrong to me to inject custom classes that we use at every level of the application and that we can trust not to be modified while the application is running, like System, for the same reasons I've written out for Python's own modules (except the typing one)
  3. It feels nice to inject dependencies that we want to restrict to the top level, like Configuration and ZAProxy. For example, I'd rather pass only the specific configuration options needed to lower level classes to prevent them from having knowledge of unnecessary options. Having to inject the configuration explicitly serves as a reminder to me to instead inject only the specific options that are needed
  4. I generally like not injecting our own classes without an explicit reason to do so. It makes the class hierarchy clearer to me and the code looks cleaner too
    a. 3014c23e illustrates what the code would look like in a case where we don't inject the dependencies

What are the relevant issue numbers?

gitlab-org/gitlab#227749 (closed)

Does this MR meet the acceptance criteria?

Edited by Avielle Wolfe

Merge request reports