test-fullautomation / python-jsonpreprocessor

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

Method 'jsonLoads' wanted #193

Open HolQue opened 5 months ago

HolQue commented 5 months ago

Standard JSON libraries usually support beneath a 'load' function also a 'loads' function - to enable users to read content directly from strings.

In my opinion this would be a nice and useful extension also for the JsonPreprocessor.

sJSON = "{testdict.subKey_1} : \"A\""
dictReturned = json_preprocessor.jsonLoads(sJSON)

I already see a certain use case for us. A 'jsonLoads' function would enable us to autogenerate JSON test code on the fly within a Python script and pass this code to the JsonPreperocessor without the need to store every test code separately in a file on hard disc - that would be rather long winded in this case.

test-fullautomation commented 4 months ago

Hi @namsonx , Holger requested this feature. It will help him to generate tests by means of strings. Thank you, Thomas

HolQue commented 4 months ago

Hi Son, hi Thomas,

in the meantime I implemented to write autogenerated JSON code to a temporary file and I let the JsonPreprocessor read from this file. I can life with that. A jsonLoads method is still a useful feature - also for customers, but from my point of view this issue is not urgent any more.

namsonx commented 3 weeks ago

Hello Holger, Hello Thomas,

I created the commit 862548e6653 to add jsonLoads method.

The jsonLoads method is now available on stabi branch.

Thank you, Son

HolQue commented 3 weeks ago

Hi Son,

you moved a lot of code from 'jsonLoad' to new 'jsonLoads'. Maybe this is OK, but the impact is that 'jsonLoads' - that only has to read JSON content from a string immediately - contains input parameter like 'jFile' and 'masterFile'. And this is really not nice. It looks like 'jsonLoads' is a mixture of stand-alone method and a helper for 'jsonLoad'.

For what kind of purpose does 'jsonLoads' require input parameter like 'jFile' and 'masterFile'? What is the use case?

'jsonLoad' does a path normalization of 'jFile'. At end the normalized 'jFile' is handed over to 'jsonLoads'. This means, 'jsonLoads' only works with the normalized path in case of this method is called by 'jsonLoad' internally. But what shall happen in case of a developer uses 'jFile' for 'jsonLoads'?

In my opinion this is a very unfavorable software design. Please try to realize a better separation here.

In case you add a new functionality, you also have to update the package version and date, and also the history. Additionally you have to rebuild and check in the PDF documentation. This is missing.

CRQ:

The question is: What happens if the JSON string contains an import based on a relative path?

sJSON = """{
   "A" : 1,
   "[import]" : "../imported.jsonp"
}"""

dictValues = oJsonPreprocessor.jsonLoads(sJSON)

This works fine! The reference for the relative path is the folder in which the executed Python script is placed. Also this is OK.

But for testing purposes there might be the need to redirect the reference path, to be able to refer to a certain folder with imported JSON files - a folder that is located at a complete different place than the currently executed test script. Therefore 'jsonLoads' should have the possibility to define an optional root path as reference for all relative file imports within the JSON string.

Something like:

dictValues = oJsonPreprocessor.jsonLoads(sJSON, reference_path="d:/my_configuration_folder/suite_1")

HolQue commented 3 weeks ago

Hi Son,

In case of JsonPreprocessor reads invalid JSON code from file, this is the error message:

'Expecting ',' delimiter: line 4 column 4 (char 76)
Nearby: '... subKey2}" : 1,\n   "testdict2" : "${testdict1}"\n   "${testdict1.subKey1 ...'
In file: 'D:/ROBFW/Dev/GenSnippetsJPP/tmp_files/JPPSnippetFile.jsonp''

In case of JsonPreprocessor reads the same invalid JSON code from string, this is the error message:

'Expecting ',' delimiter: line 4 column 4 (char 76)'

For sure, a file information cannot be available in this case, but it should be possible to provide the 'Nearby:' information.

The 'Nearby:' information is missing because it depends on 'jFile':

