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.

epage · 2025-08-12T15:43:13Z

Cargo.toml

@@ -126,8 +126,9 @@ json = ["serde_json"]
 yaml = ["yaml-rust2"]
 ini = ["rust-ini"]
 json5 = ["json5_rs", "serde/derive"]
+jsonc = ["jsonc-parser"]


Suggested change

jsonc = ["jsonc-parser"]

jsonc = ["dep:jsonc-parser"]

#676 is tracking fixing the rest.

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

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

tests/testsuite/file_ini.rs

+    assert!(res.is_ok());
+    let c = res.unwrap();
+    assert_data_eq!(
+        format!("{:?}", c),


c.to_debug()

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?

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 from f4a8c16 to 51286fd Compare August 12, 2025 07:30

feat: Support jsonc format

f4b71af

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

epage reviewed Aug 12, 2025

View reviewed changes

tests/testsuite/file_ini.rs

assert!(res.is_ok());

let c = res.unwrap();

assert_data_eq!(

format!("{:?}", c),

Copy link

Contributor

epage Aug 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

c.to_debug()

epage reviewed Aug 12, 2025

View reviewed changes

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!

matthiasbeyer Sep 20, 2023

Choose a reason for hiding this comment

Uh oh!

up9cloud Sep 20, 2023

Choose a reason for hiding this comment

Uh oh!

matthiasbeyer Sep 20, 2023

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!

epage Aug 12, 2025

Choose a reason for hiding this comment

Uh oh!

epage Aug 12, 2025

up9cloud Sep 20, 2023 •

edited

Loading

coveralls commented Aug 10, 2025 •

edited

Loading

polarathene commented Aug 11, 2025 •

edited

Loading