Allow json caster to parse boolean correctly #978

EdwardCuiPeacock · 2023-08-14T19:22:14Z

About a year ago, I made a contribution to add a feature to allow type casting (#704). In my more recent use case, I wanted to use @json to get a dictionary that contains boolean values, like the following:

my_field: 
  key: 
    attribute: True 

field_name: my_field

value: "@json @jinja {{ (this|attr(this.field_name)).get('key') or {} }}"

However, this throws the following error:

lambda x: json.loads(x.replace("'", '"'))
  File "xxxx/lib/python3.9/json/__init__.py", line 346, in loads
    return _default_decoder.decode(s)
  File "xxxx/lib/python3.9/json/decoder.py", line 337, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
  File "xxxx/lib/python3.9/json/decoder.py", line 355, in raw_decode
    raise JSONDecodeError("Expecting value", s, err.value) from None
json.decoder.JSONDecodeError: Expecting value: line 1 column 129 (char 128)

Which suggests to me that the json string parsing was not able to handle boolean values correctly. The json string to be parsed are in the form of

"{'attribute': True}"

which json.loads cannot handle. It necessarily needs to be the lower case true for json.loads to recognize this variable as a boolean.

The change I made allows @json caster to correctly parse the boolean variables by looking for strings "True" and "False" (without the quote; if it is quoted, then it stays as strings), and replace them with the lower case form.

rochacbruno · 2023-08-16T11:13:06Z

dynaconf/utils/parse_conf.py

+def _parse_json_strings(x):
+ x = x.replace("'", '"') # replace single with double quotes
+ # replace unquoted True / False with lower case true / false
+ if "True" in x:
+ x = re.sub(r'(?<!")\bTrue\b', "true", x)
+ if "False" in x:
+ x = re.sub(r'(?<!")\bFalse\b', "false", x)
+
+ return json.loads(x)


What would happen in this corner case ?

VALUE = '@json {"TrueKey": False}'

"TrueKey" won't be replaced, but I think it is worth adding this case to the tests

do we really need regex here?

what if a simple equality check?

if value in ["True", "False"]: value = value.lower()

That doesn't work because value is the whole serialized '{"TrueKey": False}' string.

You know, the simple equality makes more sense. I tested this a little bit. The string to parse is probably almost always converted from a dict after parsed by jinja. For example,

value = str({"field": True})

gives me

"{'field': True}"

So like what you have above, I can potentially just match the substring ": True, and ": False, and replace them with ": true, and ": false. I will work on this.

In this case I think the regex may be a good idea

rochacbruno · 2023-08-16T11:13:33Z

This is related to #976

pedro-psb

My suggestions are:

rename _parse_json_string param x to value
add a test to assert the replace won't affect undesired True and False occurrences. e.g

'{"KeyTrue": True}' # ok, will keep "KeyTrue"
'{"Key-True": True}' # fail, will change to "Key-true"

pedro-psb · 2023-08-17T20:28:40Z

dynaconf/utils/parse_conf.py

+def _parse_json_strings(x):
+ x = x.replace("'", '"') # replace single with double quotes
+ # replace unquoted True / False with lower case true / false
+ if "True" in x:
+ x = re.sub(r'(?<!")\bTrue\b', "true", x)
+ if "False" in x:
+ x = re.sub(r'(?<!")\bFalse\b', "false", x)
+
+ return json.loads(x)


"TrueKey" won't be replaced, but I think it is worth adding this case to the tests

EdwardCuiPeacock · 2023-08-17T21:39:48Z

My suggestions are:

rename _parse_json_string param x to value

add a test to assert the replace won't affect undesired True and False occurrences. e.g
'{"KeyTrue": True}' # ok, will keep "KeyTrue"
'{"Key-True": True}' # fail, will change to "Key-true"

Will do. Will add the test cases.

…aconf into fix/json_parse_bool

rochacbruno · 2023-08-18T18:13:35Z

dynaconf/utils/parse_conf.py

@@ -239,6 +239,17 @@ def evaluate(settings, *args, **kwargs):
 return evaluate


+def _parse_json_strings(value):
+ value = value.replace("'", '"') # replace single with double quotes


🚩

What will happen with a JSON like:

{"somekey": "This is a 'value' containing single' quotes"}

Sadly, I can't come up with a good regex to replace all the patterns I have considered,

input_strings =[ '''{"somekey": "This is a 'value' containing single' quotes", "someotherkey": "Another 'value'"}''', """{'somekey': "This is a 'value' containing single' quotes", 'someotherkey': "Another 'value'"}""", #"{'somekey': 'This is a 'value' containing single' quotes'}", # this should never happend when converting from dict to str """{'somekey': "This is a 'value' containing single' quotes", 'someotherkey': 'Another value'}""", """{'somekey': 'This is a value not containing single quotes', 'someotherkey': 'Another value'}""", # """{"somekey": 'This is a 'value', 'containing single' quotes'}""", ]

Then how about using ast.literal_eval? This would also fix the issue with True / False parsing.

codecov-commenter · 2023-08-21T17:10:36Z

Codecov Report

Merging #978 (65109cd) into master (9ca687a) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@           Coverage Diff           @@
##           master     #978   +/-   ##
=======================================
  Coverage   98.95%   98.95%           
=======================================
  Files          23       23           
  Lines        2196     2197    +1     
=======================================
+ Hits         2173     2174    +1     
  Misses         23       23

Files Changed	Coverage Δ
dynaconf/utils/parse_conf.py	`98.91% <100.00%> (+<0.01%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

rochacbruno · 2023-08-21T17:30:09Z

dynaconf/utils/parse_conf.py

 if isinstance(value, Lazy)
- else json.loads(value),
+ else ast.literal_eval(value),


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 true (valid json) and True (valid python) working.

I am rethinking this issue

Should we allow invalid json as '{"foo": True}' to be passed to @json ?

Maybe we want to make it strict, and add a new marker @dict to accept python dict literals.

Another option is adding a filter to Jinja to address your use case @EdwardCuiPeacock

"@json @jinja {{this.FOO | as_bool }}"

We can instead of dict add a @py_literal

Yep, I didn't consider that case. The tricky part is, when first rendered with @jinja, the dictionary will be converted to strings, probably by str(value), before being further casted by @json. I guess this is why we have True instead of true, or single quotes ' instead of double quotes ". I am not familiar with the code enough to pinpoint where this string conversion is (maybe under Lazy._dynaconf_encode?). But if we can make special cases when converting Python dict to strings using json.dumps instead of str, that may also solve the problem.

@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}

EdwardCuiPeacock · 2023-08-21T20:14:15Z

@rochacbruno I think the casting idea | to_bool solved my current issue. In fact, i can use | tojson within the jinja template to convert to a json string first, then @json will correctly parse the rendered string.

y_field: 
  key: 
    attribute: True 

field_name: my_field

value: "@json @jinja {{ (this|attr(this.field_name)).get('key') | tojson or {} }}"

Here is what I have so far after the update:

Adding @py_literal casting to handle non-json cases
Added some unit tests to test the new @py_literal caster
For @json casting, remove the hack to replace single quote ' with double quotes, ", as this won't work for cases like {"somekey": "This is a 'value' containing single' quotes"}.
For @json casting, adding unit testing which will correctly raise json.decoder.JSONDecodeError if the input is not strictly json

netlify · 2023-08-22T16:48:12Z

✅ Deploy Preview for dynaconf ready!

Name	Link
🔨 Latest commit	`834e974`
🔍 Latest deploy log	https://app.netlify.com/sites/dynaconf/deploys/64e7832a49d3af000809afbe
😎 Deploy Preview	https://deploy-preview-978--dynaconf.netlify.app/dynamic
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

rochacbruno

LGTM,

However we might keep this PR unmerged until we are ready for 4.0.0 as it includes a breking change.

rochacbruno · 2023-08-24T14:14:27Z

dynaconf/utils/parse_conf.py

@@ -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("'", '"'))


I like this change, to be more strict, however this is a breaking change and we will merge this only for 4.0.0

rochacbruno · 2023-08-24T14:15:29Z

dynaconf/utils/parse_conf.py

 if isinstance(value, Lazy)
 else json.loads(value),
+ "@py_literal": lambda value: value.set_casting(ast.literal_eval)


Maybe we can just short it to @pyliteral

rochacbruno · 2023-08-24T14:16:38Z

tests/test_utils.py

+ # Testing list
+ res = parse_conf_data("""@py_literal ["a", "b", 'c', 1]""")
+ assert isinstance(res, list)
+


It would be nice to add a test case for @pyliteral @jinja ... combination

EdwardCuiPeacock · 2023-08-24T15:12:18Z

@rochacbruno Sounds good to me. Let me know when that happens.

rochacbruno · 2023-08-24T15:25:42Z

BTW we need to document this

casting idea | to_bool solved my current issue. In fact, i can use | tojson within the jinja template to convert to a json string first, then @json will correctly parse the rendered string.

y_field: 
  key: 
    attribute: True 

field_name: my_field

value: "@json @jinja {{ (this|attr(this.field_name)).get('key') | tojson or {} }}"

EdwardCuiPeacock · 2023-08-24T16:28:07Z

Added the new features in docs with examples.

EdwardCuiPeacock added 2 commits August 14, 2023 15:01

allow json caster to parse boolean correctly

df435fc

Merge branch 'master' into fix/json_parse_bool

05042fe

rochacbruno reviewed Aug 16, 2023

View reviewed changes

pedro-psb requested changes Aug 17, 2023

View reviewed changes

EdwardCuiPeacock added 4 commits August 18, 2023 12:20

add more test cases; change match pattern logic

b9ad445

Merge branch 'fix/json_parse_bool' of github.com:EdwardCuiPeacock/dyn…

90a3151

…aconf into fix/json_parse_bool

Merge branch 'master' into fix/json_parse_bool

6b6a4b5

reformat test strings

7b58b84

EdwardCuiPeacock requested review from pedro-psb and rochacbruno August 18, 2023 16:23

fix trailing white space

5d9350e

rochacbruno reviewed Aug 18, 2023

View reviewed changes

use ast.literal_eval to load json dict

65109cd

rochacbruno reviewed Aug 21, 2023

View reviewed changes

rochacbruno added this to the 3.3.0 milestone Aug 21, 2023

EdwardCuiPeacock added 2 commits August 21, 2023 16:02

json casting requring strict json strings

dd58451

add test caes for py_literal

4e9d725

formatting

27a84c5

rochacbruno modified the milestones: 3.3.0, 4.0.0 Aug 22, 2023

fix unit test for py_literal on python version below 3.11

2535b09

Merge branch 'master' into fix/json_parse_bool

748c366

EdwardCuiPeacock requested a review from rochacbruno August 23, 2023 19:21

rochacbruno approved these changes Aug 24, 2023

View reviewed changes

rochacbruno added the accepted label Aug 24, 2023

rochacbruno added the DONTMERGE label Aug 24, 2023

change py_literal to pyliteral

093c5bf

rochacbruno added the Docs label Aug 24, 2023

EdwardCuiPeacock added 2 commits August 24, 2023 12:18

add more test cases for pyliteral; update documentation

2591221

fix doc

834e974

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow json caster to parse boolean correctly #978

Allow json caster to parse boolean correctly #978

EdwardCuiPeacock commented Aug 14, 2023

rochacbruno Aug 16, 2023

pedro-psb Aug 17, 2023

rochacbruno Aug 17, 2023

pedro-psb Aug 17, 2023

EdwardCuiPeacock Aug 17, 2023 •

edited

rochacbruno Aug 18, 2023

rochacbruno commented Aug 16, 2023

pedro-psb left a comment

pedro-psb Aug 17, 2023

EdwardCuiPeacock commented Aug 17, 2023

rochacbruno Aug 18, 2023

EdwardCuiPeacock Aug 18, 2023

codecov-commenter commented Aug 21, 2023

rochacbruno Aug 21, 2023

rochacbruno Aug 21, 2023

EdwardCuiPeacock Aug 21, 2023 •

edited

rochacbruno Aug 21, 2023

EdwardCuiPeacock commented Aug 21, 2023 •

edited

netlify bot commented Aug 22, 2023 •

edited

rochacbruno left a comment

rochacbruno Aug 24, 2023

rochacbruno Aug 24, 2023

rochacbruno Aug 24, 2023

EdwardCuiPeacock commented Aug 24, 2023

rochacbruno commented Aug 24, 2023

EdwardCuiPeacock commented Aug 24, 2023

Allow json caster to parse boolean correctly #978

Are you sure you want to change the base?

Allow json caster to parse boolean correctly #978

Conversation

EdwardCuiPeacock commented Aug 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EdwardCuiPeacock Aug 17, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rochacbruno commented Aug 16, 2023

pedro-psb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EdwardCuiPeacock commented Aug 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Aug 21, 2023

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EdwardCuiPeacock Aug 21, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EdwardCuiPeacock commented Aug 21, 2023 • edited

netlify bot commented Aug 22, 2023 • edited

✅ Deploy Preview for dynaconf ready!

rochacbruno left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EdwardCuiPeacock commented Aug 24, 2023

rochacbruno commented Aug 24, 2023

EdwardCuiPeacock commented Aug 24, 2023

EdwardCuiPeacock Aug 17, 2023 •

edited

EdwardCuiPeacock Aug 21, 2023 •

edited

EdwardCuiPeacock commented Aug 21, 2023 •

edited

netlify bot commented Aug 22, 2023 •

edited