test-fullautomation / python-jsonpreprocessor

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

Error messages (19) #317

Closed HolQue closed 1 month ago

HolQue commented 3 months ago

Previously this code

{
   "testlist" : ["A", "B", "C", "D"],
   "param2"   : ${testlist}[X]
}

caused

'Could not resolve expression '${testlist}[X]'. Reason: list indices must be integers, not str'

That's completely fine.

With latest changes of the JsonPreprocessor the error message changed to:

'Invalid syntax! Sub-element '[X]' in ${testlist}[X] has to enclosed in quotes.'

That's wrong. X is something completely invalid here, not a string encapsulated in quotes, and not a parameter.

Therefore we should point out the requirement: list indices must be integers. That is what the error message should tell.

namsonx commented 3 months ago

Hello Holger,

This is the improvement I made before. Please find my explanation below:

First JPP checks syntax of expressions, then it tries to resolve expressions.

For the example in this ticket:

Additional, I tried some different ways based on this example:

   "testlist" : ["A", "B", "C", "D"],
   "param2"   : ${testlist}['X']

or

   "testlist" : ["A", "B", "C", "D"],
   "param2"   : ${testlist.X}

or

   "testlist" : ["A", "B", "C", "D"],
   "param2"   : "${testlist}['X']"

or

   "testlist" : ["A", "B", "C", "D"],
   "param2"   : "${testlist.X}"

or

   "testlist" : ["A", "B", "C", "D"],
   ${testlist}['X']   :  1

or

   "testlist" : ["A", "B", "C", "D"],
   ${testlist.X}   :  1

All cases log error message Exception: Could not resolve expression '${testlist}['X']'. Reason: list indices must be integers, not str

Could you please review this ticket again?

Thank you, Son

HolQue commented 2 months ago

Hi Son,

when someone does this: ${testlist}[X], the error message tells, that [X] has to be enclosed in quotes. That's misleading. What you maybe want to indicate is, that the content of the square brackets X has to be enclosed in quotes, but not the entire part of the expression (not '[X]' but ['X']). But when someone does, what the error message proposes (${testlist}['X']), then it's also not correct (because list indices must be integers ...).

The first error message does not propose a proper solution, but a solution that causes an aftereffect. This makes no sense.

The parameter ${testlist} is of type list. Therefore within the square brackets an integer is expected. But X is not an integer and also not an integer parameter. X is also not a string (because not encapsulated in quotes). X is something completely unexpected at this position and therefore the expression is invalid and cannot be resolved.

Out of the fact that the expression is invalid, you cannot conclude that quotes are missing. That's wrong.

Nevertheless, there is one thing we know for sure: An integer is expected. Therefore please provide an error message, that tells exactly this.

namsonx commented 2 months ago

Hello Holger,

For the first part of your comment, I agreed with you regarding the statement the content of the square brackets X has to be enclosed in quotes, but not the entire part of the expression (not '[X]' but ['X']). I will update the error message to make it clearer.

For the second part, as I mentioned above JPP will check the syntax of an expression first, in case the syntax of the expression is correct then we continue to proceed next steps and issue other error messages if any. Why JPP has to check other factors like a data type of a variable in an expression while the expression is in wrong format? It will make our code is more complicated.

Thank you, Son

HolQue commented 2 months ago

Hi Son,

OK, once again: The parameter ${testlist} is of type list. Therefore within the square brackets an integer is expected.

Found is: X

X is not an integer parameter. Easy to detect this: No dollar operator involved.

X is not an integer. Easy to detect this:

try:
    X = int(X)
except as ex:
    ...

This is not complicated code. This is simple standard.

While parsing expressions you should try to get as much informations as possible, to be able to provide meaningful error messages.

In this case it's quite easy: An integer is expected. An integer is not found. Therefore you provide an error message telling that an integer is expected.

namsonx commented 2 months ago

Hello Holger,

To detect X is not an integer is simple like you mentioned above, but to detect variable ${testlist} is a list does not simple.

A parameter in jsonp configuration file could be int, float, string, list, dictionary, etc. These parameters are not a variable of Python program, they belong to JSON object.

The complicated thing does not come from how to detect X is integer or string but the way to get the data type of ${testlist} (is it list, dict, int, str, ...)

