Replacing applied middleware in dev.edn causes confusion and duplication

[dadair-ca]

Because dev.edn uses ^:replace for applied middleware, there is a duplication of keys for middleware you want active in both production and development, as ^:replace replaces the entire :applied vector.

Issues

1. Developers may be confused as to why adding new middleware in system.edn does not have an effect when developing from the repl using (reset)/etc.
2. Full-replace requires duplicating keys in both dev.edn and system.edn, which can lead to synchronization bugs/annoyances.

   Solutions

3. I attempted to alleviate this issue by contributing a note to the configuration wiki
4. Perhaps dev.edn should black-list, rather than replace. Maybe ^:except? Example:

system.edn:

{:components
 {:app  duct.component.handler/handler-component
  :http ring.component.jetty/jetty-server}
 :endpoints
 {:example foo.endpoint.example/example-endpoint}
 :dependencies
 {:http [:app]
  :app  [:example]}
 :config
 {:http {:port http-port}}
  :app  {:middleware
         {:functions {:not-found   duct.middleware.not-found/wrap-not-found
                      :hide-errors duct.middleware.errors/wrap-hide-errors}
          :arguments {:not-found   "Resource Not Found"
                      :hide-errors "Internal Error"}
          :applied   [:not-found :hide-errors]}}}

dev.edn:

{:config {:http {:port 3000}
          :app  {:applied ^:except [:hide-errors]}}}

However, this would not allow dev.edn to add development-only middleware, so maybe there should be both ^:except and perhaps ^:concat or ^:append?

[weavejester]

I see why this could be confusing. Thanks adding a note to the README.

Duct uses meta-merge, which is effectively copied right out of Leiningen. So it currently supports metadata for :replace, :displace and :prepend, with :append being the default.

The problem with :except is that there may be situation where we want to both exclude production middleware, and append development middleware, and with metadata we only get the option to do one.

I see three ways forward:

We could have an additional key that blacklists middleware, like :denied. So anything in :applied that is :denied is ignored.

We could change the middleware list entirely, so that instead of an ordered list, we instead use dependencies. e.g. wrap-cookies must come before wrap-session. So something like:

{:middleware
 {:functions {:wrap-params ring.middleware.params/wrap-params
              :not-found   duct.middleware.not-found/wrap-not-found
              :hide-errors duct.middleware.errors/wrap-hide-errors}
  :arguments {:not-found   "Resource Not Found"
              :hide-errors "Internal Error"}
  :ordering  {:hide-errors {:after :all}
              :wrap-params {:before :not-found}}

Or we could treat the problem as a documentation issue and keep the current system. We do have the problem of duplication, but the advantage is that the order of middleware is always explicit.

[dadair-ca]

I had not considered the ordering issue of middleware, I agree with your conclusion that it is better to have some duplication with explicit lists rather than have potential ordering issues using a mixture of ^:except and ^:append.

I also agree that we can treat this as a documentation issue, but I have another possible solution.

We could make the ^:replace both transparent and loud (but configurable), something like having a :warn-on-replace {:enabled true :display-help true} available in system.edn. While true, the underlying duct utils would produce a diff-like log statement on system start/reset, like in clojure.test when two vecs are different:

[TIMESTAMP] Replaced middleware!
[TIMESTAMP] before: [..]
[TIMESTAMP] after: [..]
[TIMESTAMP] you may turn-off these notifications in `system.edn` using the `:warn-on-replace` key.

Obviously the UX could be improved, but just an example. And this could be thought of mixing duct configuration with system configuration, so that may not be desirable.

This would be enabled by default, and would prevent any confusion for new developers since the replacement would be announced. Once this has been learned, having it configurable in system.edn would let established developers remove this warning.

I'm not sure if it's better to have the initial state of this configuration in system.edn or dev.edn.

While I'm thinking about it, this could be subset to a larger solution: could duct produce a diff of the final system configuration on (start)/(reset)? This would show any difference between the initial system.edn and the final dev.edn in development, and would allow for transparent system configuration.

[weavejester]

Those are some good ideas. We could make meta-merge more verbose, but I like the idea of having a diff as that's independent of meta-merge, and provides more information. I think we need to strike a balance between giving enough information to be useful, and being too verbose on startup.

[dadair-ca]

What if the log was minimal, since users can inspect the system in the REPL with (clojure.pprint/pprint system)?

So the log could be (example):

[TIMESTAMP] System override from `dev.edn`. Modified keys: [[:components :config :app :middleware :applied] ..].
[TIMESTAMP] You may inspect the final system with (clojure.pprint/pprint system)

Basically, it would produce a vec of paths (represented as vecs) to system keys that were changed from the meta-merge. The output can be enabled/disabled/configured by providing some configuration to load-system?

[weavejester]

Possibly... Maybe I need to think about some logging system for Duct, now that we're talking about adding more verbose output. Though I haven't run across any I've particularly liked yet.

[weavejester]

Not really relevant in 0.9.0.