test-fullautomation / python-jsonpreprocessor

A preprocessor for json files
Apache License 2.0
2 stars 2 forks source link

Parameter substitution needs to be limited #249

Open HolQue opened 1 month ago

HolQue commented 1 month ago

Somewhere in the past it has been decided to allow the substitution of dollar operator expressions within strings only for parameters of data type 'str' and 'int' (not others like 'dict' and 'list' any more). And on both the left hand side of the colon and the right hand side of the colon.

The following combinations are blocked correctly (like expected):

"dictParam"    : {"A" : 0, "B" : 1},
"${dictParam}" : 1
"listParam"    : ["A", "B"],
"${listParam}" : 1

But the following combinations are not blocked. The substitution work, but should be blocked now also:

"dictParam"  : {"A" : 0, "B" : 1},
"param"      : "${dictParam}"
"listParam"  : ["A", "B"],
"param"      : "${listParam}"

How about float values:

"floatParam" : 1.2,
"param"      : "${floatParam}"
"floatParam"    : 1.2,
"${floatParam}" : 1

Currently substitution work. Keep working or block it?

namsonx commented 3 weeks ago

Hello Holger,

Thomas decided to block the Parameter substitution via ticket https://github.com/test-fullautomation/python-jsonpreprocessor/issues/270 I implemented and pushed the change to stabi branch.

Thank you, Son

HolQue commented 1 week ago

Hi Son,

in expressions on the right hand side of the colon still composite data types are not blocked inside strings.

I tried:

"strval"    : "ABC",
"intval"    : 1,
"floatval"  : 2.3,
"boolval"   : True,
"noneval"   : None,
"listval"   : [1,2,3],
"dictval"   : {"A" : "B"},
"newparam1" : "prefix_${strval}_suffix",
"newparam2" : "prefix_${intval}_suffix",
"newparam3" : "prefix_${floatval}_suffix",
"newparam4" : "prefix_${boolval}_suffix",
"newparam5" : "prefix_${noneval}_suffix",
"newparam6" : "prefix_${listval}_suffix",
"newparam7" : "prefix_${dictval}_suffix"

With result:

{'boolval': True,
 'dictval': {'A': 'B'},
 'floatval': 2.3,
 'intval': 1,
 'listval': [1, 2, 3],
 'newparam1': 'prefix_ABC_suffix',
 'newparam2': 'prefix_1_suffix',
 'newparam3': 'prefix_2.3_suffix',
 'newparam4': 'prefix_True_suffix',
 'newparam5': 'prefix_None_suffix',
 'newparam6': 'prefix_[1, 2, 3]_suffix',
 'newparam7': "prefix_{'A': 'B'}_suffix",
 'noneval': None,
 'strval': 'ABC'}

In case of newparam6 and newparam7 I would expect an error message telling that the substitution of nested data types inside string values is not supported.

namsonx commented 1 week ago

Hello Holger,

The value on the right-hand side of the colon, if enclosed in double quotes, is converted to the string datatype following Python's behavior. Additionally, users can convert dictionaries or lists to strings for specific purposes, as allowed by JP.

From my point of view, we should allow users convert event dictionaries, lists, ... to string if they want to do it. Hello @test-fullautomation, could you please give us your opinion?

Thank you, Son

namsonx commented 1 week ago

We will implement like Holger's expectation

namsonx commented 5 days ago

Hello Holger,

I pushed commit c79a4368d1 to stabi branch to implement this requirement for values (the right hand side of the colons). Could you please help me verify?

Thank you, Son

HolQue commented 3 days ago

The fix only works in case of the string contains hard coded parts (like prefix_ and _suffix) additionally;

Error: 'The substitution of parameter '${listval}' inside the string value 'prefix_${listval}_suffix' is not supported! The composite data types between 'str' and 'list' is not acceptable.'!

This part of the error message is really strange:

The composite data types between 'str' and 'list' is not acceptable.

Please change to

Composite data types like lists and dictionaries cannot be substituted inside strings.

Same code example, but without prefix_ and _suffix:

"strval"    : "ABC",
"intval"    : 1,
"floatval"  : 2.3,
"boolval"   : True,
"noneval"   : None,
"listval"   : [1,2,3],
"dictval"   : {"A" : "B"},
"newparam1" : "${strval}",
"newparam2" : "${intval}",
"newparam3" : "${floatval}",
"newparam4" : "${boolval}",
"newparam5" : "${noneval}",
"newparam6" : "${listval}",
"newparam7" : "${dictval}"

Result:

{'boolval': True,
 'dictval': DotDict({'A': 'B'}),
 'floatval': 2.3,
 'intval': 1,
 'listval': [1, 2, 3],
 'newparam1': 'ABC',
 'newparam2': '1',
 'newparam3': '2.3',
 'newparam4': 'True',
 'newparam5': 'None',
 'newparam6': '[1, 2, 3]',
 'newparam7': "{'A': 'B'}",
 'noneval': None,
 'strval': 'ABC'}

newparam6 and newparam7 still with issue.

namsonx commented 3 days ago

Hello Holger,

In the case you mentioned above, new parameters newparam6 and newparam7 simply convert list or dict to string. There are no composite data types, do we also block these cases?

From my point of view, users sometimes want to convert list or dict parameters to string.

Thank you, Son

test-fullautomation commented 3 days ago

Hi Son, in case of newparam6,7 we need also an error message that composite types (lists, dicts) are not allowed.

Conversion looks nice in this case, but has no useful content if the data is more complex. E.g. nested lists, nested dicts, ... If somebody really needs this he need to do this on python level.

Thank you, Thomas

namsonx commented 3 days ago

Hello Thomas,

The requirement is clear now, I will update the code.

Thank you, Son

namsonx commented 3 days ago

Hello Holger, Hello Thomas,

I push commit ee029c467a2 and created pull-request to update this requirement.

Thank you, Son

HolQue commented 3 days ago

Retest successful. Issue can be closed.