Support jsonc format #457

up9cloud · 2023-09-19T17:53:32Z

I did a new commit, see #452

matthiasbeyer

I have some remarks. Also, the two commits could be squashed into one commit,... as well as further fixups of course!

Either way thanks for your contribution!

matthiasbeyer · 2023-09-20T06:30:47Z

tests/file_jsonc.rs

@@ -52,7 +52,7 @@ fn test_file() {
    assert_eq!(s.place.telephone, None);
    assert_eq!(s.elements.len(), 10);
    assert_eq!(s.elements[3], "4".to_string());
-    if cfg!(feature = "preserve_order") {
+    if cfg!(feature = "TODO: preserve_order") {


well, that's because jsonc-parser doesn't support preserve_order. need time to contribute that

matthiasbeyer · 2023-09-20T06:34:32Z

src/file/format/mod.rs

+        #[cfg(all(feature = "jsonc", feature = "json"))]
+        formats.insert(FileFormat::Jsonc, vec!["jsonc"]);
+
+        #[cfg(all(feature = "jsonc", not(feature = "json")))]
+        formats.insert(FileFormat::Jsonc, vec!["jsonc", "json"]);


Do I understand correctly that the jsonc parser also parses json even if that feature is disabled?

I think that's counter-intuitive and we should only parse jsonc with the jsonc parser.

well, there are 2 reasons i added that.

the file extension for jsonc is actually .json, not .jsonc, so the real question is that when i enable the feature jsonc it's about the format or the file extension?

for the common usage, people would choose only one of them, not both. and I believe the jsonc format is more common for config files than the json format.

Cargo.toml

src/file/format/jsonc.rs

tests/file_jsonc.rs

tests/testsuite/file_jsonc.rs

coveralls · 2025-08-10T22:35:36Z

Pull Request Test Coverage Report for Build 16902150946

Details

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.07%) to 65.345%

Totals
Change from base Build 16806457660:	0.07%
Covered Lines:	956
Relevant Lines:	1463

💛 - Coveralls

tests/testsuite/file_jsonc.rs

up9cloud · 2025-08-11T00:29:40Z

Well, I merged the upstream and fixed the test cases.
However, I believe forcing lowercase mapping is an absolutely broken concept, and I don’t understand why after two years, this case’s behavior has changed to: https://github.com/up9cloud/config-rs/blob/master/tests/testsuite/file_jsonc.rs#L169.

I’m certain that when I submitted the pull request back then, all test cases passed.

This behavior should follow one clear approach, rather than just forcing lowercase:

Flexible: env.BAR, env.Bar, and env.bar should all map to enum Settings { Bar(String) }.
Strict: env.BAR should map only to BAR, env.bar should only map to bar, etc

polarathene · 2025-08-11T04:30:08Z

I’m certain that when I submitted the pull request back then, all test cases passed.

The feedback was back in 2023. You later pushed some commits in Feb 2024 and the PR was silent until recently. It's not a surprise that tests could fail given a new major zero ver release was made which permits breaking changes.

Feb 2024 - v14.0 introduced case insensitivity as a fix.
Dec 2024 - v15.0 reverted the case-insensitivity, there should be a related issue (or more) on the topic.

Regarding feedback with the jsonc feature concern for the .json extension, if it doesn't make sense to have other format features handle .json when jsonc is enabled, perhaps this could be detected as a conflict via build.rs? (unless Cargo.toml can express this, doesn't seem so)

I have seen crates like cudarc handle it in build.rs, so something like this?:

#[cfg(all(feature = "json", feature = "jsonc"))]
panic!("Both `json` and `jsonc` features are active, only one feature for parsing `.json` files is permitted");

Regarding src/file/format/jsonc.rs, what are your thoughts about this proposed common method for serde supported formats? As the jsonc-parser crate offers a parse_to_serde_value() method that returns a serde_json::Value, it should simplify the file in this PR to just:

use std::error::Error;

use crate::format;
use crate::map::Map;
use crate::value::Value;

pub fn parse(
    uri: Option<&String>,
    text: &str,
) -> Result<Map<String, Value>, Box<dyn Error + Send + Sync>> {
    // Parse a JSON/JSONC input from the provided text
    let value = format::from_parsed_value(uri, jsonc_parser::parse_to_serde_value(text)?);
    format::extract_root_table(uri, value)
}

extract_root_table() is already merged so formats could skip this portion of common logic:

config-rs/src/format.rs

Lines 27 to 46 in 07f13ff

    
           // Have a proper error fire if the root of a file is ever not a Table 
        
           pub(crate) fn extract_root_table( 
        
               uri: Option<&String>, 
        
               value: Value, 
        
           ) -> Result<Map<String, Value>, Box<dyn Error + Send + Sync>> { 
        
               match value.kind { 
        
                   ValueKind::Table(map) => Ok(map), 
        
                   ValueKind::Nil => Err(Unexpected::Unit), 
        
                   ValueKind::Array(_value) => Err(Unexpected::Seq), 
        
                   ValueKind::Boolean(value) => Err(Unexpected::Bool(value)), 
        
                   ValueKind::I64(value) => Err(Unexpected::I64(value)), 
        
                   ValueKind::I128(value) => Err(Unexpected::I128(value)), 
        
                   ValueKind::U64(value) => Err(Unexpected::U64(value)), 
        
                   ValueKind::U128(value) => Err(Unexpected::U128(value)), 
        
                   ValueKind::Float(value) => Err(Unexpected::Float(value)), 
        
                   ValueKind::String(value) => Err(Unexpected::Str(value)), 
        
               } 
        
               .map_err(|err| ConfigError::invalid_root(uri, err)) 
        
               .map_err(|err| Box::new(err) as Box<dyn Error + Send + Sync>) 
        
           }

So that should already simplify your parse() method for jsonc, but as my PR introducing the from_parsed_value() method has been recently questioned if it's worth merging with it's current approach, this PR would presently need to keep your from_jsonc_value() method.

The following should work for now at least:

-    let value = format::from_parsed_value(uri, jsonc_parser::parse_to_serde_value(text)?);
+    let value = from_jsonc_value(uri, jsonc_parser::parse_to_value(text, &Default::default())?);
    format::extract_root_table(uri, value)

Should #472 get merged, then from_jsonc_value() can also be dropped in favor of using format::from_parsed_value().

EDIT: Oh just noticed that the return type of the result is the value wrapped in an option, which unlike serde-json will result in "" returning None rather than an empty string. Going with the fallback you had instead:

-    let value = format::from_parsed_value(uri, jsonc_parser::parse_to_serde_value(text)?);
+    let parsed = jsonc_parser::parse_to_value(text, &Default::default())?;
+    let value = from_jsonc_value(uri, parsed.unwrap_or(ValueKind::Nil));
    format::extract_root_table(uri, value)

I'll apply that as a review suggestion 😅

src/file/format/jsonc.rs

Cargo.toml

epage · 2025-08-11T17:16:47Z

Please rework your commits as meaningful, atomic commits to be merged.

src/file/format/jsonc.rs

tests/testsuite/file_ini.rs

tests/testsuite/file_toml.rs

tests/testsuite/file_yaml.rs

up9cloud · 2025-08-12T08:37:25Z

@epage, @polarathene all green now :D.

The .json vs .jsonc issue comes down to the project’s philosophy: should it aim to be flexible or strict?

If goes for flexible, the current behavior makes sense, because in practice, almost no one actually uses the .jsonc extension. People generally just want .json files that can contain comments.
If goes for strict, then only .jsonc should be allowed. Later, could consider adding a feature to configure which file extensions are parsed by which parser.

For example, from the earlier point about case insensitive handling and forcing lowercase mapping, these are closely tied to the overall design philosophy. Under a strict approach, case insensitive handling should be an optional feature rather than the default, and forcing lowercase mapping should be avoided - upper should map to upper, and lower should be lower.

epage · 2025-08-12T15:41:42Z

From https://jsonc.org/

The recommended file extension for JSONC documents is .jsonc.

The extension .json should be avoided, but is supported if a mode line is present at the start of the file:

imo comments inside of json is no longer json. We should be properly validating the json we parse.

Cargo.toml

epage · 2025-08-12T15:46:08Z

tests/testsuite/file_ini.rs

+
+#[test]
+fn test_nothing() {
+    let res = Config::builder()
+        .add_source(File::from_str("", FileFormat::Ini))
+        .build();
+    assert!(res.is_ok());
+    let c = res.unwrap();
+    assert_data_eq!(
+        format!("{:?}", c),
+        str!("Config { defaults: {}, overrides: {}, sources: [], cache: Value { origin: None, kind: Table({}) } }")
+    );
+}


Why are these test_nothings added? if they are indirectly related (you wanted a test_nothing for jsonc), then please add these in their own commit before this one

They might have added due to an observation I also noticed with how jsonc-parser and serde-json differ in parsing an empty string (Ok(None) vs Error(E)).

So this might be a sanity check that on the config-rs side we're aware of that parsing difference? Especially for the .json extension since this PR adopts that extension in addition to the serde-json format parser.

I had previously suggested considering a build.rs feature conflict check, so that json vs jsonc are mutually exclusive? (EDIT: actually may not be necessary since a format hint can be provided to control selection, or similar methods that take an explicit format arg)

@polarathene is correct. since parsing results in None and config-rs does not have a corresponding type for that, these tests were added because .jsonc needs this case. The goal was to decide whether None should be treated as an error or ignored, and to compare behavior with other parsers.
That said, your point makes sense. I’ll remove the tests for the other parsers and keep only the .jsonc one.

For existing formats can you add the tests in a separate commit before jsonc support as mentioned earlier in this thread?

I’d like to, but preferably after this current pull request is resolved. Since it’s been almost two years of inactivity on this thread, focusing on closing this one first makes the process clearer and prevents scope creep. Afterward, I can gradually add more test cases, including for the existing formats.

tests/testsuite/file_ini.rs

epage · 2025-08-12T15:48:10Z

tests/testsuite/file_jsonc.rs

+    assert!(res.is_err());
+    let err = res.unwrap_err().to_string();
+    let expected_prefix =
+        "Expected colon after the string or word in object property on line 4 column 1 in ";
+    assert!(
+        err.starts_with(expected_prefix),
+        "Error message does not start with expected prefix. Got: {}",
+        err
+    );


Can we switch this to snapshot testing (assert_data_eq!) like our other formats?

It was originally written using snapshot testing (assert_data_eq!), but it failed on Windows MSRV due to \ vs / path differences. Adding OS-specific handling would fix it, but it would make the code noticeably messier.

up9cloud · 2025-08-13T01:21:03Z

From https://jsonc.org/

The recommended file extension for JSONC documents is .jsonc.
The extension .json should be avoided, but is supported if a mode line is present at the start of the file:

imo comments inside of json is no longer json. We should be properly validating the json we parse.

This ultimately comes down to a design philosophy question. While the spec says otherwise, in practice, especially for configuration files, almost nobody uses the .jsonc as extension; all with .json.

That said, this discussion could go on forever. For now, I’ll change it so that jsonc only parses .jsonc. If there’s a decision later to move toward a more user-friendly approach, we can revisit it.

up9cloud · 2025-08-13T18:10:58Z

I accidentally clicked “Review changes” earlier but forgot to press the “Submit review” button, so my comment was left in pending status. It should be visible now.

matthiasbeyer requested changes Sep 20, 2023

View reviewed changes

matthiasbeyer added the hacktoberfest-accepted label Oct 23, 2023

epage removed the hacktoberfest-accepted label Oct 23, 2024

epage added the A-format label Dec 13, 2024

epage reviewed Aug 7, 2025

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

github-advanced-security bot found potential problems Aug 10, 2025

View reviewed changes

tests/testsuite/file_jsonc.rs Fixed Show fixed Hide fixed

tests/testsuite/file_jsonc.rs Fixed Show fixed Hide fixed

polarathene reviewed Aug 11, 2025

View reviewed changes

src/file/format/jsonc.rs Outdated Show resolved Hide resolved

polarathene requested changes Aug 11, 2025

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

Cargo.toml Outdated Show resolved Hide resolved

Cargo.toml Outdated Show resolved Hide resolved

polarathene mentioned this pull request Aug 12, 2025

refactor: Use a common from_value method #472

Open

github-advanced-security bot found potential problems Aug 12, 2025

View reviewed changes

tests/testsuite/file_ini.rs Fixed Show fixed Hide fixed

tests/testsuite/file_toml.rs Fixed Show fixed Hide fixed

tests/testsuite/file_yaml.rs Fixed Show fixed Hide fixed

up9cloud force-pushed the master branch 2 times, most recently from 51286fd to f4b71af Compare August 12, 2025 07:32

epage reviewed Aug 12, 2025

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

epage reviewed Aug 12, 2025

View reviewed changes

tests/testsuite/file_ini.rs Outdated Show resolved Hide resolved

epage reviewed Aug 12, 2025

View reviewed changes

up9cloud force-pushed the master branch from 16104ca to 215e7bb Compare August 13, 2025 01:27

feat: Support jsonc format

5667cc4

up9cloud force-pushed the master branch from 215e7bb to 5667cc4 Compare August 13, 2025 01:46

Support jsonc format #457

Are you sure you want to change the base?

Support jsonc format #457

Uh oh!

Conversation

up9cloud commented Sep 19, 2023

Uh oh!

matthiasbeyer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

up9cloud Sep 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coveralls commented Aug 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 16902150946

Details

💛 - Coveralls

Uh oh!

Uh oh!

Uh oh!

up9cloud commented Aug 11, 2025

Uh oh!

polarathene commented Aug 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

epage commented Aug 11, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

up9cloud commented Aug 12, 2025

Uh oh!

epage commented Aug 12, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

up9cloud Sep 20, 2023 •

edited

Loading

coveralls commented Aug 10, 2025 •

edited

Loading

polarathene commented Aug 11, 2025 •

edited

Loading