Input Support for Double2dArray for Client Generator and Service and updating Sample Measurement

[Blahpapto1]

• This contribution adheres to CONTRIBUTING.md.

What does this Pull Request accomplish?

Adding Input Support for Measurment Client Generator and Service and updating Sample Measurement.

Why should this Pull Request be merged?

Adding Input Support for Client Generator, Service and Sample Measurement

What testing has been done?

• Automated tests are passing after adding Double2DArray Input parameter.
• Measurement is running fine through commandline.
• Tested on MeasurementUI Editor with Sample Measurement.

[Blahpapto1]

For parsing Double2DArray value we are converting the default_value to string in _support.py. Also, handling repr(value) conversions in get_configuration_and_output_metadata_by_index method. This is one of the approach that we think is the best way to handle these conversions.

Another way, we can handle the parsing of Double2DArray value is to handle it after render_template method (before black._format_str method). This creates the output template then we fix all the instance in the generated output template through regex. But this happens during the _create_file method call, which may or may not be the best place to handle this case from a design perspective.

[github-actions]

Test Results

    40 files  ±0      40 suites  ±0   53m 12s ⏱️ + 2m 7s
   703 tests ±0     703 ✅ ±0      0 💤 ±0  0 ❌ ±0 
17 090 runs  ±0  16 020 ✅ ±0  1 070 💤 ±0  0 ❌ ±0 

Results for commit 1f5551c. ± Comparison against base commit 31c28b7.

♻️ This comment has been updated with latest results.

[dixonjoel]

Can you give an example of what 'default_value' is when it comes in here and what it looks like after you reconstruct it?

[dixonjoel]

I think this approach is ok. I'd like to hear what @bkeryan or @mshafer-NI think.

[bkeryan]

I disagree. I think this approach is unacceptable. str(some_double_2d_array) is for display purposes and not for parsing.

_format_default_value should downcast the default value to a Double2DArray object and use the object's fields to access its contents. If it needs more information than it currently has, then change the _format_default_value function to take value instead of value.default_value.

[Blahpapto1]

Default Value for Double2DArray before reconstruct -
rows: 2
columns: 3
data: 1
data: 2
data: 3
data: 4
data: 5
data: 6

After Reconstruct -
'Double2DArray(rows=2, columns=3, data=[1, 2, 3, 4, 5, 6])'

[Blahpapto1]

Now, the error (without reconstruct) is thrown because the generated template has Double2DArray value as default_value=rows: 2\ncolumns: 3\ndata: 1\ndata: 2\ndata: 3\ndata: 4\ndata: 5\ndata: 6\n,\r\n

and it is passed to this method

[Blahpapto1]

AssertionError: assert 1 == 0
E        +  where 1 = <Result InvalidInput('Cannot parse: 242:34:                 default_value=rows: 2')>.exit_code

tests\acceptance\test_client_generator.py:44: AssertionError

[clintverghese]

@bkeryan If I understand correctly, your point here is that default_value should contain the python data type (Double2DArray in this case) and any formatting should be done in the _format_default_value method defined in the template file.
_format_default_value should be checking if default_value is an instance of Double2DArray and do the necessary steps to convert it to the 'Double2DArray(rows=2, columns=3, data=[1, 2, 3, 4, 5, 6])' string.

That makes sense.

If the above understanding is correct then, one concern is that currently the default_value is added to the generated file not only through the _format_default_value method. Here the default value is being used directly and the reason that things work fine is because this piece of code incorporates the special string logic in _format_default_value.
Is that intentional?
If yes then, whatever Double2DArray conversion logic that is added into _format_default_value will need to be added here as well, or we would need this code to be passed through the _format_default_value workflow.

[bkeryan]

@clintverghese It may look like this is duplicated code, but get_configuration_parameters_with_type_and_default_values and _format_default_value are doing slightly different things.

get_configuration_parameters_with_type_and_default_values generates Python code for the client API class's measure() parameter list. In some cases we use more Pythonic data types in the client API. For example, path parameters are pathlib.Path in the client API and str in protobuf.

_format_default_value is used when generating Python code for the service's metadata structs. This tends to use the protobuf types or basic Python types, not stuff like pathlib.Path.

I think it would be fine to single-source _format_default_value by moving it into _support.py so that get_configuration_parameters_with_type_and_default_values can call it for non-path cases. Also, it would be helpful to update the docstrings to clarify this difference.

[Blahpapto1]

Moved the _format_default_value method to _support.py and added logic for Double2DArray. Now, we are calling this method from get_configuration_parameters_with_type_and_default_values for string and Double2dArray parameter types in _support.py and for the default value conversions in the template. Also, updated Docstring for both the methods.

[dixonjoel]

Please fill out the description especially what testing you've done interactively. I'll wait for review until the base branch goes into main. I don't see any major problems with the PR.

[bkeryan]

I disagree. I think this approach is unacceptable. str(some_double_2d_array) is for display purposes and not for parsing.

_format_default_value should downcast the default value to a Double2DArray object and use the object's fields to access its contents. If it needs more information than it currently has, then change the _format_default_value function to take value instead of value.default_value.

[bkeryan]

The code used to work this way, but we changed it for a reason. Storing the Python representation of the default value in place of the default value is confusing and makes the code hard to audit for XSS-style issues: https://bandit.readthedocs.io/en/latest/plugins/b702_use_of_mako_templates.html

[bkeryan]

Removing this function and storing the Python representation of the default value in place of the default value is not acceptable. Plus you didn't even remove the function, you left it as dead code.