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.