if jFile != '':
    failedJsonDoc = self.__getFailedJsonDoc(error)

Please add 'self.__getFailedJsonDoc(error)' also to all relevant positions where JSON code is loaded from string.

HolQue commented 3 weeks ago

Hi Son,

In case of JsonPreprocessor reads JSON code from string, comments are not considered.

Such (valid) code

{
   "A" : 1,
   //
   "B" : 2
}

causes error:

Expecting property name enclosed in double quotes

Possible reason:

self.__load_and_removeComments()

is only called in 'jsonLoad' but not in 'jsonLoads'.

HolQue commented 3 weeks ago

See also

https://github.com/test-fullautomation/python-jsonpreprocessor/issues/315

namsonx commented 3 weeks ago

Hi Son,

you moved a lot of code from 'jsonLoad' to new 'jsonLoads'. Maybe this is OK, but the impact is that 'jsonLoads' - that only has to read JSON content from a string immediately - contains input parameter like 'jFile' and 'masterFile'. And this is really not nice. It looks like 'jsonLoads' is a mixture of stand-alone method and a helper for 'jsonLoad'.

For what kind of purpose does 'jsonLoads' require input parameter like 'jFile' and 'masterFile'? What is the use case?

'jsonLoad' does a path normalization of 'jFile'. At end the normalized 'jFile' is handed over to 'jsonLoads'. This means, 'jsonLoads' only works with the normalized path in case of this method is called by 'jsonLoad' internally. But what shall happen in case of a developer uses 'jFile' for 'jsonLoads'?

In my opinion this is a very unfavorable software design. Please try to realize a better separation here.

In case you add a new functionality, you also have to update the package version and date, and also the history. Additionally you have to rebuild and check in the PDF documentation. This is missing.

CRQ:

The question is: What happens if the JSON string contains an import based on a relative path?

sJSON = """{
   "A" : 1,
   "[import]" : "../imported.jsonp"
}"""

dictValues = oJsonPreprocessor.jsonLoads(sJSON)

This works fine! The reference for the relative path is the folder in which the executed Python script is placed. Also this is OK.

But for testing purposes there might be the need to redirect the reference path, to be able to refer to a certain folder with imported JSON files - a folder that is located at a complete different place than the currently executed test script. Therefore 'jsonLoads' should have the possibility to define an optional root path as reference for all relative file imports within the JSON string.

Something like:

dictValues = oJsonPreprocessor.jsonLoads(sJSON, reference_path="d:/my_configuration_folder/suite_1")

Hello Holger,

Thank you for your review comments! For the jFile and masterFile parameters in jsonLoads method, I will find a solution to move them out of jsonLoads method.

Regarding What happens if the JSON string contains an import based on a relative path?:

Thank you, Son

HolQue commented 3 weeks ago

Hi Son,

imports are a really good feature in JsonPreprocessor. I see no reason for blocking imports if JSON code is loaded from string. I would prefer to have this possibility implemented (together with reference_path).

namsonx commented 2 weeks ago

Hello Holger,

I updated this feature based on your review comments, the change is pushed to stabi branch via commits b01d9b5b1140 and b91836e78428

Could you please help me verify and give your feedback if any?

Thank you, Son

HolQue commented 2 weeks ago

Hi Son,

the interface descriptions of jsonLoad and jsonLoads are outdated or rather incomplete. Please update.

Also __load_and_removeComments

HolQue commented 2 weeks ago

Hi Son,

please consider also:

https://github.com/test-fullautomation/python-jsonpreprocessor/issues/316

HolQue commented 2 weeks ago

Hi Son,

feature works like expected now! After adaption of interface description issue can be closed.

namsonx commented 2 weeks ago

Hello Holger,

I updated the interface descriptions and some minor points based your review comments on stabi branch.

Thank you, Son

HolQue commented 1 week ago

Hi Son,

the parameters 'referenceDir', 'firstLevel' and 'masterFile' are not required. They are optional.

Please correct the interface description.