Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge + Ref combination doesn't work #88

Open
RokLenarcic opened this issue Feb 25, 2020 · 8 comments
Open

Merge + Ref combination doesn't work #88

RokLenarcic opened this issue Feb 25, 2020 · 8 comments

Comments

@RokLenarcic
Copy link

Given this edn:

#merge [{:b 1} {:c #ref [:b]}]

You'll get:

WARNING: Unable to resolve "#ref [:b]" at [:form 1 :c]
=> {:b 1, :c nil}

This is because the ref tag seems to operate on some different representation. The following edn renders correctly though:

#merge [{:b 1} {:c #ref [:form 0 :b]}]

results in:

=> {:b 1, :c 1}

Having to name the index of reffed item in the merges decreases the value of the ref a lot and it will introduce breakage when people move items around.

I think this is the same problem as experienced in #85

@SevereOverfl0w
Copy link
Contributor

Thanks for the repro, that's really useful. Just to note, your workaround pokes at something internal which could change/go away at any time.

The problem is one of ordering. Aero attempts to automatically figure out what order to run dependencies in. In this case, your dependency tree looks like so:

[:form 1 :c] => [:form], [:form 0], [:form 0 :b]
[:form] => [:form 1]
[:form 1] => [:form 1 :c]

This essentially forms a circular dependency chain (which Aero gives up on and prints that warning).

This EDN is essentially self-referential (it depends on the merging of itself to complete in order for it to be able to refer to itself). I'm not sure if this is a use-case Aero should support. I can think of a fairly specific solution, but it is not generally applicable.

It requires #merge to be happy to merge something which is not yet fully expanded.

@RokLenarcic
Copy link
Author

RokLenarcic commented Mar 1, 2020

Resolving refs after configuration is merged is a very common use-case.

Usually you have a config.edn which is in source control and it will have content like this:

#merge [
{:port #or [# env PORT 4000]
 :host #profile {:prod "http://mysite.com"
                           :dev #join ["http://localhost:" #ref [:port]]
 :s3-bucker-name #profile {:prod "bucker-name"
                                                 :dev #join ["dev-bucket-" #ref [:dev-name]]}}
 #include "config-dev-overrides.edn"]

Then you have config-dev-overrides.edn which is not in source control. Obviously, if I put :port in that file that's different I would expect not only the :port in the final config to change, but also in all the spots where it's reffed. But in your expected behaviour, if I put a different :port in config-dev-overrides, the :port would change, but :host in dev profile would not have the correct port.

I also cannot, for instance, have a dev name defined in dev overrides and have it apply in the properties in the core config.

Or to put in in simpler, more synthetic, example:

given your idea of how Aero should work and the following config file:

 #merge [{:port 4000 :host #join ["http://localhost:" #ref [:port]]}
         {:port 3000}]

It would yield:

{:port 3000 :host "http://localhost:4000"}

Which I don't think is what people would expect.

@SevereOverfl0w
Copy link
Contributor

I would discourage this kind of config, because it detracts from Aero's aim of being explicit and intentional.

To be explicit, you could instead do this:

{:port #profile {:dev 3000 :prod 4000}
 :host #join ["http://localhost:" #ref [:port]]}

Aero doesn't aim to support override files, so I don't think a change to fix that would make much sense. If there's another use-case which requires this behaviour, it would make sense to revisit then.

@RokLenarcic
Copy link
Author

Ok but each developer on the team has a different s3 bucket name prefix, thus overrides are needed, and simply having a :dev profile doesn't solve the scenario where 99% of the configuration is the same, except for a couple of values that are different for each developer. I guess I can use an ENV variable for those values.

@SevereOverfl0w
Copy link
Contributor

There's also #user for that use case.

@p-himik
Copy link

p-himik commented Nov 18, 2022

It's broken even without self-references within the same #merge:

(aero.core/read-config (char-array "{:v {:y 2}, :a #merge [{:x 1} #ref [:y]]}"))
WARNING: Unable to resolve "#ref [:y]" at [:a :form 1]
=> {:v {:y 2}, :a {:x 1}}

It used to work just fine in Aero 1.1.3. I checked it and the regression was made by e333a30.

@SevereOverfl0w
Copy link
Contributor

@p-himik I think there's a bug in your example, you don't have a top-level #ref [:y]. But you do have a :v which works fine:

user=> (aero.core/read-config (char-array "{:v {:y 2}, :a #merge [{:x 1} #ref [:v]]}"))
{:v {:y 2}, :a {:x 1, :y 2}}

@p-himik
Copy link

p-himik commented Jan 28, 2023

Oh shoot, you're right. 🤦‍♂️ Oversimplified my config beyond being correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants