bug: config 0.14.0 only supports lowercase fields (regression since 0.13.4)

[vladgon]

Failed to deserialize camelcase fields for yaml file
https://github.com/vladgon/rust/blob/49854f6c3d02bf7ae4a8d123ac9ecfbbe3959a56/wspace/wg_util/src/common/config/model.rs#L24

#[derive(Deserialize, Serialize, Debug)]
#[allow(non_snake_case)]
pub struct Kafka {
    pub broker: String,
    pub topic: String,
    pub pollSleep: u64,  //<---
}

https://github.com/vladgon/rust/blob/49854f6c3d02bf7ae4a8d123ac9ecfbbe3959a56/wspace/wg_sample_app/resources/app_config.yaml#L9

kafka:
  broker: localhost:29092
  topic: rust
  pollSleep: 1000

https://github.com/vladgon/rust/blob/49854f6c3d02bf7ae4a8d123ac9ecfbbe3959a56/wspace/wg_util/src/common/config/app_config.rs#L73-L78

[polarathene]

Probably have to compare the crate version differences of dependencies? Unless it was an actual internal change introduced to config-rs (I recall some work was contributed to rework deserializing).

Is this specific to YAML only? Could you try it with a different config file type?

---

If it's specific to YAML, there is plans to change the crate used: #474

I'm hoping to have time next week to tackle that and related PRs.

[vladgon]

Tested with toml file same issue, works with 0.13.4, fails with 0.14.0

Setting Tracing filter config wg_util=debug,server=debug
2024-02-03T17:19:42.381705Z DEBUG ThreadId(01) wg_util::common::config::rust_app: 29: Using config files: "/Users/vova/workspace/rust/wspace/wg_sample_app/resources/app_config.toml"    
thread 'main' panicked at wg_util/src/common/config/app_config.rs:77:82:
missing field `pollSleep`
stack backtrace:

[polarathene]

works with 0.13.4, fails with 0.14.0

Ok, awesome!

So now we know that it's not format specific, it's either due to a dependency update or something that changed in the actual config-rs code. That's a tricky one someone will need to invest time into to track down.

There is a 211 commit difference to wade through, although it's not that bad as I recall this project uses merge commits from PRs rather than squash, so the actual range should be less.

I am aware of an approach called a git bisect that performs builds on different commits until it finds the one where the breakage was introduced. I've not used this before and assume it'll know to only test against the merge commits in that range rather than partial series of commits from a PR. It'd do something like start halfway through that list and if it works properly, then tries the halfway point in that smaller range and so forth.

Alternatively you could look through the commit history manually and see if anything stands out.

---

At a guess if it's failing with the rename, it may fail at other serde attributes that worked previously, which should help narrow it down to commit range between releases for specific files instead 👍

In fact at a glance it could be this: #354

I assume that'd mean your field is already internally treated as pollsleep, hence the breakage.

The project also documents the change history with a changelog thanks to @matthiasbeyer , that should help a little due to the merge approach. Give that a look, there's some other changes that could have been the culprit doing similar transforms.

---

Once that's pinned down, then we'd need to assess the intended fix vs the breakage it introduced and if both can remain fixed or if one has to be preferred 🤷‍♂️

Either way, sounds like a new test case for the project 😅

[polarathene]

I updated the title, it's still based on an assumption but may be more descriptive of what the actual regression is.

[vladgon]

I think you should have a unit test for this case, just wanted to let you know about the issue.

…

--Vladimir

On Feb 3, 2024, at 4:15 PM, Brennan Kinney ***@***.***> wrote: I updated the title, it's still based on an assumption but may be more descriptive of what the actual regression is. — Reply to this email directly, view it on GitHub <#531 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA7BYM5PREJKSYU5KRLXFW3YR2SH5AVCNFSM6AAAAABCXQUUVCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMRVGQ2TOMJSGE>. You are receiving this because you authored the thread.

[jpmckinney]

Yes, I think #354 is the cause. I would also like to be able to read in keys that don't get lowercased.

[max-m]

I noticed this change the hard way as well, my service exploded because case sensitive keys were suddenly converted to lower case.
Part of my parsed config is a hash map with case sensitive keys that must be passed as is to a downstream service.

struct Settings {
  ...
  downstream_config: HashMap<String, serde_json::Value>,
  ...
}

With version 0.14.0 all the keys of that map are now lowercase.
As a workaround I’ve rolled back to 0.13.4 for now.

And now that I think about it, I suppose this was also the issue I had when I tried to add a struct to my config that had #[serde(rename_all = "camelCase")] set and it couldn’t deserialize the config because myKey was apparently not set, even though it was. 🤔

[ada-phillips]

Chiming in to say that this bug impacted my project as well, and was unexpected to say the least. I support PR #543 and think that PR #354 probably wasn't the most appropriate fix to Issue #340. I would think a better solution would be to not lowercase environment variables either? I might take a deeper dive into Issue 340 if I have time this week.

[HactarCE]

I also just spent an hour or two debugging this in Hyperspeedcube

[primeos-work]

Any ideas how to best proceed here or rather how to make a decision whether this library should be case-sensitive (as it was up to version 0.13.4) or case-insensitive (as it is since version 0.14.0, where all keys are converted to lowercase)? (Voting might be nice but difficult - ultimately it's up to the maintainers though - cc @epage and @alerque in case you have some spare time to move this forward / make a decision; there's already #543 btw which was close to being merged)

Basically this isn't a bug but a somewhat under documented "feature" (at least in the changelog!). The changelog for version 0.14.0 doesn't really mention this major breaking change ("#354 Fix uppercase lowercase issues" is quite subtle...) but the README now clearly states that this library is "Is case insensitive and all the keys are converted to lowercase internally" (added right away as part of the "Fix uppercase lowercase isses" PR).

AFAIK/IIRC the change to lowercase (#354) was only intended to fix issues with environment variables (#340). The problem was that environment variable names are converted to lowercase internally and so it was not possible to override configuration keys that contain uppercase characters via env vars. Instead of making env vars case-senstive, it was decided to make config-rs case-insnsitive by always lowercasing all configuration keys (#354 (comment)). This fixed #340 but caused major regressions for downstream since many projects depended on the case-sensitivity.

One internal regression seems to be that Serde renamed fields are no longer supported: #522.

Looking at the links in this issue, some users of this library have downgraded to version 0.13.4:

• Downgrade the config crate to version 0.13 to fix a regression science-computing/butido#353
• bug: config 0.14.0 only supports lowercase fields (regression since 0.13.4) #531 (comment)
• lapce/lapce@80ccc70
• api: GET /files includes dirs, GET metadata TheContrappostoShop/Odyssey#18
• siryancealot-enterprises/rust-react-app-hello-world@d8067ee
• HactarCE/Hyperspeedcube@306e4f5

One project switched to another library:

• pot may fail with complain about "Error: missing field PAW_PBE" Ionizing/rsgrad#11

And one PR has been blocked by this change:

• cli: Add --show-origin option to config list martinvonz/jj#3023 (comment)

[alerque]

As mentioned before the migration, I'd still be willing to jump in a bit here and there to facilitate, but I haven't been granted any permissions on this since the migration.

[epage]

but I haven't been granted any permissions on this since the migration.

what permissions would be needed to do such a thing?

[epage]

Any ideas how to best proceed here or rather how to make a decision whether this library should be case-sensitive (as it was up to version 0.13.4) or case-insensitive (as it is since version 0.14.0, where all keys are converted to lowercase)? (Voting might be nice but difficult - ultimately it's up to the maintainers though

imo we should be case sensitive by default for config files. If we were to support case insensitivity, that should be an opt-in feature somehow (generics?). However, the core problem is dealing with env variables which, on Windows, are case insensitive which we should match.

My questions

• What would a solution look like that restricts case insensitivty to only env variables, maybe even only on Windows?
• Is the problem with env variables sufficient to hold up un-breaking everyone else?