Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

json2dict: Prevent converting boolean strings to numerical values #39

Merged
merged 1 commit into from Jan 19, 2024

Conversation

ramikhaldi
Copy link

No description provided.

@drakeapps
Copy link

I don't really think this is correct. TCL doesn't have a boolean, but 0/1 are treated as the de facto booleans for the the most part. JSON, of course, has types and does have a boolean, true/false

TCL also supports true/yes/tru/t, etc for true, similar for false.

If you're wrapping it in an if condition, if {[dict get $json boolField]} should work with any of the above options.

If you want to safely check it, if {[string is true -strict [dict get $json boolField]]}

@ramikhaldi
Copy link
Author

ramikhaldi commented Oct 1, 2021

If you have a json and you want to convert it using the yajltcl c library to a dictionary, the c based function (no Tcl code needed) should handle it correctly and don't convert true/false to 1/0. Otherwise your dict will contain different values.. The following examples demonstrate what I mean:


set json {
	{
	   "valueTrue":true,
	   "valueFalse":false,
	   "x":"true",
	   "y":false,
	   "a":{
		  "b":1,
		  "c":"true",
		  "d":{
			 "p":false
		  }
	   }
	}
}

::yajl::json2dict $json

Result (Before the fix):

valueTrue 1 valueFalse 0 x true y 0 a {b 1 c true d {p 0}}

Result (after the fix):

valueTrue true valueFalse false x true y false a {b 1 c true d {p false}}

This behavior can only be observed with the unquoted true/false..

@gahr
Copy link
Contributor

gahr commented Oct 1, 2021

Basically anything that Tcl_GetBoolean accepts would be semantically correct, but I agree with @drakeapps that 0 and 1 looks like the most canonical values for booleans.

Otherwise your dict will contain different values

json2dict is inherently lossy. You can't map it back to the original json because we lose type information.

@bovine
Copy link
Member

bovine commented Oct 1, 2021

This yajl::json2dict method is fundamentally trying to be a drop-in replacement for json::json2dict, so the fact that there is a behavior difference with bools is the concern here.

@ramikhaldi
Copy link
Author

The change has passed the ci checks and is also fixing the issue. Merge ?

@bovine
Copy link
Member

bovine commented Jan 18, 2024

After re-testing with json::json2dict, it looks like we should actually be returning true/false instead of 1/0 if we want to claim compatibility with the original implementation.

tclsh8.6 [~] package require json
1.3.4

tclsh8.6 [~] ::json::json2dict $json
valueTrue true valueFalse false x true y false a {b 1 c true d {p false}}

tclsh8.6 [~] ::yajl::json2dict $json
valueTrue 1 valueFalse 0 x true y 0 a {b 1 c true d {p 0}}

@bovine bovine merged commit 1df5594 into flightaware:master Jan 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants