-
-
Notifications
You must be signed in to change notification settings - Fork 287
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
Allow json caster to parse boolean correctly #978
base: master
Are you sure you want to change the base?
Changes from 13 commits
df435fc
05042fe
b9ad445
90a3151
6b6a4b5
7b58b84
5d9350e
65109cd
dd58451
4e9d725
27a84c5
2535b09
748c366
093c5bf
2591221
834e974
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
from __future__ import annotations | ||
|
||
import ast | ||
import json | ||
import os | ||
import re | ||
|
@@ -254,11 +255,12 @@ def evaluate(settings, *args, **kwargs): | |
) | ||
if isinstance(value, Lazy) | ||
else str(value).lower() in true_values, | ||
"@json": lambda value: value.set_casting( | ||
lambda x: json.loads(x.replace("'", '"')) | ||
) | ||
"@json": lambda value: value.set_casting(json.loads) | ||
if isinstance(value, Lazy) | ||
else json.loads(value), | ||
"@py_literal": lambda value: value.set_casting(ast.literal_eval) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we can just short it to |
||
if isinstance(value, Lazy) | ||
else ast.literal_eval(value), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this will address all the cases, look: Case 1 OK >>> ast.literal_eval("{'foo': True}")
{'foo': True} Case 2 - NOK >>> ast.literal_eval("{'foo': true}")
ValueError: malformed node or string on line 1: <ast.Name object at 0x7f4e111cb6d0> It is really trick that we need to make both I am rethinking this issue Should we allow invalid json as '{"foo": True}' to be passed to Maybe we want to make it strict, and add a new marker Another option is adding a filter to Jinja to address your use case @EdwardCuiPeacock
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can instead of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, I didn't consider that case. The tricky part is, when first rendered with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @EdwardCuiPeacock there is another option, custom JSONDEcoder In [37]: class BoolDecoder(json.JSONDecoder):
...: def raw_decode(self, s, idx=0):
...: try:
...: return super().raw_decode(s, idx=idx)
...: except json.JSONDecodeError:
...: # Handle the replacement here
...: # find :True : True, :False, : False
...: # pattern and replace with lowercase
# BETTER USE A REGEX HERE
...: if "True" in s:
...: s = s.replace("True", "true")
...: if "False" in s:
...: s = s.replace("False", "false")
...: return super().raw_decode(s, idx=idx)
...:
In [38]: json.loads('{"foo": True, "a": 1}', cls=BoolDecoder)
Out[38]: {'foo': True, 'a': 1}
In [39]: json.loads('{"foo": False, "a": 1}', cls=BoolDecoder)
Out[39]: {'foo': False, 'a': 1}
In [40]: json.loads('{"foo": false, "a": 1}', cls=BoolDecoder)
Out[40]: {'foo': False, 'a': 1}
In [41]: json.loads('{"foo": true, "a": 1}', cls=BoolDecoder)
Out[41]: {'foo': True, 'a': 1} |
||
"@format": lambda value: Lazy(value), | ||
"@jinja": lambda value: Lazy(value, formatter=Formatters.jinja_formatter), | ||
# Meta Values to trigger pre assignment actions | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -162,33 +162,104 @@ def test_casting_bool(settings): | |
|
||
|
||
def test_casting_json(settings): | ||
res = parse_conf_data("""@json {"FOO": "bar"}""") | ||
# Testing cases where input is strictly json | ||
res = parse_conf_data( | ||
"""@json { | ||
"FOO": "bar", | ||
"key": false, | ||
"somekey": "this is a 'value' with single quote" | ||
}""" | ||
) | ||
assert isinstance(res, dict) | ||
assert "FOO" in res and "bar" in res.values() | ||
|
||
# Test how single quotes cases are handled. | ||
# When jinja uses `attr` to render a json string, | ||
# it may convert double quotes to single quotes. | ||
settings.set("value", "{'FOO': 'bar'}") | ||
res = parse_conf_data("@json @jinja {{ this.value }}")(settings) | ||
# parsing true and false okay | ||
assert "key" in res and False in res.values() | ||
# parsing single quote okay | ||
assert "somekey" in res and "'value'" in res["somekey"] | ||
|
||
# Testing invalid json: single quotes | ||
json_err = json.decoder.JSONDecodeError | ||
err_str = "Expecting property name enclosed in double quotes" | ||
with pytest.raises(json_err, match=err_str): | ||
res = parse_conf_data("""@json {'FOO': 'bar'}""") | ||
|
||
# Testing invalid json: upper case True | ||
with pytest.raises(json_err, match="Expecting value"): | ||
res = parse_conf_data("""@json {"FOO": True}""") | ||
|
||
# Testing jinja parsed dictionary | ||
settings.set("value", {"FOO": "bar", "Y": True, "Z": "False"}) | ||
# This will fail since jinja will convert the | ||
# dict to string using single quotes | ||
with pytest.raises(json_err, match=err_str): | ||
res = parse_conf_data("@json @jinja {{ this.value }}")(settings) | ||
# However, casting to json first before parsing will pass | ||
res = parse_conf_data("@json @jinja {{ this.value | tojson }}")(settings) | ||
assert isinstance(res, dict) | ||
assert "FOO" in res and "bar" in res.values() | ||
|
||
res = parse_conf_data("@json @format {this.value}")(settings) | ||
assert "FOO" in res and "bar" == res["FOO"] | ||
assert "Y" in res and res["Y"] is True | ||
assert "Z" in res and res["Z"] == "False" | ||
|
||
# Testing format parsed dictionary | ||
# This will fail if 'value' is a dict | ||
with pytest.raises(json_err, match=err_str): | ||
res = parse_conf_data("@json @format {this.value}")(settings) | ||
# This will work if 'value' is a proper json string | ||
settings.set("value", '{"FOO": "bar", "Y": true, "Z": "False"}') | ||
parse_conf_data("@json @format {this.value}")(settings) | ||
assert isinstance(res, dict) | ||
assert "FOO" in res and "bar" in res.values() | ||
assert "FOO" in res and "bar" == res["FOO"] | ||
assert "Y" in res and res["Y"] is True | ||
assert "Z" in res and res["Z"] == "False" | ||
|
||
# Test jinja rendering a dict | ||
settings.set("value", "OPTION1") | ||
settings.set("OPTION1", {"bar": 1}) | ||
settings.set("OPTION2", {"bar": 2}) | ||
res = parse_conf_data("@jinja {{ this|attr(this.value) }}")(settings) | ||
assert isinstance(res, str) | ||
res = parse_conf_data("@json @jinja {{ this|attr(this.value) }}")(settings) | ||
with pytest.raises(json_err, match=err_str): | ||
res = parse_conf_data("@json @jinja {{ this|attr(this.value) }}")( | ||
settings | ||
) | ||
res = parse_conf_data("@json @jinja {{ this|attr(this.value)|tojson }}")( | ||
settings | ||
) | ||
assert isinstance(res, dict) | ||
assert "bar" in res and res["bar"] == 1 | ||
|
||
|
||
def test_casting_pyliteral(settings): | ||
# Testing cases where the input is strictly json | ||
# This will fail at the boolean | ||
with pytest.raises(ValueError): | ||
res = parse_conf_data( | ||
"""@py_literal { | ||
"FOO": "bar", | ||
"key": false, | ||
"somekey": "this is a 'value' with single quote" | ||
}""" | ||
) | ||
|
||
# Testing cases where input is a dict but not | ||
# strictly json | ||
res = parse_conf_data( | ||
"""@py_literal { | ||
"FOO": "bar", | ||
"key": False, | ||
'somekey': "this is a 'value' with single quote" | ||
}""" | ||
) | ||
assert isinstance(res, dict) | ||
assert "FOO" in res and res["FOO"] == "bar" | ||
assert "key" in res and res["key"] is False | ||
assert "somekey" in res and "'value'" in res["somekey"] | ||
|
||
# Testing list | ||
res = parse_conf_data("""@py_literal ["a", "b", 'c', 1]""") | ||
assert isinstance(res, list) | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be nice to add a test case for |
||
|
||
def test_disable_cast(monkeypatch): | ||
# this casts for int | ||
assert parse_conf_data("@int 42", box_settings={}) == 42 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this change, to be more strict, however this is a breaking change and we will merge this only for 4.0.0