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() with a leading join() doesn't overwrite #128

Open
jaads opened this issue Sep 25, 2024 · 5 comments
Open

merge() with a leading join() doesn't overwrite #128

jaads opened this issue Sep 25, 2024 · 5 comments

Comments

@jaads
Copy link

jaads commented Sep 25, 2024

I'm trying to load configuration variables from various sources using figment. The module looks like this:

#[derive(Deserialize)]
pub struct Config {
    pub port: u16,
    pub host: String,
}

pub fn load_config() -> Config {
    let file_name = "config";

    Figment::new()
        .join(("port", 8080))
        .join(("host", "localhost"))
        .merge(Env::raw())
        .extract()
        .expect("Fail to load config")
}

When I'm tried to test the load_condig function, it seems that the environment variables don't get loaded. Here is my test module

#[cfg(test)]
mod tests {
    use super::*;
    use std::env;

    #[test]
    fn loads_defaults() {
        let res = load_config();
        assert_eq!(res.host, "localhost");
        assert_eq!(res.port, 8080);
    }

    #[test]
    fn environment_variables_override_defaults() {
        env::set_var("port", "8888");
        let res = load_config();
        assert_eq!(res.port, 8888);
    }
}

The first test run successfully. But the second test fails - the port stays unchanged although I expected it to be overwritten in the figment with 8888.

What I am doing wrong? It seems that the env vars don't get loaded or not overwritten correctly. I also tested with Jail as follows, but with the same result with the port variables don't get overwritten.

#[test]
  fn environment_variables_override_defaults_in_jail() {
      figment::Jail::expect_with(|jail| {
          jail.set_env("port", 8888);
          let res = load_config();
          assert_eq!(res.port, 8888);
          Ok(())
      });
  }
@jaads jaads changed the title Figment doesn't load env variables merge() doesn't overwrite config values Sep 26, 2024
@jaads
Copy link
Author

jaads commented Sep 26, 2024

I also wanted to load configuration variables from a file which should overwrite the defaults. But this doesn't seem to work either, although I basically used this example from the docs.

so I think the merge function does not work as expected.

here is a test which is failing:

  #[test]
    fn file_overwrites_defaults_with_jail() {

  #[derive(Debug, Deserialize, PartialEq)]
        pub struct Config {
            pub port: u16,
        }

        figment::Jail::expect_with(|jail| {
            jail.create_file("config.toml", r#"
        port = 9999
    "#)?;

            let config: Config = Figment::new()
                .join(("port", 8080))
                .merge(Toml::file("config.toml"))
                .extract().expect("extract result");

            assert_eq!(config, Config {
                port: 9999,
            });

            Ok(())
        });
    }

why is the port not overwritten by merge() with the value 9999?

@jaads jaads changed the title merge() doesn't overwrite config values merge() doesn't overwrite Sep 26, 2024
@jaads jaads changed the title merge() doesn't overwrite merge() doesn't overwrite, with a leading join() Sep 26, 2024
@jaads jaads changed the title merge() doesn't overwrite, with a leading join() merge() with a leading join() doesn't overwrite Sep 26, 2024
@LDSpits
Copy link

LDSpits commented Oct 18, 2024

Hello! I was looking at this issue as I seem to have the same problem. But I made an interesting observation when working with @jaads example test.

At first glance, the issue seems to be in the merge/join logic (from looking at the code, the term coalescence is used internally). But if you replace the Toml::file(...) call. The issue seems to go away.

#[test]
fn file_overwrites_defaults_with_jail() {

    #[derive(Debug, Deserialize, PartialEq)]
    pub struct Config {
        pub port: u16,
    }

    figment::Jail::expect_with(|jail| {

        let config: Config = Figment::new()
            .join(("port", 8080))
            // Note the inlining of the config file vs `Toml::file("config.toml")` before
            .merge(("port", 9999)) 
            .extract().expect("extract result");

        // Test passes!
        assert_eq!(config, Config {
            port: 9999,
        });

        Ok(())
    });
}

This suggests that there is a problem in the interaction between Data::file (the internal impl of Toml::file) and coalescing. I'll be taking a look later to see if I can find the actual issue there.

@LDSpits
Copy link

LDSpits commented Nov 1, 2024

I have found the issue with the example test case that you provided @jaads . At first glance it seems to be an issue with the join & merge operators. But it actually boils down to interaction between profiles.

a good way to visualise this is using the .data() method available on anything that implements Provider. This gives the first clue to what actually is going on.

If we visualise the data shape of ("port", 8080)

#[test]
fn visualise_tuple() {
    let config = ("port", 8080);
    let shape = dbg!(&config.data());
}

We see the following (cleaned up for readability):

&config.data() = Ok(
    {
        Profile(string: "global"): {
            "port": Num(8080),
        },
    },
)

Note the Profile(string: "global"). This means that the data is assigned to the global profile. Which influences merging & joining. If we visualise the result of Toml::File (using Toml::string() so we don't actually need a file).

#[test]
fn visualise_toml_data() {
    let toml = Data::<Toml>::string("port = 9999");
    let shape = dbg!(&toml.data());
}

we see (again, cleaned up for readability):

&toml.data() = Ok(
    {
        Profile(string: "default"): {
            "port": Num(9999),
        },
    },
)

Where you can see that the value is under a different profile. If you use the toml & tuple file and try to combine them inside a Figment, you don't get what you expect.

What actually happens inside the test case then?

first, an empty Figment is constructed (let config: Config = Figment::new()). The internal .data() looks like this

{
   // No data here
}

The first set of port information is joined (.join(("port", 8080))). The data inside figment looks like this.

{
   // Data is joined against an empty dictionary, so the port is set
   Profile(string: "global"): {
       "port": Num(8080),
   },
}

Then we merge the toml file .merge(Toml::file("config.toml"))

{
   // Original data was not touched because it has the `global` profile.
   Profile(string: "global"): {
       "port": Num(8080),
   },
   // Because the data was merged with a different profile (`default`), the data was not combined
   Profile(string: "default"): {
       "port": Num(9999),
   },
}

So the data never actually got combined! But how do we get 8080 as extracted value then? That's the next part of this rabbit hole...

Default combining behaviour of Figment

If we take a look at Figment::extract() we see that it first retrieves a the current value of the Figment using the merged() method. What does that do? We can get a clue from the profiles section in the docs

There are two built-in profiles: the aforementioned default profile and the Global profile. As the name implies, the default profile contains default values for all profiles. The global profile also contains values that correspond to all profiles, but those values supersede values of any other profile except the global profile, even when another source is merged.

merged() implements this behaviour. The internal steps basically boil down to this:

  • Is the currently selected profile default?
    • Take the value of the current (the default) profile
    • Merge global into it.

So when deserializing your port value:

  • take 9999
  • Merge 8080 on top.

The end result becomes 8080!

Original code example

The bug becomes easy to see once you know all this:

#[derive(Deserialize)]
pub struct Config {
    pub port: u16,
    pub host: String,
}

pub fn load_config() -> Config {
    let file_name = "config";

    Figment::new()
        .join(("port", 8080))        // Loaded in global profile
        .join(("host", "localhost")) // Loaded in global profile
        .merge(Env::raw())           // Loaded in default profile
        .extract()                   // Values were not actually combined at this point. Combined using the `default combining behaviour` 
        .expect("Fail to load config") // Values were "not joined" at this point (correct, they were merged)
}

Hope this helps :)

@jaads
Copy link
Author

jaads commented Nov 2, 2024

nice thanks for the update 👍

@jaads
Copy link
Author

jaads commented Nov 2, 2024

btw, I overcame this issue by using this approach for declaring default values for host and port.

the issue still exists though I'd say

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

2 participants