Below is the code to get a value of a parameter in jsonp configuration file: (from value we can detect a parameter is dict or list)

            sExec = "value = self.JPGlobals"
            oTmpObj = self.JPGlobals
            for element in lElements:
                bList = False
                if re.match(r"^[\s\-]*\d+$", element):
                    bList = True
                    sExec = sExec + f"[{element}]"
                elif re.match(rf"^'\s*[^{re.escape(self.specialCharacters)}]+\s*'$", element.strip()):
                    element = element.strip("'")
                if not bList:
                    if isinstance(oTmpObj, dict) and element not in oTmpObj.keys():
                        sDuplicatedCheck = element + CNameMangling.DUPLICATEDKEY_01.value
                        for key in oTmpObj.keys():
                            if sDuplicatedCheck in key and CNameMangling.DUPLICATEDKEY_00.value not in key:
                                element = key                            
                    sExec = sExec + f"['{element}']"
                if not bList and isinstance(oTmpObj, dict):
                    if element in oTmpObj and (isinstance(oTmpObj[element], dict) or \
                                               isinstance(oTmpObj[element], list)):
                        oTmpObj = oTmpObj[element]
                elif bList and isinstance(oTmpObj, list):
                    if int(element)<len(oTmpObj) and (isinstance(oTmpObj[int(element)], dict) or \
                                                      isinstance(oTmpObj[int(element)], list)):
                        oTmpObj = oTmpObj[int(element)]
            try:
                ldict = {}
                exec(sExec, locals(), ldict)
                tmpValue = ldict['value']
            except Exception as error:
                self.__reset()
                errorMsg = ''
                for errorType in self.pythonTypeError:
                    if errorType in str(error):
                        errorMsg = f"Could not resolve expression '{sNestedParam.replace('$$', '$')}'."
                if errorMsg != '':
                    errorMsg = errorMsg + f" Reason: {error}" if ' or slices' not in str(error) else \
                                errorMsg + f" Reason: {str(error).replace(' or slices', '')}"
                else:
                    if isinstance(error, KeyError) and re.search(r"\[\s*" + str(error) + "\s*\]", sNestedParam):
                        errorMsg = f"Could not resolve expression '{sNestedParam.replace('$$', '$')}'. \
Reason: Key error {error}"
                    else:
                        errorMsg = f"The parameter '{sNestedParam.replace('$$', '$')}' is not available!"
                raise Exception(errorMsg)

Thank you, Son

HolQue commented 2 months ago

Hi Son,

seems that you really have a severe issue in your parser. Already in so many cases, again and again and again and again I told you: In case of expression like ${X}[Y] you need to consider the data type and X and Y. This is a must. You cannot compute these expressions in a proper way if you do not know the data types. And now - after months are gone - you tell that computing the data types is complicated? What are you doing? This is so much strange. Please take a look at the top of this issue. I mentioned there an error message, that already has been provided by the JsonPreprocessor:

'Could not resolve expression '${testlist}[X]'. Reason: list indices must be integers, not str'

This means you already know that ${testlist} is a list. The problem we are discussing now, was already solved.

But you have implemented changes - and now the error message is gone. Replaced by a worse error message.

Why are you doing this?

namsonx commented 2 months ago

Hello Holger,

I think there is no problem with this error message, let I explain detail to you.

Suppose there is a parameter call abc was defined in jsonp file, then user uses the expression ${abc}[xyz]. What we observe in this expression? User wants to get a value of object xyz in ${abc}. Now, there is no syntax which matching in this case:

As I mentioned in my comment above, in my implemented change:

In case user updated the expression to ${abc}['xyz'], while JPP computes this expression, and if it detect ${abc} is a list, then JPP will raise the error message Could not resolve expression '${abc}['xyz']'. Reason: list indices must be integers, not str

Conclusion:

Thank you, Son

HolQue commented 2 months ago

Hi Son,

I think, now I understand. It has something to do with the order of computation. At first the syntax, after this the data types. And in case of syntax issues no further analysis. Clear to me now.

But when you stop the analysis, informations about data types are missing. In this case you cannot give such detailed hints like sub elements that have to be enclosed in quotes. You cannot know if quotes are missing. Because this depends on the data type. If e.g. an index is expected, the content of the square brackets must not be enclosed in quotes. Therefore your hint about the missing quotes is potentially misleading.

And it is really not user friendly that a hint in an error message causes aftereffects. Hints or recommendations in error messages should help to solve the problem, but should not cause further problems. Adding quotes is not the solution in every case.

OK, coming back to the invalid code:

"param2" : ${testlist}[X]

JsonPreprocessor detects a syntax error. Further informations are not available because of a stopped analysis.

In this case an error message could be:

Invalid syntax in expression '${testlist}[X]' (nearby X).

Or

Invalid syntax in expression '${testlist}[X]' (nearby [X]).

That's all. No further recommendations or proposals about how to fix. Without further analysis you simply do not know what's wrong in detail.

namsonx commented 2 months ago

Hello Holger,

you're right! The error message in this invalid syntax should be modified to make it's clearer, I will update this error message.

Additionally, I have tried to check the behavior of the same case in this ticket.

I created a simple Python script with invalid syntax:

listVar = ['A', 'B', 'C']
param = listVar[A]
print(param)

I received the error message NameError: name 'A' is not defined

Then I corrected the syntax:

listVar = ['A', 'B', 'C']
param = listVar['A']
print(param)

I received the error message TypeError: list indices must be integers or slices, not str

Thank you, Son

test-fullautomation commented 1 month ago

Hi @namsonx ,

As discussed Here my proposal for the error message: “Exception: Invalid syntax! Sub-element 'X' in ${testlist}[X] need to be referenced using ${X} or enclosed in quotes [‘X’].”

If X is not existing is not relevant in the Exception because this will cause anyhow another Exception if the syntax is corrected.

Thank you, Thomas

namsonx commented 1 month ago

Ready for sprint 0.13.0 [09.08.24](jsonpreprocessor / tsm)