juxt / aero

A small library for explicit, intentful configuration.
MIT License
747 stars 60 forks source link

#merge #ref fails under some circumstances. #85

Closed mchughs closed 4 years ago

mchughs commented 4 years ago

My convoluted configuration looks something like

{ ...
:bar #profile {:some-profile {:value #or [#ref [:some-path #keyword #env MY_ENV]
                                          "my-backup-value"]}}
:foo #merge [{:w :x :y :z}
             #ref [:bar]]
...}

When I apply aero's reader on this .edn I'll get something like

{ ...
:bar {:value "my-backup-value"}
:foo {:w :x :y :z}
...}

The merge bit fails. However if I alter some other piece of the map such as duplicating a key-value pair such as

{ ...
:bar #profile {:some-profile {:value #or [#ref [:some-path #keyword #env MY_ENV]
                                          "my-backup-value"]}}
:foo #merge [{:w :x :y :z}
             #ref [:bar]]
:foo-again #merge [{:w :x :y :z}
                    #ref [:bar]]
...}

Then it will properly evaluate to

{ ...
:bar {:value "my-backup-value"}
:foo {:w :x :y :z :value "my-backup-value"}
:foo-again {:w :x :y :z :value "my-backup-value"}
...}

This makes me suspect that some evaluation-order issue is responsible. Looking at the implementation of the #merge reader I don't see any piece for checking for incomplete contents so I'm surprised it seems to work so often.

Unfortunately I could not recreate a small example, and even anonymizing the keys and values in my personal config destroy the behavior so I cannot share that either.

SevereOverfl0w commented 4 years ago

Merge is a "function" not a macro, so it isn't supposed to be concerned with incomplete items.

SevereOverfl0w commented 4 years ago

On mobile, bug is in

(defmethod eval-tagged-literal :default

[tl opts env ks]

(let [{:keys [:aero.core/incomplete?] :as expansion}

(expand (:form tl) opts env ks)]

(if incomplete?

(update expansion ::value (rewrap tl))

(update expansion ::value #(reader opts (:tag tl) %)))))

SevereOverfl0w commented 4 years ago

It should probably expand repeatedly in the same way #or does

SevereOverfl0w commented 4 years ago

I am having trouble reproducing this. My suspicion was that the arguments to #merge weren't being expanded recursively first. As far as I can tell they are though. That leaves me a little bit stumped.

SevereOverfl0w commented 4 years ago

Okay, finally giving up. If you redefine aero.alpha.core/expand as:

(def level (atom 0))

(defn expand
  "Expand value x.  Dispatches on whether it's a scalar or collection.  If it's
  a collection it will expand the elements of the collection."
  [x opts env ks]
  (let [lvl (swap! level inc)]
    (print (apply str "|" (repeat lvl \space)) " <= ")
    (pr x)
    (print " ")
    (prn ks)
    (let [res (if (or (and (map? x) (not (record? x))) (set? x) (seq? x) (vector? x))
                (expand-coll x opts env ks)
                (expand-scalar x opts env ks))]
      (print (apply str "|" (repeat lvl \space))  " => ")
      (prn (select-keys res [:aero.core/incomplete? :aero.core/incomplete]))
      (reset! level (dec lvl))
      res)))

It will print out some useful debugging information about the order of things tried, when/if they were marked incomplete, and what went in. tools.trace was producing way too much output because of how big env gets. The code is a little messy.

Hopefully this will provide you the data to figure out a minimal repro

mchughs commented 4 years ago

Awesome. I will get back to you with my findings.

mchughs commented 4 years ago

I think I would like to close the issue because in simplifying down the config.edn this issue has been avoided. I don't think there are many actionable steps for this issue given I couldn't recreate the issue. Thoughts?

SevereOverfl0w commented 4 years ago

Maybe the next person will have a clearer picture of what's going on (or maybe you weren't understanding your complex config properly? :stuck_out_tongue_winking_eye:).

Let's close until someone figures it out.