search

mehcode / config-rs

⚙️ Layered configuration system for Rust applications (with strong support for 12-factor applications).
Apache License 2.0
2.43k stars 206 forks source link

refactor: Use a common `from_value` method #472

Open polarathene opened 8 months ago

polarathene commented 8 months ago

Inspired from the json5 feature (Original PR July 2020, revised PR May 2021) with the refactoring in the revision by @matthiasbeyer

It looked useful for other formats that use serde to simplify their logic (so long as test coverage is informative of no issues 🙏 )

DRY: The json, json5 and toml parsers all leverage serde and can share a common enum to deserialize data into, instead of individual methods performing roughly the same transformations.

from_parsed_value() is based on the approached used by format/json5.rs:

Resolves: https://github.com/mehcode/config-rs/pull/394

polarathene commented 8 months ago

I was able to resolve the datetime error by including the toml type for it in the ParsedValue enum, but since that'd interfere with the generic method and the feature conditional compilation, I guess I should revert the toml.rs change.

It's Datetime is not compatible with chrono DateTime type unfortunately, at least serde doesn't recognize it for that.

polarathene commented 8 months ago

This PR has been updated to also include support for formats Ron and Yaml.

Ron:

Yaml:

During the integration of Ron and Yaml into this PR (like with earlier TOML test for datetime), both failed running their tests due to the more implicit deserialization_any used by Serde to support deserializing for the untagged enum.

Resolving requires either:

It's not too different from the prior approach, these types were in the formats own from_value() method, whereas this approach better identifies when this is actually necessary. They still leverage feature toggles for conditional compilation.

For TOML with the Datetime type that was easier to resolve thanks to the specific test failure. Ron and YAML failures were more cryptic usually, such as:

running 7 tests
test test_error_parse ... ok
test test_override_lowercase_value_for_enums ... ok
test test_override_uppercase_value_for_enums ... ok
test test_override_lowercase_value_for_struct ... ok
test test_override_uppercase_value_for_struct ... ok
test test_file ... ok

thread 'test_yaml_parsing_key' has overflowed its stack
fatal runtime error: stack overflow
error: test failed, to rerun pass `--test file_yaml`

Which IIRC I had read was due to how deserialize_any works with the untagged enum, that is a situation of it recursively trying to resolve the enum incorrectly due to reaching a variant that references Self and performing deserialize_any on that and so forth.

Note Ron 0.9 will introduce a Bytes value variant (Vec<u8>) for byte strings.

  • I don't believe the current approach would match it? You'd get the fallback Nil.
  • Probably acceptable given it's unclear how config-rs would want to support that.
polarathene commented 8 months ago

@matthiasbeyer this is ready for review, I know you're quite busy so here's a TLDR:

polarathene commented 8 months ago

For custom format support, the format method added here, one I previously contributed (extract_root_table()) and sometimes ParsedValue are required to leverage it (_to call ParsedValue::deserialize(input_value)_). This would require lib.rs to make the module pub, or some refactoring.

As it's all focused on using Serde with untagged enum, perhaps it shouldn't live in format.rs if it isn't useful to other formats (not that I expect those to be contributed much).

I also need to verify an assumption that other number types like u8 would correctly be cast to u64 supported by config-rs. Prior to this the mapping was more explicit, and I may have a misunderstanding with the untagged enum approach using deserialize_any() this way 😅

I will also share the other formats I integrated locally, if desired they can be contributed via follow-up PR.

matthiasbeyer commented 8 months ago

So from what I read, we want this in. @polarathene I'd like this PR to be rebased to latest master, so we get a CI run with latest master. After that, I think you can merge this!

polarathene commented 8 months ago

After that, I think you can merge this!

I still need to address my concerns from my prior comment here. I will return to config-rs later today or tomorrow hopefully, presently tied up elsewhere for a bit.

After that's sorted locally, I'll rebase as requested and it'd be great to see this get merged :)

Regarding custom format support, how would you approach verifying that? Mostly so that you can catch that format module should be made public, incase someone were to revert that accidentally?

polarathene commented 3 months ago

Note to self, since this PR was raised the proposed yaml alternative (serde compatible) has been archived and a different yaml crate has been introduced instead, so this PR will need to drop that support (unfortunate, since the serde approach was preferrable I think).