test-fullautomation / python-jsonpreprocessor

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

Scope clarification wanted #260

Open HolQue opened 1 month ago

HolQue commented 1 month ago

Inside a nested dictionary, at a position where a key name is expected, a dollar operator expression is placed (not within quotes - this is another discussion):

"param"  : "value",
"params" : {"001" : {"002" : {
                              ${param} : 1
                             }
                    }
           }

Here I have doubts. Why should someone do this? What is the expectation here? What is the use case?

We need to define a name at that position. We do not speak about overwriting the value of an already existing parameter in the code above. This is the initial definition of the dictionary.

Plausible for me would be this:

"params" : {"001" : {"002" : {
                              "param" : 1,
                              ${params.001.002.param} : 2
                             }
                    }
           }

This works. Value is integer 2.

Maybe I overlook something, but my feeling is that code like in the first code example should be blocked. But the precondition for this is, that it is possible to distinguish between

${param} : 1

and

${params.001.002.param} : 2

while parsing the expression.

Or in other words: Is it possible to detect

Nevertheless. What is the current state?

(1)

This:

"param"  : "value",
"params" : {"001" : {"002" : {
                              ${param} : 1
                             }
                    }
           }

causes:

{'param': 1, 'params': {'001': {'002': {'param': 1}}}}

This is dangerous in my opinion, because an assignment within the dictionary has an impact on a parameter outside this dictionary. And the key name is the name of the used parameter. This means, inside the dictionary the ${} has no effect and is removed only.

(2)

And this:

"params" : {"001" : {"002" : {
                              ${IAMNOTEXISTING} : 1
                             }
                    }
           }

causes:

{'IAMNOTEXISTING': 1, 'params': {'001': {'002': {'IAMNOTEXISTING': 1}}}}

Also here the scopes are not separated strictly. Because both is created a single parameter with name 'IAMNOTEXISTING' and also a key with this name inside the dictionary.

The behavior of the JsonPreprocessor is dubious in this case.

How to handle this?

test-fullautomation commented 1 month ago

Hi Holger, expectation would be:

  1. $-operator won't be resolved as key, because there is no "param" existing at level params.001.002
  2. Therefore we need to raise a corresponding error messages that key is not existing in this scope.
  3. ${param} as part of the value is allowed if param exists on root level (or corresonding level) (e.g. ${abc.param})

Regards, Thomas

namsonx commented 4 days ago

Hello Holger, Hello Thomas,

This ticket was discussed and implemented in ticket: https://github.com/test-fullautomation/robotframework-testsuitesmanagement/issues/251

The change is now available on stabi branch.

Thank you, Son

HolQue commented 3 days ago

The code

"param"  : "value",
"params" : {"001" : {"002" : {
                              ${param} : 1
                             }
                    }
           }

causes

Error: 'Could not set the value for parameter '${param}'.
Please set this parameter with an absolute path, for example: '${params}['001']['002']['param']''!

The problem in the code above is that a parameter with name "param" does not exist in the scope of "params.001.002".

In my opinion this is worth to be mentioned. The key can for sure be created with full path and dollar operator syntax

${params}['001']['002']['param'] : 1

But this is really long winded. Who one will do this in this way? In my opinion we should not recommend this. This is much easier:

"param" : 1

I suggest to change the error message in this case:

A key with name ${param} does not exist at this position. Use the "<name> : <value>" syntax to create a new key.

It can be explained in more detail in the documentation that a full absolute path is required to overwrite a key that already exists at this position (but there is no need to use an absolute path to create a new key).

Another try (parameter exists but full path not defined; parameter with same name at top level)

"param"  : "value",
"params" : {"001" : {"002" : {
                              "param" : 1,
                              ${param} : 2
                             }
                    }
           }

Outcome:

Top level "param" still has value "value". But "param" under "params.001.002" has value 2. This means: ${param} : 2 works - even in case of the absolute path is not given.

How to handle this? Shall this be OK? Currently this is in opposite to the recommendation in error message above: Please set this parameter with an absolute path

HolQue commented 3 days ago

Another use case: Dictionary is list element:

"param"  : "value",
"params" : {"001" : ["002" , {${param} : 1}]}

Result:

{'param': 1,
 'params': DotDict({'001': ['002', DotDict({'param': 1})]})}

Not expected: param with value 1 (at top level and inside the dictionary) Expected error message: A key with name ${param} does not exist at this position. A value cannot be assigned.

HolQue commented 3 days ago
"params" : {"001" : "002",
            ${indexP} : ["004", {"005" : "006",
            ...

causes:

'Could not set the value for parameter '${indexP}'.
Please set this parameter with an absolute path, for example: '${params}['indexP']''

The absolute path recommendation '${params}['indexP']' is correct.

But:

"params" : {"001" : "002",
            "003" : ["004", {"005" : "006",
                           ${indexP} : ["008", ["009", "010"]],
            ...

causes:

'Could not set the value for parameter '${indexP}'.
Please set this parameter with an absolute path, for example: '${005}['indexP']''

What is the origin of this recommendation: '${005}['indexP']'? From where does the expression ${005} come from? This does not fit to the JSON code

The full path is: ${params}['003'][1]['indexP'].

But like already mentioned: The problem is that indexP does not exist at this position.

HolQue commented 3 days ago
"param"  : "value",
"params" : {"001" : ["002" , {"param" : 1,
                              ${param} : 2}]}

causes:

{'param': 'value',
 'params': {'001': ['002',
                    {'param': ['__handleDuplicatedKey__', 1, 2],
                     'param__handleDuplicatedKey__2': 0}]}}

Internal keywords in output are not expected.

Cross check with full path:

"param"  : "value",
"params" : {"001" : ["002" , {"param" : 1,
                              ${params}['001'][1]['param'] : 2}]}

Correct:

{'param': 'value',
 'params': DotDict({'001': ['002', DotDict({'param': 2})]})}

Shorter version (without "param" initialized within dictionary):

"params" : {"001" : ["002" , {${params}['001'][1]['param'] : 2}]}

Result:

Error: '${params} expects integer as index. Got string instead in ${params}['001']__IndexOfList__['param']'!

What is this???

namsonx commented 1 day ago

Another use case: Dictionary is list element:

"param"  : "value",
"params" : {"001" : ["002" , {${param} : 1}]}

Result:

{'param': 1,
 'params': DotDict({'001': ['002', DotDict({'param': 1})]})}

Not expected: param with value 1 (at top level and inside the dictionary) Expected error message: A key with name ${param} does not exist at this position. A value cannot be assigned.

Hello Holger,

Thank you for your finding! "param with value 1 (at top level and inside the dictionary)" is the issue and I'll solve it. For the error message you suggest in this case A key with name ${param} does not exist at this position. A value cannot be assigned.

Thank you, Son

namsonx commented 1 day ago
"param"  : "value",
"params" : {"001" : ["002" , {"param" : 1,
                              ${param} : 2}]}

causes:

{'param': 'value',
 'params': {'001': ['002',
                    {'param': ['__handleDuplicatedKey__', 1, 2],
                     'param__handleDuplicatedKey__2': 0}]}}

Internal keywords in output are not expected.

Cross check with full path:

"param"  : "value",
"params" : {"001" : ["002" , {"param" : 1,
                              ${params}['001'][1]['param'] : 2}]}

Correct:

{'param': 'value',
 'params': DotDict({'001': ['002', DotDict({'param': 2})]})}

Shorter version (without "param" initialized within dictionary):

"params" : {"001" : ["002" , {${params}['001'][1]['param'] : 2}]}

Result:

Error: '${params} expects integer as index. Got string instead in ${params}['001']__IndexOfList__['param']'!

What is this???

Hello Holger,

According to this comment, I would separate into 2 areas: 1. The issue related to Internal keywords in output are not expected.

Thank you, Son

HolQue commented 1 day ago

Hi Son,

currently the JsonPreprocessor converts

{${param} : 1}

to

DotDict({'param': 1})

And this is definitely wrong. You cannot ignore the dollar operator and use the parameter name as key name.

Also in case of {${param} : 1} is part of a list: A parameter with name 'param' does not exist inside {${param} : 1}.

In my opinion, code like this

"param"  : "value",
"params" : {"001" : ["002" , {${param} : 1}]}

should cause an error.

This would be a way to work with dictionaries inside lists:

"keyparam" : "key",
// create the key 'key':
"params"   : {"001" : ["002" , {"key" : 1}]},
// overwrite the key:
${params}['001'][1][${keyparam}] : 2

This works like expected:

{'keyparam': 'key',
 'params': DotDict({'001': ['002', DotDict({'key': 2})]})}

Unfortunately the same in dotdict notation does not work:

"keyparam" : "key",
"params"   : {"001" : ["002" , {"key" : 1}]},
${params.001.1.${keyparam}} : 2

Result:

Error: 'Identified dynamic name of key '${params.001.1.${keyparam}}' that does not exist. But new keys can only be created based on hard code names.'!

But expected is the same result as in standard notation before.

namsonx commented 1 day ago

Hello Holger,

Yes, I agree with you, and I will implement to raise an error message for the first case you mentioned in previous comment. For the third case you mentioned in your comment, this is an issue, and I will fixe it.

Note: Like I mentioned in my comment above, I will block in case user try to overwrite a parameter inside a JSON object which is an item of list value with an error message A key with name <key_name> does not exist at this position.. Is it fine based on your view?

Thank you, Son

HolQue commented 1 day ago

Hi Son,

like I already suggested above in this issue, a second sentence would be helpful:

A key with name <key_name> does not exist at this position. Use the "<name> : <value>" syntax to create a new key.

w.r.t your statement:

I will block in case user try to overwrite a parameter inside a JSON object which is an item of list value

This statement is completely wrong. The data structure doesn't matter! If lists are involved or not doesn't matter! It is for sure allowed to overwrite a value. Even in case of this value belongs to an element of a list. I already demonstrated this above with:

"keyparam" : "key",
// create the key 'key':
"params"   : {"001" : ["002" , {"key" : 1}]},
// overwrite the key:
${params}['001'][1][${keyparam}] : 2

But to change a key value with dollar operator syntax, requires that this key exists already.

And an error has to be thrown, if the key does not exist. This is the striking point.

namsonx commented 1 day ago

Hello Holger,

It seems we were not in the same page :) I mean we will block the case below:

"var" : {"var1" : 1},
"keyparam" : "key",
// create the key 'key':
"params"   : {"001" : ["002" , {"key" : 1, ${var.var1} : 2}]}  // This case dose not allow

Thank you, Son

HolQue commented 1 day ago

Hi Son,

I complained about the reason you gave, not about the fix you plan. The fix is fine to me.